[RFC] add port information for ATA devices in sysfs

[RFC] add port information for ATA devices in sysfs

[Nicolas Schichan]

Hi,

Currently, it is  not possible to know on what  particular port an ata
device is located  with sysfs.  the current scheme  creates an host<x>
directory directly under  the sata host device node  like this (with a
sata_mv driver, having two sata ports):

/sys/platform/sata_mv.0/host<x>
/sys/platform/sata_mv.0/host<y>

If the  system probes successfully  other scsi devices  before probing
the sata bus, we have no guarantees that the same numbers will be used
all the  time, and if we  rmmod/modprobe sata_mv the  host numbers are
not persistent accross module insertion.

The  inlined patch  fixes  this by  adding  a port<x>  device that  is
parented to the sata controller device  and uses this device as a base
to create the scsi hosts for the devices behind a given sata port. <x>
is set to be the value of the port_no field in struct ata_port.

the  naming then looks  like this  (assuming two  sata ports  behind a
sata_mv driver):

/sys/platform/sata_mv.0/port0/host<x>
/sys/platform/sata_mv.0/port1/host<y>

My  patch adds a  struct device  inside struct  ata_port for  the sole
purpose of having this port0,port1,... directory under sata_mv.

This patch also moves the  assignation of port_no to ata_port_alloc to
ease the initialisation of the device structure embedded in sata_port.
this most  likely messes up  the ata_sas_port_alloc code  which always
sets port_no to 0 and will make device_add complain loudly in dmesg if
this function  is called more than  once. Unfortunately I  do not have
any SAS devices at hand to fix this.

I do  not know yet  how this would  behave with sata devices  behind a
SATA port multiplier but I would  guess that no new scsi host would be
created in this  case and that the other  fields of target<host>:x:y:z
would be used by the scsi subsystem.

This is not a very polished patch  and it is aimed to know whether the
approach  used  is correct  and  I would  like  to  have your  advices
regarding this.

Regards,

Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>

-- 
Nicolas Schichan

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)
+{
+}
+
 /**
  *	ata_port_alloc - allocate and initialize basic ATA port resources
  *	@host: ATA host this allocated port belongs to
@@ -5672,7 +5676,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 +5694,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 */
@@ -5739,6 +5757,9 @@ static void ata_host_release(struct device *gendev, void *res)
 		if (ap->scsi_host)
 			scsi_host_put(ap->scsi_host);
 
+
+		device_unregister(&ap->ata_port_device);
+
 		kfree(ap->pmp_link);
 		kfree(ap->slave_link);
 		kfree(ap);
@@ -5797,11 +5818,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;

Re: [RFC] add port information for ATA devices in sysfs

[Greg KH]

On Mon, Apr 26, 2010 at 06:29:04PM +0200, Nicolas Schichan wrote:
> 
> Hi,
> 
> Currently, it is  not possible to know on what  particular port an ata
> device is located  with sysfs.  the current scheme  creates an host<x>
> directory directly under  the sata host device node  like this (with a
> sata_mv driver, having two sata ports):
> 
> /sys/platform/sata_mv.0/host<x>
> /sys/platform/sata_mv.0/host<y>
> 
> If the  system probes successfully  other scsi devices  before probing
> the sata bus, we have no guarantees that the same numbers will be used
> all the  time, and if we  rmmod/modprobe sata_mv the  host numbers are
> not persistent accross module insertion.
> 
> The  inlined patch  fixes  this by  adding  a port<x>  device that  is
> parented to the sata controller device  and uses this device as a base
> to create the scsi hosts for the devices behind a given sata port. <x>
> is set to be the value of the port_no field in struct ata_port.
> 
> the  naming then looks  like this  (assuming two  sata ports  behind a
> sata_mv driver):
> 
> /sys/platform/sata_mv.0/port0/host<x>
> /sys/platform/sata_mv.0/port1/host<y>
> 
> My  patch adds a  struct device  inside struct  ata_port for  the sole
> purpose of having this port0,port1,... directory under sata_mv.
> 
> This patch also moves the  assignation of port_no to ata_port_alloc to
> ease the initialisation of the device structure embedded in sata_port.
> this most  likely messes up  the ata_sas_port_alloc code  which always
> sets port_no to 0 and will make device_add complain loudly in dmesg if
> this function  is called more than  once. Unfortunately I  do not have
> any SAS devices at hand to fix this.
> 
> I do  not know yet  how this would  behave with sata devices  behind a
> SATA port multiplier but I would  guess that no new scsi host would be
> created in this  case and that the other  fields of target<host>:x:y:z
> would be used by the scsi subsystem.
> 
> This is not a very polished patch  and it is aimed to know whether the
> approach  used  is correct  and  I would  like  to  have your  advices
> regarding this.
> 
> Regards,
> 
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> 
> -- 
> Nicolas Schichan
> 
> 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.  Didn't you think that the
message was there for a reason, and it was not to be worked around?

{sigh}

greg k-h

Re: [RFC] add port information for ATA devices in sysfs

[Nicolas Schichan]

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>

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;



-- 
Nicolas Schichan

Re: [RFC] add port information for ATA devices in sysfs

[Greg KH]

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

Re: [RFC] add port information for ATA devices in sysfs

[Maxime Bizon]

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.

-- 
Maxime