| Message ID | 1247701438-18266-1-git-send-email-mjg@redhat.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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
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.
> 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
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 ...
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 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.
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.
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
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.
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.
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
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.
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 */
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(-)