diff mbox series

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

Message ID 20240626180031.4050226-20-cassel@kernel.org (mailing list archive)
State Superseded
Headers show
Series ata,libsas: Assign the unique id used for printing earlier | expand

Commit Message

Niklas Cassel June 26, 2024, 6 p.m. UTC 
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(-)

Comments

Damien Le Moal June 27, 2024, 1:15 a.m. UTC | #1 
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,
Hannes Reinecke June 27, 2024, 6:30 a.m. UTC | #2 
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
Niklas Cassel June 29, 2024, 12:09 p.m. UTC | #3 
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@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() ?

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
>
diff mbox series

Patch

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);
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 1c2400c96ebd..e8987dce585f 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 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);
 		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,