Re: [PATCH v2 05/13] ata,scsi: libata-core: Add ata_port_free()

Hannes Reinecke
2024-06-27T06:30:38Z

On 6/26/24 20: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).
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> drivers/ata/libata-core.c | 19 +++++++++++++------
> drivers/scsi/libsas/sas_ata.c | 2 +-
> drivers/scsi/libsas/sas_discover.c | 2 +-
> include/linux/libata.h | 1 +
> 4 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index c916cbe3e099..591020ea8989 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);
> @@ -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);
> host->ports[i] = NULL;
> }
> kfree(host);

Can't you squish this into the first patch?

Otherwise looks good.

Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich