Patchwork [1/4] scsi: Allow hosts to be flagged as hotpluggable

login
register
mail settings
Submitter Matthew Garrett
Date July 15, 2009, 11:43 p.m.
Message ID <1247701438-18266-1-git-send-email-mjg@redhat.com>
Download mbox | patch
Permalink /patch/29834/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Matthew Garrett - July 15, 2009, 11:43 p.m.
Userspace may wish to make policy decisions based on whether a host
supports device hotplug or not - for example, AHCI link power management
disables hotplug, so may only be desirable on hotplug ports. Add
support for marking hosts as hotpluggable in order to allow userspace to
treat them appropriately.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/scsi/hosts.c      |    1 +
 drivers/scsi/scsi_sysfs.c |    2 ++
 include/scsi/scsi_host.h  |    9 +++++++++
 3 files changed, 12 insertions(+), 0 deletions(-)
James Bottomley - July 16, 2009, 1:12 a.m.
On Thu, 2009-07-16 at 00:43 +0100, Matthew Garrett wrote:
> Userspace may wish to make policy decisions based on whether a host
> supports device hotplug or not - for example, AHCI link power management
> disables hotplug, so may only be desirable on hotplug ports. Add
> support for marking hosts as hotpluggable in order to allow userspace to
> treat them appropriately.

OK, so I don't really understand what the hotplug flag means.

You seem to be setting it unconditionally on most sata HBAs.  If it just
means "bus is hotpluggable", it should be set to 1 at initialisation and
the few non hot plug busses (like SPI) get to reset it.

However, by definition SATA (like SAS) is a hotplug bus ... why isn't it
set for some SATA controllers ... is it because the HBA itself does
something wrong when a hotplug event comes in?

James


--
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
Matthew Garrett - July 16, 2009, 1:26 a.m.
On Thu, Jul 16, 2009 at 01:12:28AM +0000, James Bottomley wrote:
> On Thu, 2009-07-16 at 00:43 +0100, Matthew Garrett wrote:
> > Userspace may wish to make policy decisions based on whether a host
> > supports device hotplug or not - for example, AHCI link power management
> > disables hotplug, so may only be desirable on hotplug ports. Add
> > support for marking hosts as hotpluggable in order to allow userspace to
> > treat them appropriately.
> 
> OK, so I don't really understand what the hotplug flag means.
> 
> You seem to be setting it unconditionally on most sata HBAs.  If it just
> means "bus is hotpluggable", it should be set to 1 at initialisation and
> the few non hot plug busses (like SPI) get to reset it.

It's a tossup. PATA's not hotpluggable (in the general case), so I just 
picked a default and went with it. Inverting it would be easy enough, I 
guess.

> However, by definition SATA (like SAS) is a hotplug bus ... why isn't it
> set for some SATA controllers ... is it because the HBA itself does
> something wrong when a hotplug event comes in?

Some older controllers don't provide direct access to the phy registers, 
so there's no way to interpret hotplug events correctly.
Alan Cox - July 16, 2009, 7:59 a.m.
> However, by definition SATA (like SAS) is a hotplug bus ... why isn't it
> set for some SATA controllers ... is it because the HBA itself does
> something wrong when a hotplug event comes in?

A lot of older controllers emulate various forms of the IDE interfaces.
They don't support detection/reporting of hotplug events to the OS. Some
of them support "warm-plug" - where you tell the controller the device is
going to go away, then remove it, then add a new one, then tell the
controller. Often they need to execute code when told these things (eg to
disable IORDY for PATA devices)

--
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
Matthew Wilcox - July 16, 2009, 11:59 a.m.
On Thu, Jul 16, 2009 at 12:43:55AM +0100, Matthew Garrett wrote:
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index b62a097..b099493 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -421,6 +421,7 @@ struct scsi_host_template {
>  	 */
>  	unsigned unchecked_isa_dma:1;
>  
> +
>  	/*
>  	 * True if this host adapter can make good use of clustering.
>  	 * I originally thought that if the tablesize was large that it

Spurious hunk ...
Stefan Richter - July 16, 2009, 2:30 p.m.
Matthew Garrett wrote:
> Userspace may wish to make policy decisions based on whether a host
> supports device hotplug or not - for example, AHCI link power management
> disables hotplug, so may only be desirable on hotplug ports. Add
> support for marking hosts as hotpluggable in order to allow userspace to
> treat them appropriately.
[...]
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -421,6 +421,7 @@ struct scsi_host_template {
>  	 */
>  	unsigned unchecked_isa_dma:1;
>  
> +
>  	/*
>  	 * True if this host adapter can make good use of clustering.
>  	 * I originally thought that if the tablesize was large that it
> @@ -447,6 +448,11 @@ struct scsi_host_template {
>  	unsigned ordered_tag:1;
>  
>  	/*
> +	 * True if host supports hotplugging
> +	 */
> +	unsigned hotpluggable:1;
> +

The comment should specify what the actual effects of the flag are.

(Provides the default for Scsi_Host.hotpluggable?)

> +	/*
>  	 * Countdown for host blocking with no commands outstanding.
>  	 */
>  	unsigned int max_host_blocked;
> @@ -622,6 +628,9 @@ struct Scsi_Host {
>  	/* Asynchronous scan in progress */
>  	unsigned async_scan:1;
>  
> +	/* 1 if hotpluggable, 0 if not */
> +	unsigned hotpluggable:1;
> +

Ditto here.

(Is used by power management infrastructure to decide over runtime PM
policy?  I.e. don't enter power states which would prevent the port from
detecting/ reporting hotplug events?)

>  	/*
>  	 * Optional work queue to be utilized by the transport
>  	 */
Stefan Richter - July 16, 2009, 2:36 p.m.
Stefan Richter wrote:
> The comment should specify what the actual effects of the flag are.
[...]
> (Is used by power management infrastructure to decide over runtime PM
> policy?  I.e. don't enter power states which would prevent the port from
> detecting/ reporting hotplug events?)

Perhaps the flag should be called differently (keep_ports_powered or
whatever) --- unless you have additional future uses of the flag in mind
which justify the more generic name.
Matthew Garrett - July 16, 2009, 2:38 p.m.
On Thu, Jul 16, 2009 at 04:30:13PM +0200, Stefan Richter wrote:
> Matthew Garrett wrote:
> > @@ -447,6 +448,11 @@ struct scsi_host_template {
> >  	unsigned ordered_tag:1;
> >  
> >  	/*
> > +	 * True if host supports hotplugging
> > +	 */
> > +	unsigned hotpluggable:1;
> > +
> 
> The comment should specify what the actual effects of the flag are.
> 
> (Provides the default for Scsi_Host.hotpluggable?)

Ok.

> >  
> > +	/* 1 if hotpluggable, 0 if not */
> > +	unsigned hotpluggable:1;
> > +
> 
> Ditto here.

There's no in-kernel effect of this flag - it's just exposed to 
userspace.

> (Is used by power management infrastructure to decide over runtime PM
> policy?  I.e. don't enter power states which would prevent the port from
> detecting/ reporting hotplug events?)

Exactly.
James Bottomley - July 16, 2009, 2:43 p.m.
On Thu, 2009-07-16 at 15:38 +0100, Matthew Garrett wrote:
> On Thu, Jul 16, 2009 at 04:30:13PM +0200, Stefan Richter wrote:
> > Matthew Garrett wrote:
> > > @@ -447,6 +448,11 @@ struct scsi_host_template {
> > >  	unsigned ordered_tag:1;
> > >  
> > >  	/*
> > > +	 * True if host supports hotplugging
> > > +	 */
> > > +	unsigned hotpluggable:1;
> > > +
> > 
> > The comment should specify what the actual effects of the flag are.
> > 
> > (Provides the default for Scsi_Host.hotpluggable?)
> 
> Ok.
> 
> > >  
> > > +	/* 1 if hotpluggable, 0 if not */
> > > +	unsigned hotpluggable:1;
> > > +
> > 
> > Ditto here.
> 
> There's no in-kernel effect of this flag - it's just exposed to 
> userspace.
> 
> > (Is used by power management infrastructure to decide over runtime PM
> > policy?  I.e. don't enter power states which would prevent the port from
> > detecting/ reporting hotplug events?)
> 
> Exactly.

OK, so if this is only in relation to SATA power management, put it in
libata and call it something like pm_capable.  That way we don't have to
work out what to do with it for the rest of SCSI.

James


--
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
Matthew Garrett - July 16, 2009, 2:44 p.m.
On Thu, Jul 16, 2009 at 04:36:14PM +0200, Stefan Richter wrote:
> Stefan Richter wrote:
> > The comment should specify what the actual effects of the flag are.
> [...]
> > (Is used by power management infrastructure to decide over runtime PM
> > policy?  I.e. don't enter power states which would prevent the port from
> > detecting/ reporting hotplug events?)
> 
> Perhaps the flag should be called differently (keep_ports_powered or
> whatever) --- unless you have additional future uses of the flag in mind
> which justify the more generic name.

Mm. I was actually wondering about that. For instance, laptop bays are 
hotpluggable but provide notification out of band and so can use ALPM 
without losing functionality. I'm not sure about "keep_ports_powered", 
but I'll try to think of something better.
Matthew Garrett - July 16, 2009, 2:45 p.m.
On Thu, Jul 16, 2009 at 02:43:19PM +0000, James Bottomley wrote:
> On Thu, 2009-07-16 at 15:38 +0100, Matthew Garrett wrote:
> > Exactly.
> 
> OK, so if this is only in relation to SATA power management, put it in
> libata and call it something like pm_capable.  That way we don't have to
> work out what to do with it for the rest of SCSI.

I have vague memories of this coming up before and it being suggested 
that it should be done at the SCSI layer instead, but I can't find that 
now. Doing it entirely in libata certainly isn't a problem.
James Bottomley - July 16, 2009, 2:53 p.m.
On Thu, 2009-07-16 at 15:45 +0100, Matthew Garrett wrote:
> On Thu, Jul 16, 2009 at 02:43:19PM +0000, James Bottomley wrote:
> > On Thu, 2009-07-16 at 15:38 +0100, Matthew Garrett wrote:
> > > Exactly.
> > 
> > OK, so if this is only in relation to SATA power management, put it in
> > libata and call it something like pm_capable.  That way we don't have to
> > work out what to do with it for the rest of SCSI.
> 
> I have vague memories of this coming up before and it being suggested 
> that it should be done at the SCSI layer instead, but I can't find that 
> now. Doing it entirely in libata certainly isn't a problem.

Well, a flag that says 'hotplug' and means both the controller and bus
support hotplugging might be SCSI specific.  However, the fact is that
most people make such a determination on the bus type, so it's a bit
redundant (in true SCSI there really is no controller on a hotplug bus
that doesn't support hotplug because they can't scan the bus without
it).  If you intend to use it to make link power management decisions,
that's completely different because SAS PM support still isn't
standardised and most of the rest of SCSI doesn't have it either.  So it
sounds to me you're looking for a flag that says "might have a problem
with SATA link power management" ... in which case this is currently
libata specific.  We might be able to expand it to libsas if (when) we
actually get link power management standardised, but a lot of the other
busses aren't even going to have a concept of link power management.

James


--
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
Matthew Garrett - July 16, 2009, 2:55 p.m.
On Thu, Jul 16, 2009 at 02:53:29PM +0000, James Bottomley wrote:

> Well, a flag that says 'hotplug' and means both the controller and bus
> support hotplugging might be SCSI specific.  However, the fact is that
> most people make such a determination on the bus type, so it's a bit
> redundant (in true SCSI there really is no controller on a hotplug bus
> that doesn't support hotplug because they can't scan the bus without
> it).  If you intend to use it to make link power management decisions,
> that's completely different because SAS PM support still isn't
> standardised and most of the rest of SCSI doesn't have it either.  So it
> sounds to me you're looking for a flag that says "might have a problem
> with SATA link power management" ... in which case this is currently
> libata specific.  We might be able to expand it to libsas if (when) we
> actually get link power management standardised, but a lot of the other
> busses aren't even going to have a concept of link power management.

Makes sense. I'll respin it as a libata feature.

Patch

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 5fd2da4..5a48bac 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -363,6 +363,7 @@  struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
 	shost->use_clustering = sht->use_clustering;
 	shost->ordered_tag = sht->ordered_tag;
+	shost->hotpluggable = sht->hotpluggable;
 
 	if (sht->supported_mode == MODE_UNKNOWN)
 		/* means we didn't set it ... default to INITIATOR */
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 91482f2..7669cbf 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -246,6 +246,7 @@  static DEVICE_ATTR(active_mode, S_IRUGO | S_IWUSR, show_shost_active_mode, NULL)
 
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(host_busy, "%hu\n");
+shost_rd_attr(hotpluggable, "%d\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
 shost_rd_attr(can_queue, "%hd\n");
 shost_rd_attr(sg_tablesize, "%hu\n");
@@ -257,6 +258,7 @@  shost_rd_attr2(proc_name, hostt->proc_name, "%s\n");
 static struct attribute *scsi_sysfs_shost_attrs[] = {
 	&dev_attr_unique_id.attr,
 	&dev_attr_host_busy.attr,
+	&dev_attr_hotpluggable.attr,
 	&dev_attr_cmd_per_lun.attr,
 	&dev_attr_can_queue.attr,
 	&dev_attr_sg_tablesize.attr,
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index b62a097..b099493 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -421,6 +421,7 @@  struct scsi_host_template {
 	 */
 	unsigned unchecked_isa_dma:1;
 
+
 	/*
 	 * True if this host adapter can make good use of clustering.
 	 * I originally thought that if the tablesize was large that it
@@ -447,6 +448,11 @@  struct scsi_host_template {
 	unsigned ordered_tag:1;
 
 	/*
+	 * True if host supports hotplugging
+	 */
+	unsigned hotpluggable:1;
+
+	/*
 	 * Countdown for host blocking with no commands outstanding.
 	 */
 	unsigned int max_host_blocked;
@@ -622,6 +628,9 @@  struct Scsi_Host {
 	/* Asynchronous scan in progress */
 	unsigned async_scan:1;
 
+	/* 1 if hotpluggable, 0 if not */
+	unsigned hotpluggable:1;
+
 	/*
 	 * Optional work queue to be utilized by the transport
 	 */