On Thu, Jun 27, 2024 at 10:15:37AM +0900, Damien Le Moal wrote:
> On 6/27/24 03:00, Niklas Cassel wrote:
> > Add a function, ata_port_free(), that is used to free a ata_port.
> > It makes sense to keep the code related to freeing a ata_port in its own
> > function, which will also free all the struct members of struct ata_port.
> >
> > libsas is currently not freeing all the struct ata_port struct members,
> > e.g. ncq_sense_buf for a driver supporting Command Duration Limits (CDL).
>
> This part should be a separate fix patch and sent out this cycle.
>
> >
> > Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
>
> > @@ -5518,12 +5530,7 @@ static void ata_host_release(struct kref *kref)
> > for (i = 0; i < host->n_ports; i++) {
> > struct ata_port *ap = host->ports[i];
> >
> > - if (ap) {
> > - kfree(ap->pmp_link);
> > - kfree(ap->slave_link);
> > - kfree(ap->ncq_sense_buf);
> > - kfree(ap);
> > - }
> > + ata_port_free(ap);
>
> The variable "ap" can be removed too.
>
> > host->ports[i] = NULL;
> > }
> > kfree(host);
>
> > diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> > index 6e01ddec10c9..951bdc554a10 100644
> > --- a/drivers/scsi/libsas/sas_discover.c
> > +++ b/drivers/scsi/libsas/sas_discover.c
> > @@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref)
> >
> > if (dev_is_sata(dev) && dev->sata_dev.ap) {
> > ata_tport_delete(dev->sata_dev.ap);
> > - kfree(dev->sata_dev.ap);
> > + ata_port_free(dev->sata_dev.ap);
>
> Why not have this inside ata_tport_delete() ?
In the current code, the allocating and freeing of the ports
are done separately from adding and deleting a transport.
The tport_del will be called by:
ahci_remove_one()
drivers/base/dd.c:__device_release_driver() will call
device_remove() which will call ahci_remove_one().
ahci_remove_one() (.remove()), will call ata_host_detach(), which calls
ata_port_detach(), which calls ata_tport_delete().
This will not free the port however. That is instead done even later, by:
ata_host_release() will be called even later:
drivers/base/dd.c:__device_release_driver() will call
device_unbind_cleanup(), which will call ata_host_release().
We could evaluate if we want to change this, since ALL libata driver do
call tport_add()/tport_delete(), but since this is a fix patch, I would
rather not do fundamental changes like that in this patch.
>
> > ata_host_put(dev->sata_dev.ata_host);
> > dev->sata_dev.ata_host = NULL;
> > dev->sata_dev.ap = NULL;
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 581e166615fa..586f0116d1d7 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1249,6 +1249,7 @@ extern int ata_slave_link_init(struct ata_port *ap);
> > extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
> > struct ata_port_info *, struct Scsi_Host *);
> > extern void ata_port_probe(struct ata_port *ap);
> > +extern void ata_port_free(struct ata_port *ap);
> > extern int ata_tport_add(struct device *parent, struct ata_port *ap);
> > extern void ata_tport_delete(struct ata_port *ap);
> > int ata_sas_device_configure(struct scsi_device *sdev, struct queue_limits *lim,
>
> --
> Damien Le Moal
> Western Digital Research
>
