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

[Damien Le Moal <dlemoal@kernel.org>]

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@kernel.org>

> @@ -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() ?

>  		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