Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Jeff Garzik]

Alan Cox wrote:
> Something like this then ?
> 
> 
> The legacy old IDE API for this is a bit primitive so we try and map stuff
> sensibly onto it.
> 
> - Add flags2 as we ran out of flags
> - Set PIO over DMA devices to report 32bit
> - Add ability to change the PIO32 settings if the controller permits it
> - Add that functionality into the sff drivers
> - Add that functionality into the VLB legacy driver
> - Turn on the 32bit PIO on the ninja32 and add support there
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>

I think the flag2 stuff needs a closer look WRT driver setup, but 
overall looks good.

Consider it ACK'd, if I cannot come up with anything better for flags2 :)

	Jeff

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Alan Cox]

On Thu, 09 Apr 2009 08:50:10 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Alan Cox wrote:
> > Something like this then ?
> > 
> > 
> > The legacy old IDE API for this is a bit primitive so we try and map stuff
> > sensibly onto it.
> > 
> > - Add flags2 as we ran out of flags
> > - Set PIO over DMA devices to report 32bit
> > - Add ability to change the PIO32 settings if the controller permits it
> > - Add that functionality into the sff drivers
> > - Add that functionality into the VLB legacy driver
> > - Turn on the 32bit PIO on the ninja32 and add support there
> > 
> > Signed-off-by: Alan Cox <alan@linux.intel.com>
> 
> I think the flag2 stuff needs a closer look WRT driver setup, but 
> overall looks good.
> 
> Consider it ACK'd, if I cannot come up with anything better for flags2 

The alternative would be to remove the private 8 flags from flags but
that means fairly excessive changes to all the devices

[Looks smug and notes that he pointed out that driver private flags in
the same field would turn out to be a bad idea early on ...]

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Mark Lord]

Alan Cox wrote:
> Something like this then ?
> 
> 
> The legacy old IDE API for this is a bit primitive so we try and map stuff
> sensibly onto it.
> 
> - Add flags2 as we ran out of flags
> - Set PIO over DMA devices to report 32bit
> - Add ability to change the PIO32 settings if the controller permits it
> - Add that functionality into the sff drivers
> - Add that functionality into the VLB legacy driver
> - Turn on the 32bit PIO on the ninja32 and add support there
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
..

By all means go ahead with something like this,
and thanks for doing it, Alan.

But.. these do look rather suspiciously like sysfs attrs to me.

We didn't have a sysfs in the IDE days (so Gadi added the klunky
procfs "settings" thing for there), but we do now.

I'm happy to update hdparm to check sysfs if that's where this
ends up, or to do almost nothing for it if we stick with the ioctls.

Thanks either way!

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

Alan Cox wrote:
> On Thu, 09 Apr 2009 08:50:10 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> Alan Cox wrote:
>>> Something like this then ?
>>>
>>>
>>> The legacy old IDE API for this is a bit primitive so we try and map stuff
>>> sensibly onto it.
>>>
>>> - Add flags2 as we ran out of flags
>>> - Set PIO over DMA devices to report 32bit
>>> - Add ability to change the PIO32 settings if the controller permits it
>>> - Add that functionality into the sff drivers
>>> - Add that functionality into the VLB legacy driver
>>> - Turn on the 32bit PIO on the ninja32 and add support there
>>>
>>> Signed-off-by: Alan Cox <alan@linux.intel.com>
>> I think the flag2 stuff needs a closer look WRT driver setup, but 
>> overall looks good.
>>
>> Consider it ACK'd, if I cannot come up with anything better for flags2 
> 
> The alternative would be to remove the private 8 flags from flags but
> that means fairly excessive changes to all the devices

Well the obvious alternative is attached...  kick the can down the road.


> [Looks smug and notes that he pointed out that driver private flags in
> the same field would turn out to be a bad idea early on ...]

True.  At present the flags are inherited from the ata_port_info 
structure, so introducing a driver_flags field isn't as trivial as it 
might seem, unfortunately.

With the attached patch, you can use ->flags for the two new flags, and 
you can set those bits in the driver where you set the other 
ATA_FLAG_xxx bits.

	Jeff




[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2495 bytes --]

diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index b1fd7d6..c7bd4b4 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -127,9 +127,9 @@ enum {
 				  ATA_FLAG_PIO_POLLING,
 
 	/* ap->flags bits */
-	PDC_FLAG_GEN_II		= (1 << 24),
-	PDC_FLAG_SATA_PATA	= (1 << 25), /* supports SATA + PATA */
-	PDC_FLAG_4_PORTS	= (1 << 26), /* 4 ports */
+	PDC_FLAG_GEN_II		= (1 << 29),
+	PDC_FLAG_SATA_PATA	= (1 << 30), /* supports SATA + PATA */
+	PDC_FLAG_4_PORTS	= (1 << 31), /* 4 ports */
 };
 
 struct pdc_port_priv {
diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index e67ce8e..d21afc7 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -57,9 +57,9 @@ enum {
 	/*
 	 * host flags
 	 */
-	SIL_FLAG_NO_SATA_IRQ	= (1 << 28),
-	SIL_FLAG_RERR_ON_DMA_ACT = (1 << 29),
-	SIL_FLAG_MOD15WRITE	= (1 << 30),
+	SIL_FLAG_NO_SATA_IRQ	= (1 << 29),
+	SIL_FLAG_RERR_ON_DMA_ACT= (1 << 30),
+	SIL_FLAG_MOD15WRITE	= (1 << 31),
 
 	SIL_DFL_PORT_FLAGS	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				  ATA_FLAG_MMIO,
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 77aa8d7..3852a1c 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -247,7 +247,7 @@ enum {
 				  ATA_FLAG_MMIO | ATA_FLAG_PIO_DMA |
 				  ATA_FLAG_NCQ | ATA_FLAG_ACPI_SATA |
 				  ATA_FLAG_AN | ATA_FLAG_PMP,
-	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 24), /* IRQ loss errata on PCI-X */
+	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 31), /* IRQ loss errata on PCI-X */
 
 	IRQ_STAT_4PORTS		= 0xf,
 };
diff --git a/drivers/ata/sata_svw.c b/drivers/ata/sata_svw.c
index 7257f2d..3c37ed0 100644
--- a/drivers/ata/sata_svw.c
+++ b/drivers/ata/sata_svw.c
@@ -59,9 +59,9 @@
 
 enum {
 	/* ap->flags bits */
-	K2_FLAG_SATA_8_PORTS		= (1 << 24),
-	K2_FLAG_NO_ATAPI_DMA		= (1 << 25),
-	K2_FLAG_BAR_POS_3			= (1 << 26),
+	K2_FLAG_SATA_8_PORTS		= (1 << 29),
+	K2_FLAG_NO_ATAPI_DMA		= (1 << 30),
+	K2_FLAG_BAR_POS_3		= (1 << 31),
 
 	/* Taskfile registers offsets */
 	K2_SATA_TF_CMD_OFFSET		= 0x00,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b450a26..42ab418 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -207,7 +207,7 @@ enum {
 	 */
 	ATA_FLAG_DISABLED	= (1 << 23), /* port is disabled, ignore it */
 
-	/* bits 24:31 of ap->flags are reserved for LLD specific flags */
+	/* bits 29:31 of ap->flags are reserved for LLD specific flags */
 
 	/* struct ata_port pflags */
 	ATA_PFLAG_EH_PENDING	= (1 << 0), /* EH pending */

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Alan Cox]

> But.. these do look rather suspiciously like sysfs attrs to me.

I'd rather any future sysfs interface actually exposed something more
meaningful. This is a legacy fixup 8)

> 
> We didn't have a sysfs in the IDE days (so Gadi added the klunky
> procfs "settings" thing for there), but we do now.
> 
> I'm happy to update hdparm to check sysfs if that's where this
> ends up, or to do almost nothing for it if we stick with the ioctls.

I'm not sure sysfs helps much anyway - you have to open the device file
and keep it open while accessing the sysfs nodes anyway (something huge
numbers of apps hopelessly fail to do so)

If you don't you have no guarantee the pci device won't be
removed/replaced and you code will go off and adjust some other
unfortunate bit of hardware.

Alan

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Alan Cox]

> With the attached patch, you can use ->flags for the two new flags, and 
> you can set those bits in the driver where you set the other 
> ATA_FLAG_xxx bits.

Its half way but I realised there is another reason that doesn't work
(and indeed why the device private flags are asking for trouble)

What are the locking sematics for ap->flags ?

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Jeff Garzik]

Mark Lord wrote:
> Alan Cox wrote:
>> Something like this then ?
>>
>>
>> The legacy old IDE API for this is a bit primitive so we try and map 
>> stuff
>> sensibly onto it.
>>
>> - Add flags2 as we ran out of flags
>> - Set PIO over DMA devices to report 32bit
>> - Add ability to change the PIO32 settings if the controller permits it
>> - Add that functionality into the sff drivers
>> - Add that functionality into the VLB legacy driver
>> - Turn on the 32bit PIO on the ninja32 and add support there
>>
>> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ..
> 
> By all means go ahead with something like this,
> and thanks for doing it, Alan.
> 
> But.. these do look rather suspiciously like sysfs attrs to me.
> 
> We didn't have a sysfs in the IDE days (so Gadi added the klunky
> procfs "settings" thing for there), but we do now.
> 
> I'm happy to update hdparm to check sysfs if that's where this
> ends up, or to do almost nothing for it if we stick with the ioctls.

hmmm, Tejun's storage summit presentation would be quite relevant at 
this juncture :)

The short answer:  we need a control mechanism for tasks such SATA PHY 
twiddling, host controller reset, host controller configuration 
(set/clear 32-bit I/O in this example), ...

sysfs attrs are desired for this, but we've held off largely because 
this is all intertwined with the long term direction of "storage model", 
"transport protocol" and libata's eventual move away from strict SCSI 
dependency.

I think the best place for sysfs attr attachment is an ata_transport 
module, something that's needed to be written for quite a while.  You 
can see some of the eventual direction in scsi_transport_*.[ch] and 
raid_class.c.

Eventually I hope to reach a point where the current crop of SATA+SAS 
controllers out in the field (e.g. mvsas) will attach to 
scsi_transport_sas or ata_transport, depending on controller port 
configuration and attached device(s).  And for the sake of users' 
sanity, a single utility should be used to configure a SATA+SAS 
controller port.  Requiring use of hdparm when the port is in SATA mode, 
and sg_* when the port is in SAS mode, is a bit annoying.

	Jeff

[PROPOSED] ata: Report 16/32bit PIO as best we can

[Alan Cox]

Something like this then ?


The legacy old IDE API for this is a bit primitive so we try and map stuff
sensibly onto it.

- Add flags2 as we ran out of flags
- Set PIO over DMA devices to report 32bit
- Add ability to change the PIO32 settings if the controller permits it
- Add that functionality into the sff drivers
- Add that functionality into the VLB legacy driver
- Turn on the 32bit PIO on the ninja32 and add support there

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/ata/libata-scsi.c  |   30 ++++++++++++++++++++++++++----
 drivers/ata/libata-sff.c   |   27 +++++++++++++++++++++++++++
 drivers/ata/pata_legacy.c  |   33 ++++++++++++++++++++-------------
 drivers/ata/pata_ninja32.c |    2 ++
 include/linux/libata.h     |    6 ++++++
 5 files changed, 81 insertions(+), 17 deletions(-)


diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b9747fa..3f19423 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -647,23 +647,45 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 	return rc;
 }
 
+static int ata_ioc32(struct ata_port *ap)
+{
+	if (ap->flags & ATA_FLAG_PIO_DMA)
+		return 1;
+	if (ap->flags2 & ATA_FLAG2_PIO32)
+		return 1;
+	return 0;
+}
+
 int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *scsidev,
 		     int cmd, void __user *arg)
 {
 	int val = -EINVAL, rc = -EINVAL;
+	unsigned long flags;
 
 	switch (cmd) {
 	case ATA_IOC_GET_IO32:
-		val = 0;
+		spin_lock_irqsave(ap->lock, flags);
+		val = ata_ioc32(ap);
+		spin_unlock_irqrestore(ap->lock, flags);
 		if (copy_to_user(arg, &val, 1))
 			return -EFAULT;
 		return 0;
 
 	case ATA_IOC_SET_IO32:
 		val = (unsigned long) arg;
-		if (val != 0)
-			return -EINVAL;
-		return 0;
+		rc = 0;
+		spin_lock_irqsave(ap->lock, flags);
+		if (ap->flags2 & ATA_FLAG2_PIO32CHANGE) {
+			if (val)
+				ap->flags2 |= ATA_FLAG2_PIO32;
+			else
+				ap->flags2 &= ~ATA_FLAG2_PIO32;
+		} else {
+			if (val != ata_ioc32(ap))
+				rc = -EINVAL;
+		}
+		spin_unlock_irqrestore(ap->lock, flags);
+		return rc;
 
 	case HDIO_GET_IDENTITY:
 		return ata_get_identity(ap, scsidev, arg);
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 8332e97..847eacc 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -87,6 +87,7 @@ const struct ata_port_operations ata_bmdma32_port_ops = {
 	.inherits		= &ata_bmdma_port_ops,
 
 	.sff_data_xfer		= ata_sff_data_xfer32,
+	.port_start		= ata_sff_port_start32,
 };
 EXPORT_SYMBOL_GPL(ata_bmdma32_port_ops);
 
@@ -769,6 +770,9 @@ unsigned int ata_sff_data_xfer32(struct ata_device *dev, unsigned char *buf,
 	void __iomem *data_addr = ap->ioaddr.data_addr;
 	unsigned int words = buflen >> 2;
 	int slop = buflen & 3;
+	
+	if (!(ap->flags2 & ATA_FLAG2_PIO32))
+		return ata_sff_data_xfer(dev, buf, buflen, rw);
 
 	/* Transfer multiple of 4 bytes */
 	if (rw == READ)
@@ -2402,6 +2406,29 @@ int ata_sff_port_start(struct ata_port *ap)
 EXPORT_SYMBOL_GPL(ata_sff_port_start);
 
 /**
+ *	ata_sff_port_start32 - Set port up for dma.
+ *	@ap: Port to initialize
+ *
+ *	Called just after data structures for each port are
+ *	initialized.  Allocates space for PRD table if the device
+ *	is DMA capable SFF.
+ *
+ *	May be used as the port_start() entry in ata_port_operations for
+ *	devices that are capable of 32bit PIO.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+int ata_sff_port_start32(struct ata_port *ap)
+{
+	ap->flags2 |= ATA_FLAG2_PIO32 | ATA_FLAG2_PIO32CHANGE;
+	if (ap->ioaddr.bmdma_addr)
+		return ata_port_start(ap);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ata_sff_port_start32);
+
+/**
  *	ata_sff_std_ports - initialize ioaddr with standard port offsets.
  *	@ioaddr: IO address structure to be initialized
  *
diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
index 3f830f0..fc40ba3 100644
--- a/drivers/ata/pata_legacy.c
+++ b/drivers/ata/pata_legacy.c
@@ -108,6 +108,7 @@ struct legacy_controller {
 	struct ata_port_operations *ops;
 	unsigned int pio_mask;
 	unsigned int flags;
+	unsigned int flags2;
 	int (*setup)(struct platform_device *, struct legacy_probe *probe,
 		struct legacy_data *data);
 };
@@ -285,7 +286,8 @@ static unsigned int pdc_data_xfer_vlb(struct ata_device *dev,
 {
 	int slop = buflen & 3;
 	/* 32bit I/O capable *and* we need to write a whole number of dwords */
-	if (ata_id_has_dword_io(dev->id) && (slop == 0 || slop == 3)) {
+	if (ata_id_has_dword_io(dev->id) && (slop == 0 || slop == 3)
+					&& (ap->flags2 & ATA_FLAG2_PIO32)) {
 		struct ata_port *ap = dev->link->ap;
 		unsigned long flags;
 
@@ -736,7 +738,8 @@ static unsigned int vlb32_data_xfer(struct ata_device *adev, unsigned char *buf,
 	struct ata_port *ap = adev->link->ap;
 	int slop = buflen & 3;
 
-	if (ata_id_has_dword_io(adev->id) && (slop == 0 || slop == 3)) {
+	if (ata_id_has_dword_io(adev->id) && (slop == 0 || slop == 3)
+					&& (ap->flags2 & ATA_FLAG2_PIO32)) {
 		if (rw == WRITE)
 			iowrite32_rep(ap->ioaddr.data_addr, buf, buflen >> 2);
 		else
@@ -858,27 +861,30 @@ static struct ata_port_operations winbond_port_ops = {
 
 static struct legacy_controller controllers[] = {
 	{"BIOS",	&legacy_port_ops, 	0x1F,
-						ATA_FLAG_NO_IORDY,	NULL },
+			ATA_FLAG_NO_IORDY,	0,			NULL },
 	{"Snooping", 	&simple_port_ops, 	0x1F,
-						0	       ,	NULL },
+			0,			0,			NULL },
 	{"PDC20230",	&pdc20230_port_ops,	0x7,
-						ATA_FLAG_NO_IORDY,	NULL },
+			ATA_FLAG_NO_IORDY,
+			ATA_FLAG2_PIO32 | ATA_FLAG2_PIO32_CHANGE,	NULL },
 	{"HT6560A",	&ht6560a_port_ops,	0x07,
-						ATA_FLAG_NO_IORDY,	NULL },
+			ATA_FLAG_NO_IORDY,	0,			NULL },
 	{"HT6560B",	&ht6560b_port_ops,	0x1F,
-						ATA_FLAG_NO_IORDY,	NULL },
+			ATA_FLAG_NO_IORDY,	0,			NULL },
 	{"OPTI82C611A",	&opti82c611a_port_ops,	0x0F,
-						0	       ,	NULL },
+			0,			0,			NULL },
 	{"OPTI82C46X",	&opti82c46x_port_ops,	0x0F,
-						0	       ,	NULL },
+			0,			0,			NULL },
 	{"QDI6500",	&qdi6500_port_ops,	0x07,
-					ATA_FLAG_NO_IORDY,	qdi_port },
+			ATA_FLAG_NO_IORDY,
+			ATA_FLAG2_PIO32 | ATA_FLAG2_PIO32_CHANGE,    qdi_port },
 	{"QDI6580",	&qdi6580_port_ops,	0x1F,
-					0	       ,	qdi_port },
+			0, ATA_FLAG2_PIO32 | ATA_FLAG2_PIO32_CHANGE, qdi_port },
 	{"QDI6580DP",	&qdi6580dp_port_ops,	0x1F,
-					0	       ,	qdi_port },
+			0, ATA_FLAG2_PIO32 | ATA_FLAG2_PIO32_CHANGE, qdi_port },
 	{"W83759A",	&winbond_port_ops,	0x1F,
-					0	       ,	winbond_port }
+			0, ATA_FLAG2_PIO32 | ATA_FLAG2_PIO32_CHANGE,
+								winbond_port }
 };
 
 /**
@@ -1008,6 +1014,7 @@ static __init int legacy_init_one(struct legacy_probe *probe)
 	ap->ops = ops;
 	ap->pio_mask = pio_modes;
 	ap->flags |= ATA_FLAG_SLAVE_POSS | iordy;
+	ap->flags2 |= controller->flags2;
 	ap->ioaddr.cmd_addr = io_addr;
 	ap->ioaddr.altstatus_addr = ctrl_addr;
 	ap->ioaddr.ctl_addr = ctrl_addr;
diff --git a/drivers/ata/pata_ninja32.c b/drivers/ata/pata_ninja32.c
index 0fb6b1b..f8c5043 100644
--- a/drivers/ata/pata_ninja32.c
+++ b/drivers/ata/pata_ninja32.c
@@ -86,6 +86,7 @@ static struct ata_port_operations ninja32_port_ops = {
 	.sff_dev_select = ninja32_dev_select,
 	.cable_detect	= ata_cable_40wire,
 	.set_piomode	= ninja32_set_piomode,
+	.data_xfer	= ata_aff_data_xfer_32
 };
 
 static void ninja32_program(void __iomem *base)
@@ -144,6 +145,7 @@ static int ninja32_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 	ap->ioaddr.altstatus_addr = base + 0x1E;
 	ap->ioaddr.bmdma_addr = base;
 	ata_sff_std_ports(&ap->ioaddr);
+	ap->flags2 = ATA_FLAG2_PIO32 | ATA_FLAG2_PIO32_CHANGE;
 
 	ninja32_program(base);
 	/* FIXME: Should we disable them at remove ? */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b450a26..bb82835 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -209,6 +209,10 @@ enum {
 
 	/* bits 24:31 of ap->flags are reserved for LLD specific flags */
 
+	/* Overflow flags in ap->flags2 */	
+	ATA_FLAG2_PIO32		= (1 << 0),  /* 32bit PIO */
+	ATA_FLAG2_PIO32CHANGE	= (1 << 1),  /* 32bit PIO can be turned on/off */
+
 	/* struct ata_port pflags */
 	ATA_PFLAG_EH_PENDING	= (1 << 0), /* EH pending */
 	ATA_PFLAG_EH_IN_PROGRESS = (1 << 1), /* EH in progress */
@@ -690,6 +694,7 @@ struct ata_port {
 	struct ata_port_operations *ops;
 	spinlock_t		*lock;
 	unsigned long		flags;	/* ATA_FLAG_xxx */
+	unsigned int		flags2;	/* ATA_FLAG2_xxx */
 	unsigned int		pflags; /* ATA_PFLAG_xxx */
 	unsigned int		print_id; /* user visible unique port ID */
 	unsigned int		port_no; /* 0 based port no. inside the host */
@@ -1595,6 +1600,7 @@ extern void ata_sff_drain_fifo(struct ata_queued_cmd *qc);
 extern void ata_sff_error_handler(struct ata_port *ap);
 extern void ata_sff_post_internal_cmd(struct ata_queued_cmd *qc);
 extern int ata_sff_port_start(struct ata_port *ap);
+extern int ata_sff_port_start32(struct ata_port *ap);
 extern void ata_sff_std_ports(struct ata_ioports *ioaddr);
 extern unsigned long ata_bmdma_mode_filter(struct ata_device *dev,
 					   unsigned long xfer_mask);

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Jeff Garzik]

Alan Cox wrote:
>> With the attached patch, you can use ->flags for the two new flags, and 
>> you can set those bits in the driver where you set the other 
>> ATA_FLAG_xxx bits.
> 
> Its half way but I realised there is another reason that doesn't work
> (and indeed why the device private flags are asking for trouble)
> 
> What are the locking sematics for ap->flags ?

In general, for both driver-private flags and ATA_FLAG_xxx, the flags 
should be set at compile or pci_driver::probe() time, and never touched 
after that.

So your patch is a bit of abuse, really, by fiddling with ap->flags at 
all ;-)  The drivers, for example, never touch the driver-private flags 
after they are set at init.  The core will occasionally twiddle flags, 
inside of spin_lock_irqsave(ap->lock)

Since they are legacy ioctls, I think that reflecting current reality 
should be sufficient.  That is what the current set does:

	* GET: return "16 bit"
	* SET: if "16 bit" return success else fail

Thus 'set' has always been an illusion for libata.  It's really been 
read-only.

	Jeff

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Alan Cox]

> > What are the locking sematics for ap->flags ?
> 
> In general, for both driver-private flags and ATA_FLAG_xxx, the flags 
> should be set at compile or pci_driver::probe() time, and never touched 
> after that.

Ok we should document that.

> 
> So your patch is a bit of abuse, really, by fiddling with ap->flags at 
> all ;-)  The drivers, for example, never touch the driver-private flags 
> after they are set at init.  The core will occasionally twiddle flags, 
> inside of spin_lock_irqsave(ap->lock)

Which is the lock I am using

> Thus 'set' has always been an illusion for libata.  It's really been 
> read-only.

Which nicely solves the flags2 problem - we have flags, and we have
dynamic flags and if we move the other changing flags into dynflags over
time all will be happy.

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Tejun Heo]

Alan Cox wrote:
>> With the attached patch, you can use ->flags for the two new flags, and 
>> you can set those bits in the driver where you set the other 
>> ATA_FLAG_xxx bits.
> 
> Its half way but I realised there is another reason that doesn't work
> (and indeed why the device private flags are asking for trouble)
> 
> What are the locking sematics for ap->flags ?

ap->flags is owned by EH context and only EH can change it.
ap->pflags is for dynamic flags and can be changed under port lock by
anyone but I forgot why I named that pflags instead of dflags.

Thanks.

-- 
tejun

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Jeff Garzik]

Alan Cox wrote:
> Which nicely solves the flags2 problem - we have flags, and we have
> dynamic flags and if we move the other changing flags into dynflags over
> time all will be happy.

Such as ->pflags and ATA_PFLAG_xxx perhaps?  :)

	Jeff

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Tejun Heo]

Alan Cox wrote:
>> Thus 'set' has always been an illusion for libata.  It's really been 
>> read-only.
> 
> Which nicely solves the flags2 problem - we have flags, and we have
> dynamic flags and if we move the other changing flags into dynflags over
> time all will be happy.

Umm... there's already ap->pflags for the purpose and it was splitted
out for exactly the same reason.  If we're running out of flags space,
I think we better switch to bitfields than introducing flagsN and
risking testing/setting the wrong flag to the wrong field.

Thanks.

-- 
tejun

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Tejun Heo]

Tejun Heo wrote:
> Alan Cox wrote:
>>> Thus 'set' has always been an illusion for libata.  It's really been 
>>> read-only.
>> Which nicely solves the flags2 problem - we have flags, and we have
>> dynamic flags and if we move the other changing flags into dynflags over
>> time all will be happy.
> 
> Umm... there's already ap->pflags for the purpose and it was splitted
> out for exactly the same reason.  If we're running out of flags space,
> I think we better switch to bitfields than introducing flagsN and
                                 ^^^^^
				 map
Sorry, sleep deprived.  :-)

-- 
tejun

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Alan Cox]

> ap->flags is owned by EH context and only EH can change it.
> ap->pflags is for dynamic flags and can be changed under port lock by
> anyone but I forgot why I named that pflags instead of dflags.

Ok added comments to that effect to libata.h (I guess you used pflag not
dflag as ATA_DFLAG_xx is already used ?) and switched to using pflags.

Alan

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Tejun Heo]

Hello,

Alan Cox wrote:
>> ap->flags is owned by EH context and only EH can change it.
>> ap->pflags is for dynamic flags and can be changed under port lock by
>> anyone but I forgot why I named that pflags instead of dflags.
> 
> Ok added comments to that effect to libata.h

Great.

> (I guess you used pflag not dflag as ATA_DFLAG_xx is already used ?)
> and switched to using pflags.

Yeah, right, that was it.  Thanks. :-)

-- 
tejun

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Mark Lord]

Jeff Garzik wrote:
> a single utility should be used to configure a SATA+SAS 
> controller port.  Requiring use of hdparm when the port is in SATA mode, 
> and sg_* when the port is in SAS mode, is a bit annoying.
..

That's why hdparm uses sg_* whenever possible already.

-ml

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> a single utility should be used to configure a SATA+SAS controller 
>> port.  Requiring use of hdparm when the port is in SATA mode, and sg_* 
>> when the port is in SAS mode, is a bit annoying.
> ..
> 
> That's why hdparm uses sg_* whenever possible already.

Does hdparm really call out to external applications sg_senddiag, 
sg_opcodes, sg_inq, sg_turs, etc.?

	Jeff

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Douglas Gilbert]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Alan Cox wrote:
>>> Something like this then ?
>>>
>>>
>>> The legacy old IDE API for this is a bit primitive so we try and map 
>>> stuff
>>> sensibly onto it.
>>>
>>> - Add flags2 as we ran out of flags
>>> - Set PIO over DMA devices to report 32bit
>>> - Add ability to change the PIO32 settings if the controller permits it
>>> - Add that functionality into the sff drivers
>>> - Add that functionality into the VLB legacy driver
>>> - Turn on the 32bit PIO on the ninja32 and add support there
>>>
>>> Signed-off-by: Alan Cox <alan@linux.intel.com>
>> ..
>>
>> By all means go ahead with something like this,
>> and thanks for doing it, Alan.
>>
>> But.. these do look rather suspiciously like sysfs attrs to me.
>>
>> We didn't have a sysfs in the IDE days (so Gadi added the klunky
>> procfs "settings" thing for there), but we do now.
>>
>> I'm happy to update hdparm to check sysfs if that's where this
>> ends up, or to do almost nothing for it if we stick with the ioctls.
> 
> hmmm, Tejun's storage summit presentation would be quite relevant at 
> this juncture :)
> 
> The short answer:  we need a control mechanism for tasks such SATA PHY 
> twiddling, host controller reset, host controller configuration 
> (set/clear 32-bit I/O in this example), ...
> 
> sysfs attrs are desired for this, but we've held off largely because 
> this is all intertwined with the long term direction of "storage model", 
> "transport protocol" and libata's eventual move away from strict SCSI 
> dependency.
> 
> I think the best place for sysfs attr attachment is an ata_transport 
> module, something that's needed to be written for quite a while.  You 
> can see some of the eventual direction in scsi_transport_*.[ch] and 
> raid_class.c.
> 
> Eventually I hope to reach a point where the current crop of SATA+SAS 
> controllers out in the field (e.g. mvsas) will attach to 
> scsi_transport_sas or ata_transport, depending on controller port 
> configuration and attached device(s).  And for the sake of users' 
> sanity, a single utility should be used to configure a SATA+SAS 
> controller port.  Requiring use of hdparm when the port is in SATA mode, 
> and sg_* when the port is in SAS mode, is a bit annoying.

The original question seemed to be about PATA. sysfs
attributes would be one solution. Another solution is
for libata to implement the SAT (ref: sat2r06.pdf at
www.t10.org) specific mode page called "PATA Control"
(see section 12.3.2). The latter solution may well work
with FCoE, iSCSI, FC, USB, 1394 and SAS. Currently there
is no SATA equivalent to that mode page.
In the PATA case surprise me and choose the better solution.

Getting to the host phy directly connected to a SATA disk
is a particularly thorny problem and a no go area for
sysfs in all but the direct connect case. For example
a SATA "host" phy connected to a SATA disk attached to
a SAS expander can be controlled via the SAS SMP protocol.

hdparm assumes the device it is talking to is a PATA or
SATA disk (with some support for the ATA part of an
ATAPI device). hdparm is now capable of talking to
such devices irrespective of whether the OS sees a
ATA transport (PATA or SATA) or one of those other (evil)
SCSI friendly transports (same list as above). My
sg3_utils are lower level utilities that mostly map
directly to SCSI or ATA commands. Feel free to write
a utility that unifies the attributes of
PATA/SATA/FC/SCSI/SAS/logical disks and OSD devices :-)

As for the freestanding ata_transport idea which transport
would a mvsas host adapter belong to which has one phy
connected to a SATA disk, another connected to a SAS disk and
a third one connected to a SAS expander with lots of SATA
disks the other side of that?

Doug Gilbert

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Jeff Garzik]

Douglas Gilbert wrote:
> As for the freestanding ata_transport idea which transport
> would a mvsas host adapter belong to which has one phy
> connected to a SATA disk, another connected to a SAS disk and
> a third one connected to a SAS expander with lots of SATA
> disks the other side of that?

Only the direct attach example would use ata_transport.

	Jeff

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Jeff Garzik]

Alan Cox wrote:
>> But.. these do look rather suspiciously like sysfs attrs to me.
> 
> I'd rather any future sysfs interface actually exposed something more
> meaningful. This is a legacy fixup 8)
> 
>> We didn't have a sysfs in the IDE days (so Gadi added the klunky
>> procfs "settings" thing for there), but we do now.
>>
>> I'm happy to update hdparm to check sysfs if that's where this
>> ends up, or to do almost nothing for it if we stick with the ioctls.
> 
> I'm not sure sysfs helps much anyway - you have to open the device file
> and keep it open while accessing the sysfs nodes anyway (something huge
> numbers of apps hopelessly fail to do so)

FWIW...  here is the sysfs work I referred to (in a message sent several 
days ago in this thread)...

http://lwn.net/Articles/294608/

	Jeff

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Alan Cox]

On Mon, 13 Apr 2009 12:32:57 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> > I'm not sure sysfs helps much anyway - you have to open the device file
> > and keep it open while accessing the sysfs nodes anyway (something huge
> > numbers of apps hopelessly fail to do so)  
> 
> FWIW...  here is the sysfs work I referred to (in a message sent several 
> days ago in this thread)...
> 
> http://lwn.net/Articles/294608/

Which indeed shows the same problems. There is nothing to stop changes in
the rest of the topology from causing me to write to the sysfs at the
wrong moment and reconfigure/misconfigure a different object to the one
intended.

Alan

Re: [PROPOSED] ata: Report 16/32bit PIO as best we can

[Jeff Garzik]

Alan Cox wrote:
> On Mon, 13 Apr 2009 12:32:57 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>>> I'm not sure sysfs helps much anyway - you have to open the device file
>>> and keep it open while accessing the sysfs nodes anyway (something huge
>>> numbers of apps hopelessly fail to do so)  
>> FWIW...  here is the sysfs work I referred to (in a message sent several 
>> days ago in this thread)...
>>
>> http://lwn.net/Articles/294608/
> 
> Which indeed shows the same problems. There is nothing to stop changes in
> the rest of the topology from causing me to write to the sysfs at the
> wrong moment and reconfigure/misconfigure a different object to the one
> intended.

The horse has already left the barn, on that one...

Google's ata transport class is consistent with existing transport class 
work in the kernel.  It is also consistent with recent admonitions in 
the osdblk thread, regarding the "one piece of data per sysfs file" rule.

Personally I think a netlink-like approach to managing and controlling 
SAS and ATA would be better, but that's not what gets merged...

	Jeff