| Message ID | 20240629124210.181537-8-cassel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | libata/libsas cleanup fixes | expand |
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@kernel.org> Apart from a couple of nitpicks: Reviewed-by: John Garry <john.g.garry@oracle.com> > --- > 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. > + > + 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. > 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,
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@kernel.org> > > Apart from a couple of nitpicks: > > Reviewed-by: John Garry <john.g.garry@oracle.com> > > > --- > > 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, >
On 6/29/24 14: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@kernel.org> > --- > 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(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
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; + + 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); 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,
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@kernel.org> --- 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(-)