Patchwork [RFC] add port information for ATA devices in sysfs

login
register
mail settings
Submitter Nicolas Schichan
Date April 27, 2010, 1:18 p.m.
Message ID <201004271518.13611.nschichan@freebox.fr>
Download mbox | patch
Permalink /patch/51077/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Nicolas Schichan - April 27, 2010, 1:18 p.m.
On Tuesday 27 April 2010 05:42:10 am Greg KH wrote:
> On Mon, Apr 26, 2010 at 06:29:04PM +0200, Nicolas Schichan wrote:

> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 91fed3c..179abad 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -5660,6 +5660,10 @@ int sata_link_init_spd(struct ata_link *link)
> >  	return 0;
> >  }
> >
> > +static void ata_port_dev_release(struct device *dev)
> > +{
> > +}
>
> {sigh}
>
> By doing the above, you have guaranteed that I will make fun of this
> code.  Consider this the ridicule it deserves.
>
> (hint, read the kobject documentation for why I get to make fun of
> it...)
>
> Think about why you created an empty release function, to try to get the
> kernel to stop spitting out a message to you. 

That's right, shame on me for not reading the documentation.

>  Didn't you think that the
> message was there for a reason, and it was not to be worked around?

Well after reading  the kobject documentation, I understand  why it is
bad thing to have this function empty.  Since someone may still hold a
reference on the  device when I call device_unregister(),  I guess the
only safe place  where to kfree the struct ata_port  is in the release
callback of the device.

Please find an updated patch addressing your comments below:

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
Greg KH - April 28, 2010, 5:50 a.m.
On Tue, Apr 27, 2010 at 03:18:13PM +0200, Nicolas Schichan wrote:
> On Tuesday 27 April 2010 05:42:10 am Greg KH wrote:
> > On Mon, Apr 26, 2010 at 06:29:04PM +0200, Nicolas Schichan wrote:
> 
> > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > > index 91fed3c..179abad 100644
> > > --- a/drivers/ata/libata-core.c
> > > +++ b/drivers/ata/libata-core.c
> > > @@ -5660,6 +5660,10 @@ int sata_link_init_spd(struct ata_link *link)
> > >  	return 0;
> > >  }
> > >
> > > +static void ata_port_dev_release(struct device *dev)
> > > +{
> > > +}
> >
> > {sigh}
> >
> > By doing the above, you have guaranteed that I will make fun of this
> > code.  Consider this the ridicule it deserves.
> >
> > (hint, read the kobject documentation for why I get to make fun of
> > it...)
> >
> > Think about why you created an empty release function, to try to get the
> > kernel to stop spitting out a message to you. 
> 
> That's right, shame on me for not reading the documentation.
> 
> >  Didn't you think that the
> > message was there for a reason, and it was not to be worked around?
> 
> Well after reading  the kobject documentation, I understand  why it is
> bad thing to have this function empty.  Since someone may still hold a
> reference on the  device when I call device_unregister(),  I guess the
> only safe place  where to kfree the struct ata_port  is in the release
> callback of the device.
> 
> Please find an updated patch addressing your comments below:

Looks better, thanks.

As for the whole idea of the extra device (which it doesn't look like
you ever initialize anywhere), it's not good to have one sitting in the
middle of the device chain that isn't owned by a bus somehow.  That just
looks wierd and can cause problems with udev rules.

why is this really needed at all?  Can't you just export the port number
in the device as an attribute instead?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Bizon - April 28, 2010, 2:57 p.m.
On Tue, 2010-04-27 at 22:50 -0700, Greg KH wrote:

Hi Greg,

> As for the whole idea of the extra device (which it doesn't look like
> you ever initialize anywhere), it's not good to have one sitting in the
> middle of the device chain that isn't owned by a bus somehow.  That just
> looks wierd and can cause problems with udev rules.
> 
> why is this really needed at all?  Can't you just export the port number
> in the device as an attribute instead?

You're right, please disregard this. We got fooled by the incrementing
scsi host id in the device path.

The misleading part (for me) is that even if a device sysfs devpath
*seems* to represent its topology, some parts of the path are not
"persistent" (across simple rmmod/modprobe)

I just read udev path_id source code and saw the trick for the scsi host
number (which would have been broken with this patch). Since the ata
code always creates scsi hosts in ata port order, we can assume that
lowest scsi host number is the first ata port, so the patch is not
needed.

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 91fed3c..f96d8c0 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5660,6 +5660,14 @@  int sata_link_init_spd(struct ata_link *link)
 	return 0;
 }
 
+static void ata_port_dev_release(struct device *dev)
+{
+	struct ata_port *ap;
+
+	ap = container_of(dev, struct ata_port, ata_port_device);
+	kfree(ap);
+}
+
 /**
  *	ata_port_alloc - allocate and initialize basic ATA port resources
  *	@host: ATA host this allocated port belongs to
@@ -5672,7 +5680,7 @@  int sata_link_init_spd(struct ata_link *link)
  *	LOCKING:
  *	Inherited from calling layer (may sleep).
  */
-struct ata_port *ata_port_alloc(struct ata_host *host)
+struct ata_port *ata_port_alloc(struct ata_host *host, int port_no)
 {
 	struct ata_port *ap;
 
@@ -5690,6 +5698,20 @@  struct ata_port *ata_port_alloc(struct ata_host *host)
 	ap->host = host;
 	ap->dev = host->dev;
 	ap->last_ctl = 0xFF;
+	ap->port_no = port_no;
+
+	/*
+	 * register device used to add the scsi host behind this
+	 * port. make it a parent of the struct device of the host.
+	 */
+	ap->ata_port_device.release = ata_port_dev_release;
+	dev_set_name(&ap->ata_port_device, "port%i", ap->port_no);
+	ap->ata_port_device.parent = ap->dev;
+	if (device_register(&ap->ata_port_device) < 0) {
+		kfree(ap);
+		return NULL;
+	}
+
 
 #if defined(ATA_VERBOSE_DEBUG)
 	/* turn on all debugging levels */
@@ -5741,7 +5763,11 @@  static void ata_host_release(struct device *gendev, 
void *res)
 
 		kfree(ap->pmp_link);
 		kfree(ap->slave_link);
-		kfree(ap);
+
+		/*
+		 * ap will be kfree'ed in device release callback.
+		 */
+		device_unregister(&ap->ata_port_device);
 		host->ports[i] = NULL;
 	}
 
@@ -5797,11 +5823,10 @@  struct ata_host *ata_host_alloc(struct device *dev, 
int max_ports)
 	for (i = 0; i < max_ports; i++) {
 		struct ata_port *ap;
 
-		ap = ata_port_alloc(host);
+		ap = ata_port_alloc(host, i);
 		if (!ap)
 			goto err_out;
 
-		ap->port_no = i;
 		host->ports[i] = ap;
 	}
 
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b4ee28d..1cbda75 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3210,7 +3210,7 @@  int ata_scsi_add_hosts(struct ata_host *host, struct 
scsi_host_template *sht)
 		 */
 		shost->max_host_blocked = 1;
 
-		rc = scsi_add_host(ap->scsi_host, ap->host->dev);
+		rc = scsi_add_host(ap->scsi_host, &ap->ata_port_device);
 		if (rc)
 			goto err_add;
 	}
@@ -3593,11 +3593,10 @@  struct ata_port *ata_sas_port_alloc(struct ata_host 
*host,
 {
 	struct ata_port *ap;
 
-	ap = ata_port_alloc(host);
+	ap = ata_port_alloc(host, 0);
 	if (!ap)
 		return NULL;
 
-	ap->port_no = 0;
 	ap->lock = shost->host_lock;
 	ap->pio_mask = port_info->pio_mask;
 	ap->mwdma_mask = port_info->mwdma_mask;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 823e630..cd44e9e 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -112,7 +112,7 @@  extern void ata_link_init(struct ata_port *ap, struct 
ata_link *link, int pmp);
 extern int sata_link_init_spd(struct ata_link *link);
 extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
-extern struct ata_port *ata_port_alloc(struct ata_host *host);
+extern struct ata_port *ata_port_alloc(struct ata_host *host, int port_no);
 extern void ata_dev_enable_pm(struct ata_device *dev, enum link_pm policy);
 extern void ata_lpm_schedule(struct ata_port *ap, enum link_pm);
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b0f6d97..64d94d9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -749,6 +749,9 @@  struct ata_port {
 	struct ata_host		*host;
 	struct device 		*dev;
 
+	/* used as base to create scsi host behind the port.*/
+	struct device		ata_port_device;
+
 	void			*port_task_data;
 	struct delayed_work	port_task;
 	struct delayed_work	hotplug_task;