On Sun, Jun 30, 2024 at 10:42:45AM +0100, John Garry wrote:
> On 29/06/2024 13:42, Niklas Cassel wrote:
> > 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).
> >
> > Add a function, ata_port_free(), that is used to free a ata_port,
> > including its struct members. 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.
> >
> > Fixes: 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")
> > Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
>
> Apart from a couple of nitpicks:
>
> Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
>
> > ---
> > drivers/ata/libata-core.c | 24 ++++++++++++++----------
> > drivers/scsi/libsas/sas_ata.c | 2 +-
> > drivers/scsi/libsas/sas_discover.c | 2 +-
> > include/linux/libata.h | 1 +
> > 4 files changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index f47838da75d7..481baa55ebfc 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -5490,6 +5490,18 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
> > return ap;
> > }
> > +void ata_port_free(struct ata_port *ap)
> > +{
> > + if (!ap)
> > + return;
>
> nit: personally I'd have the caller check this. The main reason for that is
> that often it seems an unnecessary check.
People are used to free() family of functions handling NULL,
so I think it is wise to keep it as is.
>
> > +
> > + kfree(ap->pmp_link);
> > + kfree(ap->slave_link);
> > + kfree(ap->ncq_sense_buf);
> > + kfree(ap);
> > +}
> > +EXPORT_SYMBOL_GPL(ata_port_free);
> > +
> > static void ata_devres_release(struct device *gendev, void *res)
> > {
> > struct ata_host *host = dev_get_drvdata(gendev);
> > @@ -5516,15 +5528,7 @@ static void ata_host_release(struct kref *kref)
> > int i;
> > for (i = 0; i < host->n_ports; i++) {
> > - struct ata_port *ap = host->ports[i];
> > -
> > - if (!ap)
> > - continue;
> > -
> > - kfree(ap->pmp_link);
> > - kfree(ap->slave_link);
> > - kfree(ap->ncq_sense_buf);
> > - kfree(ap);
> > + ata_port_free(host->ports[i]);
> > host->ports[i] = NULL;
> > }
> > kfree(host);
> > @@ -5907,7 +5911,7 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh
> > * allocation time.
> > */
> > for (i = host->n_ports; host->ports[i]; i++)
> > - kfree(host->ports[i]);
> > + ata_port_free(host->ports[i]);
> > /* give ports names and add SCSI hosts */
> > for (i = 0; i < host->n_ports; i++) {
> > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> > index 4c69fc63c119..1f247a8cd185 100644
> > --- a/drivers/scsi/libsas/sas_ata.c
> > +++ b/drivers/scsi/libsas/sas_ata.c
> > @@ -618,7 +618,7 @@ int sas_ata_init(struct domain_device *found_dev)
> > return 0;
> > destroy_port:
> > - kfree(ap);
> > + ata_port_free(ap);
>
> nit: If not a nuisance, could we change the label name to free_port, like
> free_host, below. No big deal.
Sure, will fixup when applying.
>
> > free_host:
> > ata_host_put(ata_host);
> > return rc;
> > diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> > index 8fb7c41c0962..48d975c6dbf2 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_sas_tport_delete(dev->sata_dev.ap);
> > - kfree(dev->sata_dev.ap);
> > + ata_port_free(dev->sata_dev.ap);
> > 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 13fb41d25da6..7d3bd7c9664a 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_sas_tport_add(struct device *parent, struct ata_port *ap);
> > extern void ata_sas_tport_delete(struct ata_port *ap);
> > int ata_sas_device_configure(struct scsi_device *sdev, struct queue_limits *lim,
>
