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

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

[Matthew Garrett]

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(-)

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
 	 */
-- 
1.6.2.5

[PATCH 2/4] libata: Flag most SATA ports as hotpluggable

[Matthew Garrett]

SATA is generally hotpluggable, though some controllers don't expose
enough information to make it possible. Flag most SATA controllers as
supporting hotplug, leaving the remaining ones (ich in PIIX mode, Promise
SX4, Pacific Digital Talon) marked as not supporting it.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/ata/ahci.c          |    1 +
 drivers/ata/sata_fsl.c      |    1 +
 drivers/ata/sata_inic162x.c |    1 +
 drivers/ata/sata_mv.c       |    2 ++
 drivers/ata/sata_nv.c       |    3 +++
 drivers/ata/sata_promise.c  |    1 +
 drivers/ata/sata_qstor.c    |    1 +
 drivers/ata/sata_sil.c      |    3 ++-
 drivers/ata/sata_sil24.c    |    1 +
 drivers/ata/sata_sis.c      |    1 +
 drivers/ata/sata_svw.c      |    1 +
 drivers/ata/sata_uli.c      |    1 +
 drivers/ata/sata_via.c      |    1 +
 drivers/ata/sata_vsc.c      |    1 +
 14 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 15a2303..de84054 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -347,6 +347,7 @@ static struct scsi_host_template ahci_sht = {
 	.dma_boundary		= AHCI_DMA_BOUNDARY,
 	.shost_attrs		= ahci_shost_attrs,
 	.sdev_attrs		= ahci_sdev_attrs,
+	.hotpluggable		= 1,
 };
 
 static struct ata_port_operations ahci_ops = {
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index 94eaa43..e7738c7 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -1252,6 +1252,7 @@ static struct scsi_host_template sata_fsl_sht = {
 	.can_queue = SATA_FSL_QUEUE_DEPTH,
 	.sg_tablesize = SATA_FSL_MAX_PRD_USABLE,
 	.dma_boundary = ATA_DMA_BOUNDARY,
+	.hotpluggable = 1,
 };
 
 static struct ata_port_operations sata_fsl_ops = {
diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c
index 8d890cc..f51e71d 100644
--- a/drivers/ata/sata_inic162x.c
+++ b/drivers/ata/sata_inic162x.c
@@ -234,6 +234,7 @@ static struct scsi_host_template inic_sht = {
 	ATA_BASE_SHT(DRV_NAME),
 	.sg_tablesize	= LIBATA_MAX_PRD,	/* maybe it can be larger? */
 	.dma_boundary	= INIC_DMA_BOUNDARY,
+	.hotpluggable	= 1,
 };
 
 static const int scr_map[] = {
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 23714ae..8d61998 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -647,6 +647,7 @@ static struct scsi_host_template mv5_sht = {
 	ATA_BASE_SHT(DRV_NAME),
 	.sg_tablesize		= MV_MAX_SG_CT / 2,
 	.dma_boundary		= MV_DMA_BOUNDARY,
+	.hotpluggable		= 1,
 };
 
 static struct scsi_host_template mv6_sht = {
@@ -654,6 +655,7 @@ static struct scsi_host_template mv6_sht = {
 	.can_queue		= MV_MAX_Q_DEPTH - 1,
 	.sg_tablesize		= MV_MAX_SG_CT / 2,
 	.dma_boundary		= MV_DMA_BOUNDARY,
+	.hotpluggable		= 1,
 };
 
 static struct ata_port_operations mv5_ops = {
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index b2d11f3..9e93c56 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -388,6 +388,7 @@ static struct pci_driver nv_pci_driver = {
 
 static struct scsi_host_template nv_sht = {
 	ATA_BMDMA_SHT(DRV_NAME),
+	.hotpluggable		= 1,
 };
 
 static struct scsi_host_template nv_adma_sht = {
@@ -396,6 +397,7 @@ static struct scsi_host_template nv_adma_sht = {
 	.sg_tablesize		= NV_ADMA_SGTBL_TOTAL_LEN,
 	.dma_boundary		= NV_ADMA_DMA_BOUNDARY,
 	.slave_configure	= nv_adma_slave_config,
+	.hotpluggable		= 1,
 };
 
 static struct scsi_host_template nv_swncq_sht = {
@@ -404,6 +406,7 @@ static struct scsi_host_template nv_swncq_sht = {
 	.sg_tablesize		= LIBATA_MAX_PRD,
 	.dma_boundary		= ATA_DMA_BOUNDARY,
 	.slave_configure	= nv_swncq_slave_config,
+	.hotpluggable		= 1,
 };
 
 /*
diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index b1fd7d6..abddf93 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -166,6 +166,7 @@ static struct scsi_host_template pdc_ata_sht = {
 	ATA_BASE_SHT(DRV_NAME),
 	.sg_tablesize		= PDC_MAX_PRD,
 	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.hotpluggable		= 1,
 };
 
 static const struct ata_port_operations pdc_common_ops = {
diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
index 326c0cf..f9db8fe 100644
--- a/drivers/ata/sata_qstor.c
+++ b/drivers/ata/sata_qstor.c
@@ -130,6 +130,7 @@ static struct scsi_host_template qs_ata_sht = {
 	ATA_BASE_SHT(DRV_NAME),
 	.sg_tablesize		= QS_MAX_PRD,
 	.dma_boundary		= QS_DMA_BOUNDARY,
+	.hotpluggable		= 1,
 };
 
 static struct ata_port_operations qs_ata_ops = {
diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index 030ec07..7237abb 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -179,7 +179,8 @@ static struct scsi_host_template sil_sht = {
 	    transfer chunks up to 2GB and which cross 64KB boundaries,
 	    therefore the DMA limits are more relaxed than standard ATA SFF. */
 	.dma_boundary		= SIL_DMA_BOUNDARY,
-	.sg_tablesize		= ATA_MAX_PRD
+	.sg_tablesize		= ATA_MAX_PRD,
+	.hotpluggable		= 1,
 };
 
 static struct ata_port_operations sil_ops = {
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 77aa8d7..166be9e 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -386,6 +386,7 @@ static struct scsi_host_template sil24_sht = {
 	.can_queue		= SIL24_MAX_CMDS,
 	.sg_tablesize		= SIL24_MAX_SGE,
 	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.hotpluggable		= 1,
 };
 
 static struct ata_port_operations sil24_ops = {
diff --git a/drivers/ata/sata_sis.c b/drivers/ata/sata_sis.c
index 8f98332..4cc5469 100644
--- a/drivers/ata/sata_sis.c
+++ b/drivers/ata/sata_sis.c
@@ -87,6 +87,7 @@ static struct pci_driver sis_pci_driver = {
 
 static struct scsi_host_template sis_sht = {
 	ATA_BMDMA_SHT(DRV_NAME),
+	.hotpluggable		= 1,
 };
 
 static struct ata_port_operations sis_ops = {
diff --git a/drivers/ata/sata_svw.c b/drivers/ata/sata_svw.c
index 7257f2d..2f90770 100644
--- a/drivers/ata/sata_svw.c
+++ b/drivers/ata/sata_svw.c
@@ -341,6 +341,7 @@ static struct scsi_host_template k2_sata_sht = {
 #ifdef CONFIG_PPC_OF
 	.proc_info		= k2_sata_proc_info,
 #endif
+	.hotpluggable		= 1,
 };
 
 
diff --git a/drivers/ata/sata_uli.c b/drivers/ata/sata_uli.c
index e5bff47..9f2ae95 100644
--- a/drivers/ata/sata_uli.c
+++ b/drivers/ata/sata_uli.c
@@ -77,6 +77,7 @@ static struct pci_driver uli_pci_driver = {
 
 static struct scsi_host_template uli_sht = {
 	ATA_BMDMA_SHT(DRV_NAME),
+	.hotpluggable		= 1,
 };
 
 static struct ata_port_operations uli_ops = {
diff --git a/drivers/ata/sata_via.c b/drivers/ata/sata_via.c
index bdd43c7..c554b2e 100644
--- a/drivers/ata/sata_via.c
+++ b/drivers/ata/sata_via.c
@@ -111,6 +111,7 @@ static struct pci_driver svia_pci_driver = {
 
 static struct scsi_host_template svia_sht = {
 	ATA_BMDMA_SHT(DRV_NAME),
+	.hotpluggable		= 1,
 };
 
 static struct ata_port_operations svia_base_ops = {
diff --git a/drivers/ata/sata_vsc.c b/drivers/ata/sata_vsc.c
index 8b2a278..dc58b2b 100644
--- a/drivers/ata/sata_vsc.c
+++ b/drivers/ata/sata_vsc.c
@@ -303,6 +303,7 @@ out:
 
 static struct scsi_host_template vsc_sata_sht = {
 	ATA_BMDMA_SHT(DRV_NAME),
+	.hotpluggable		= 1,
 };
 
 
-- 
1.6.2.5

[PATCH 3/4] libata: Make it possible for host drivers to flag hotplug ports

[Matthew Garrett]

Some libata host drivers may be able to provide hotplug information
on a port by port basis. Add a port operation to allow this to be
done, and ensure that it's called during registration.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/ata/libata-core.c |    4 ++++
 include/linux/libata.h    |    1 +
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 045a486..e4efd68 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6120,6 +6120,10 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 		xfer_mask = ata_pack_xfermask(ap->pio_mask, ap->mwdma_mask,
 					      ap->udma_mask);
 
+		if (ap->ops->is_hotpluggable)
+			ap->scsi_host->hotpluggable =
+				ap->ops->is_hotpluggable(ap);
+
 		if (!ata_port_is_dummy(ap)) {
 			ata_port_printk(ap, KERN_INFO,
 					"%cATA max %s %s\n",
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3d501db..7b1da91 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -814,6 +814,7 @@ struct ata_port_operations {
 	void (*pmp_detach)(struct ata_port *ap);
 	int  (*enable_pm)(struct ata_port *ap, enum link_pm policy);
 	void (*disable_pm)(struct ata_port *ap);
+	int  (*is_hotpluggable)(struct ata_port *ap);
 
 	/*
 	 * Start, stop, suspend and resume
-- 
1.6.2.5

[PATCH 4/4] libata: Allow AHCI to flag ports as not hotpluggable

[Matthew Garrett]

Some SATA ports are fully internal and unlikely to be hotplugged. The
AHCI spec allows vendors to flag these ports differently, allowing
hotplug bays and eSATA ports to be treated differently. Ensure that
this information is exposed to userspace so policy decisions can be
made.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/ata/ahci.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index de84054..51d8350 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -77,6 +77,7 @@ static ssize_t ahci_led_store(struct ata_port *ap, const char *buf,
 			      size_t size);
 static ssize_t ahci_transmit_led_message(struct ata_port *ap, u32 state,
 					ssize_t size);
+static int ahci_is_hotplug_capable(struct ata_port *ap);
 
 enum {
 	AHCI_PCI_BAR		= 5,
@@ -193,6 +194,8 @@ enum {
 	PORT_CMD_ASP		= (1 << 27), /* Aggressive Slumber/Partial */
 	PORT_CMD_ALPE		= (1 << 26), /* Aggressive Link PM enable */
 	PORT_CMD_ATAPI		= (1 << 24), /* Device is ATAPI */
+	PORT_CMD_ESP		= (1 << 21), /* port is esata capable */
+	PORT_CMD_HPCP           = (1 << 18), /* port is hot plug capable */
 	PORT_CMD_PMP		= (1 << 17), /* PMP attached */
 	PORT_CMD_LIST_ON	= (1 << 15), /* cmd list DMA engine running */
 	PORT_CMD_FIS_ON		= (1 << 14), /* FIS DMA engine running */
@@ -373,6 +376,8 @@ static struct ata_port_operations ahci_ops = {
 	.pmp_attach		= ahci_pmp_attach,
 	.pmp_detach		= ahci_pmp_detach,
 
+	.is_hotpluggable	= ahci_is_hotplug_capable,
+
 	.enable_pm		= ahci_enable_alpm,
 	.disable_pm		= ahci_disable_alpm,
 	.em_show		= ahci_led_show,
@@ -876,6 +881,15 @@ static int ahci_scr_write(struct ata_link *link, unsigned int sc_reg, u32 val)
 	return -EINVAL;
 }
 
+static int ahci_is_hotplug_capable(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u8 cmd;
+
+	cmd = readl(port_mmio + PORT_CMD);
+	return cmd & (PORT_CMD_HPCP | PORT_CMD_ESP);
+}
+
 static void ahci_start_engine(struct ata_port *ap)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
-- 
1.6.2.5

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

[James Bottomley]

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

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

[Matthew Garrett]

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.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Alan Cox]

> 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)

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

[Matthew Wilcox]

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 Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

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

[Stefan Richter]

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
-=====-==--= -=== =----
http://arcgraph.de/sr/

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

[Stefan Richter]

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.
-- 
Stefan Richter
-=====-==--= -=== =----
http://arcgraph.de/sr/

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

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

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

[Matthew Garrett]

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.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[James Bottomley]

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

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

[Matthew Garrett]

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.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[James Bottomley]

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

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

[Matthew Garrett]

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.

-- 
Matthew Garrett | mjg59@srcf.ucam.org