[PATCHSET] libata: implement per-dev xfer masks

[PATCHSET] libata: implement per-dev xfer masks

[Tejun Heo]

Hello, all.

This patchset adds per-dev pio, mwdma and udma_mask.  Because
port-wide xfer_mask is currently used, this patch does not change any
behavior.  Per-dev xfer masks will also be used by EH to slow down
devices suffering frequent transmission errors.

I marked with comment where the mode filtering for low level drivers
should be done but I'm not sure how the interface should look like.
->pre_set_mode(ap) is okay but then the LLDD should iterate over all
devices.  Maybe ->pre_set_mode(ap, dev) or ->filter_xfer_mask(ap, dev)?

The patchset is against the current #upstream[1].

Thanks.

--
tejun

[1] 418dc1f5a805822fcf1118804ddc689a4156db4a

[PATCH 2/4] libata: implement ata_dev_init()

[Tejun Heo]

Implement ata_dev_init() which is called before probing for new
device.  Note that ata_dev_init() is called only once for each
attached device while ata_dev_configure() can be called multiple times
for the same device.

This change will be used by following per-dev xfer_mask patch.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   33 +++++++++++++++++++++++++--------
 1 files changed, 25 insertions(+), 8 deletions(-)

85ebfaba5b9f0c02afa9a3a2bde8eb6bf14380d4
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 96954c7..eef67f5 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1042,6 +1042,25 @@ unsigned int ata_pio_need_iordy(const st
 }
 
 /**
+ *	ata_dev_init - initialize ata_device structure
+ *	@ap: Port on which target device resides
+ *	@dev: Target device to initialize
+ *
+ *	Initialize @dev in preparation for device probing.  This
+ *	function is called only when a new device is about to be
+ *	probed.  ie. ata_dev_configure() is responsible for cleaning
+ *	up affected fields across revalidations.
+ *
+ *	LOCKING:
+ *	none.
+ */
+static void ata_dev_init(struct ata_port *ap, struct ata_device *dev)
+{
+	memset(dev, 0, sizeof(*dev));
+	dev->devno = dev - ap->device;
+}
+
+/**
  *	ata_dev_read_id - Read ID data from the specified device
  *	@ap: port on which target device resides
  *	@dev: target device
@@ -1367,11 +1386,14 @@ static int ata_bus_probe(struct ata_port
 
 	ata_port_probe(ap);
 
+	/* initialize devices and classes */
+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
+		ata_dev_init(ap, &ap->device[i]);
+		classes[i] = ATA_DEV_UNKNOWN;
+	}
+
 	/* reset */
 	if (ap->ops->probe_reset) {
-		for (i = 0; i < ATA_MAX_DEVICES; i++)
-			classes[i] = ATA_DEV_UNKNOWN;
-
 		rc = ap->ops->probe_reset(ap, classes);
 		if (rc) {
 			printk("ata%u: reset failed (errno=%d)\n", ap->id, rc);
@@ -4606,8 +4628,6 @@ static void ata_host_init(struct ata_por
 			  struct ata_host_set *host_set,
 			  const struct ata_probe_ent *ent, unsigned int port_no)
 {
-	unsigned int i;
-
 	host->max_id = 16;
 	host->max_lun = 1;
 	host->max_channel = 1;
@@ -4634,9 +4654,6 @@ static void ata_host_init(struct ata_por
 	INIT_WORK(&ap->port_task, NULL, NULL);
 	INIT_LIST_HEAD(&ap->eh_done_q);
 
-	for (i = 0; i < ATA_MAX_DEVICES; i++)
-		ap->device[i].devno = i;
-
 #ifdef ATA_IRQ_TRAP
 	ap->stats.unhandled_irq = 1;
 	ap->stats.idle_irq = 1;
-- 
1.2.4

[PATCH 2/4] libata: implement ata_dev_init()

[Tejun Heo]

Implement ata_dev_init() which is called before probing for new
device.  Note that ata_dev_init() is called only once for each
attached device while ata_dev_configure() can be called multiple times
for the same device.

This change will be used by following per-dev xfer_mask patch.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

fix-class-handling-in-ata_bus_probe[1] patch clashes with this one.
So, this is the regenerated patch.  All others apply fine with offets.

[1] http://article.gmane.org/gmane.linux.ide/8712

 libata-core.c |   28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2006-03-13 14:52:17.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2006-03-13 14:52:57.000000000 +0900
@@ -1045,6 +1045,25 @@ unsigned int ata_pio_need_iordy(const st
 }
 
 /**
+ *	ata_dev_init - initialize ata_device structure
+ *	@ap: Port on which target device resides
+ *	@dev: Target device to initialize
+ *
+ *	Initialize @dev in preparation for device probing.  This
+ *	function is called only when a new device is about to be
+ *	probed.  ie. ata_dev_configure() is responsible for cleaning
+ *	up affected fields across revalidations.
+ *
+ *	LOCKING:
+ *	none.
+ */
+static void ata_dev_init(struct ata_port *ap, struct ata_device *dev)
+{
+	memset(dev, 0, sizeof(*dev));
+	dev->devno = dev - ap->device;
+}
+
+/**
  *	ata_dev_read_id - Read ID data from the specified device
  *	@ap: port on which target device resides
  *	@dev: target device
@@ -1371,8 +1390,10 @@ static int ata_bus_probe(struct ata_port
 	ata_port_probe(ap);
 
 	/* reset and determine device classes */
-	for (i = 0; i < ATA_MAX_DEVICES; i++)
+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
+		ata_dev_init(ap, &ap->device[i]);
 		classes[i] = ATA_DEV_UNKNOWN;
+	}
 
 	if (ap->ops->probe_reset) {
 		rc = ap->ops->probe_reset(ap, classes);
@@ -4607,8 +4628,6 @@ static void ata_host_init(struct ata_por
 			  struct ata_host_set *host_set,
 			  const struct ata_probe_ent *ent, unsigned int port_no)
 {
-	unsigned int i;
-
 	host->max_id = 16;
 	host->max_lun = 1;
 	host->max_channel = 1;
@@ -4635,9 +4654,6 @@ static void ata_host_init(struct ata_por
 	INIT_WORK(&ap->port_task, NULL, NULL);
 	INIT_LIST_HEAD(&ap->eh_done_q);
 
-	for (i = 0; i < ATA_MAX_DEVICES; i++)
-		ap->device[i].devno = i;
-
 #ifdef ATA_IRQ_TRAP
 	ap->stats.unhandled_irq = 1;
 	ap->stats.idle_irq = 1;

Re: [PATCH 2/4] libata: implement ata_dev_init()

[Jeff Garzik]

Tejun Heo wrote:
> Implement ata_dev_init() which is called before probing for new
> device.  Note that ata_dev_init() is called only once for each
> attached device while ata_dev_configure() can be called multiple times
> for the same device.
> 
> This change will be used by following per-dev xfer_mask patch.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
> fix-class-handling-in-ata_bus_probe[1] patch clashes with this one.
> So, this is the regenerated patch.  All others apply fine with offets.
> 
> [1] http://article.gmane.org/gmane.linux.ide/8712
> 
>  libata-core.c |   28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> Index: work/drivers/scsi/libata-core.c
> ===================================================================
> --- work.orig/drivers/scsi/libata-core.c	2006-03-13 14:52:17.000000000 +0900
> +++ work/drivers/scsi/libata-core.c	2006-03-13 14:52:57.000000000 +0900
> @@ -1045,6 +1045,25 @@ unsigned int ata_pio_need_iordy(const st
>  }
>  
>  /**
> + *	ata_dev_init - initialize ata_device structure
> + *	@ap: Port on which target device resides
> + *	@dev: Target device to initialize
> + *
> + *	Initialize @dev in preparation for device probing.  This
> + *	function is called only when a new device is about to be
> + *	probed.  ie. ata_dev_configure() is responsible for cleaning
> + *	up affected fields across revalidations.
> + *
> + *	LOCKING:
> + *	none.
> + */
> +static void ata_dev_init(struct ata_port *ap, struct ata_device *dev)
> +{
> +	memset(dev, 0, sizeof(*dev));
> +	dev->devno = dev - ap->device;
> +}
> +
> +/**
>   *	ata_dev_read_id - Read ID data from the specified device
>   *	@ap: port on which target device resides
>   *	@dev: target device
> @@ -1371,8 +1390,10 @@ static int ata_bus_probe(struct ata_port
>  	ata_port_probe(ap);
>  
>  	/* reset and determine device classes */
> -	for (i = 0; i < ATA_MAX_DEVICES; i++)
> +	for (i = 0; i < ATA_MAX_DEVICES; i++) {
> +		ata_dev_init(ap, &ap->device[i]);
>  		classes[i] = ATA_DEV_UNKNOWN;
> +	}
>  
>  	if (ap->ops->probe_reset) {
>  		rc = ap->ops->probe_reset(ap, classes);
> @@ -4607,8 +4628,6 @@ static void ata_host_init(struct ata_por
>  			  struct ata_host_set *host_set,
>  			  const struct ata_probe_ent *ent, unsigned int port_no)
>  {
> -	unsigned int i;
> -
>  	host->max_id = 16;
>  	host->max_lun = 1;
>  	host->max_channel = 1;
> @@ -4635,9 +4654,6 @@ static void ata_host_init(struct ata_por
>  	INIT_WORK(&ap->port_task, NULL, NULL);
>  	INIT_LIST_HEAD(&ap->eh_done_q);
>  
> -	for (i = 0; i < ATA_MAX_DEVICES; i++)
> -		ap->device[i].devno = i;

NAK:

1) the devices are embedded with ata_port, and so all static 
initialization should be done in ata_host_init().  It's only called once 
for each device, as you say, anyway...  no need to separate it from the 
rest of the struct ata_port initialization AFAICS.

2) there is no need to memset() [unless you are re-initializing, I suppose]

Re: [PATCH 2/4] libata: implement ata_dev_init()

[Tejun Heo]

On Mon, Mar 13, 2006 at 03:26:37AM -0500, Jeff Garzik wrote:
> NAK:
> 
> 1) the devices are embedded with ata_port, and so all static 
> initialization should be done in ata_host_init().  It's only called once 
> for each device, as you say, anyway...  no need to separate it from the 
> rest of the struct ata_port initialization AFAICS.

I was mainly thinking of hotplug cases.  dev liftime becomes different
form ap lifetime in hotplug cases, so it needs reinitialization before
recycling.  I guess it can wait till we actually do hotplug.  I'll
drop this patch.

> 2) there is no need to memset() [unless you are re-initializing, I suppose]

ditto.

-- 
tejun

[PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask

[Tejun Heo]

Add per-dev pio/mwdma/udma_mask.  All transfer mode limits used to be
applied to ap->*_mask which unnecessarily restricted other devices
sharing the port.  This change will also benefit later EH speed down
and hotplug.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   38 ++++++++++++++++++++++----------------
 include/linux/libata.h     |    5 +++++
 2 files changed, 27 insertions(+), 16 deletions(-)

ff1f6ace25a33e2338f7dc18a15951e7aa22c14f
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index eef67f5..beedc4b 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -65,8 +65,7 @@ static unsigned int ata_dev_init_params(
 					struct ata_device *dev);
 static void ata_set_mode(struct ata_port *ap);
 static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev);
-static unsigned int ata_dev_xfermask(struct ata_port *ap,
-				     struct ata_device *dev);
+static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev);
 
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
@@ -1058,6 +1057,10 @@ static void ata_dev_init(struct ata_port
 {
 	memset(dev, 0, sizeof(*dev));
 	dev->devno = dev - ap->device;
+
+	dev->pio_mask = UINT_MAX;
+	dev->mwdma_mask = UINT_MAX;
+	dev->udma_mask = UINT_MAX;
 }
 
 /**
@@ -1825,16 +1828,19 @@ static void ata_set_mode(struct ata_port
 	/* step 1: calculate xfer_mask */
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
-		unsigned int xfer_mask;
+		unsigned int pio_mask, dma_mask;
 
 		if (!ata_dev_present(dev))
 			continue;
 
-		xfer_mask = ata_dev_xfermask(ap, dev);
+		ata_dev_xfermask(ap, dev);
+
+		/* TODO: let LLDD filter dev->*_mask here */
 
-		dev->pio_mode = ata_xfer_mask2mode(xfer_mask & ATA_MASK_PIO);
-		dev->dma_mode = ata_xfer_mask2mode(xfer_mask & (ATA_MASK_MWDMA |
-								ATA_MASK_UDMA));
+		pio_mask = ata_pack_xfermask(dev->pio_mask, 0, 0);
+		dma_mask = ata_pack_xfermask(0, dev->mwdma_mask, dev->udma_mask);
+		dev->pio_mode = ata_xfer_mask2mode(pio_mask);
+		dev->dma_mode = ata_xfer_mask2mode(dma_mask);
 	}
 
 	/* step 2: always set host PIO timings */
@@ -2643,18 +2649,15 @@ static int ata_dma_blacklisted(const str
  *	@ap: Port on which the device to compute xfermask for resides
  *	@dev: Device to compute xfermask for
  *
- *	Compute supported xfermask of @dev.  This function is
- *	responsible for applying all known limits including host
- *	controller limits, device blacklist, etc...
+ *	Compute supported xfermask of @dev and store it in
+ *	dev->*_mask.  This function is responsible for applying all
+ *	known limits including host controller limits, device
+ *	blacklist, etc...
  *
  *	LOCKING:
  *	None.
- *
- *	RETURNS:
- *	Computed xfermask.
  */
-static unsigned int ata_dev_xfermask(struct ata_port *ap,
-				     struct ata_device *dev)
+static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev)
 {
 	unsigned long xfer_mask;
 	int i;
@@ -2667,6 +2670,8 @@ static unsigned int ata_dev_xfermask(str
 		struct ata_device *d = &ap->device[i];
 		if (!ata_dev_present(d))
 			continue;
+		xfer_mask &= ata_pack_xfermask(d->pio_mask, d->mwdma_mask,
+					       d->udma_mask);
 		xfer_mask &= ata_id_xfermask(d->id);
 		if (ata_dma_blacklisted(d))
 			xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
@@ -2676,7 +2681,8 @@ static unsigned int ata_dev_xfermask(str
 		printk(KERN_WARNING "ata%u: dev %u is on DMA blacklist, "
 		       "disabling DMA\n", ap->id, dev->devno);
 
-	return xfer_mask;
+	ata_unpack_xfermask(xfer_mask, &dev->pio_mask, &dev->mwdma_mask,
+			    &dev->udma_mask);
 }
 
 /**
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 239408e..2c624af 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -358,6 +358,11 @@ struct ata_device {
 	unsigned int		max_sectors;	/* per-device max sectors */
 	unsigned int		cdb_len;
 
+	/* per-dev xfer mask */
+	unsigned int		pio_mask;
+	unsigned int		mwdma_mask;
+	unsigned int		udma_mask;
+
 	/* for CHS addressing */
 	u16			cylinders;	/* Number of cylinders */
 	u16			heads;		/* Number of heads */
-- 
1.2.4

Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask

[Jeff Garzik]

Tejun Heo wrote:
> Add per-dev pio/mwdma/udma_mask.  All transfer mode limits used to be
> applied to ap->*_mask which unnecessarily restricted other devices
> sharing the port.  This change will also benefit later EH speed down
> and hotplug.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

I don't see much value in the separation.  Rather than 3 separate masks, 
it seems like this patch would be simplified if you simply added 
dev->xfer_mask.

	Jeff

Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask

[Tejun Heo]

On Mon, Mar 13, 2006 at 03:29:24AM -0500, Jeff Garzik wrote:
> Tejun Heo wrote:
> >Add per-dev pio/mwdma/udma_mask.  All transfer mode limits used to be
> >applied to ap->*_mask which unnecessarily restricted other devices
> >sharing the port.  This change will also benefit later EH speed down
> >and hotplug.
> >
> >Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> I don't see much value in the separation.  Rather than 3 separate masks, 
> it seems like this patch would be simplified if you simply added 
> dev->xfer_mask.
> 

The thing is that ap->*_mask's are separated the same way and all
masking constants are defined as such.  e.g.

	ap->udma_mask &= ATA_UDMA5;
or
	ap->udma_mask &= ATA_UDMA_MASK_40C;

Making dev->*_mask's the same enables share those constants and code
convention.  So, things to consider here are...

1. Port xfer masks are defined as three separate masks.

2. All the constants are defined according to that.

3. Three separate masks are easier to deal with for LLDD's.

I'm okay either way but using different approach for port and device
doesn't sound too hot.

-- 
tejun

Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask

[Jeff Garzik]

Tejun Heo wrote:
> On Mon, Mar 13, 2006 at 03:29:24AM -0500, Jeff Garzik wrote:
> 
>>Tejun Heo wrote:
>>
>>>Add per-dev pio/mwdma/udma_mask.  All transfer mode limits used to be
>>>applied to ap->*_mask which unnecessarily restricted other devices
>>>sharing the port.  This change will also benefit later EH speed down
>>>and hotplug.
>>>
>>>Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>>I don't see much value in the separation.  Rather than 3 separate masks, 
>>it seems like this patch would be simplified if you simply added 
>>dev->xfer_mask.
>>
> 
> 
> The thing is that ap->*_mask's are separated the same way and all
> masking constants are defined as such.  e.g.
> 
> 	ap->udma_mask &= ATA_UDMA5;
> or
> 	ap->udma_mask &= ATA_UDMA_MASK_40C;
> 
> Making dev->*_mask's the same enables share those constants and code
> convention.  So, things to consider here are...
> 
> 1. Port xfer masks are defined as three separate masks.
> 
> 2. All the constants are defined according to that.
> 
> 3. Three separate masks are easier to deal with for LLDD's.

Separate masks is better for the LLDD interface, but the packed version 
seems superior for internal libata use.

No reason why ATA_UDMA_MASK_40C can't simply operate on a packed 
xfer_mask variable, for example.

	Jeff

Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask

[Tejun Heo]

On Mon, Mar 13, 2006 at 04:52:28AM -0500, Jeff Garzik wrote:
> Tejun Heo wrote:
> >On Mon, Mar 13, 2006 at 03:29:24AM -0500, Jeff Garzik wrote:
> >
> >>Tejun Heo wrote:
> >>
> >>>Add per-dev pio/mwdma/udma_mask.  All transfer mode limits used to be
> >>>applied to ap->*_mask which unnecessarily restricted other devices
> >>>sharing the port.  This change will also benefit later EH speed down
> >>>and hotplug.
> >>>
> >>>Signed-off-by: Tejun Heo <htejun@gmail.com>
> >>
> >>I don't see much value in the separation.  Rather than 3 separate masks, 
> >>it seems like this patch would be simplified if you simply added 
> >>dev->xfer_mask.
> >>
> >
> >
> >The thing is that ap->*_mask's are separated the same way and all
> >masking constants are defined as such.  e.g.
> >
> >	ap->udma_mask &= ATA_UDMA5;
> >or
> >	ap->udma_mask &= ATA_UDMA_MASK_40C;
> >
> >Making dev->*_mask's the same enables share those constants and code
> >convention.  So, things to consider here are...
> >
> >1. Port xfer masks are defined as three separate masks.
> >
> >2. All the constants are defined according to that.
> >
> >3. Three separate masks are easier to deal with for LLDD's.
> 
> Separate masks is better for the LLDD interface, but the packed version 
> seems superior for internal libata use.

Yeah, dev->*_mask's will be used by LLDD's.  Actually, in patch #4 of
this series, sata_sil's ->dev_config() does exactly that.  Also, mode
mask filtering can be done by diddling dev->*_mask's.

> No reason why ATA_UDMA_MASK_40C can't simply operate on a packed 
> xfer_mask variable, for example.

Because it's used to filter ap->*_mask and to allow LLDD's use the
same constants on dev->*_mask's.

Do you think LLDD's shouldn't access dev->*_mask's?

-- 
tejun

Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask

[Jeff Garzik]

Tejun Heo wrote:
> On Mon, Mar 13, 2006 at 04:52:28AM -0500, Jeff Garzik wrote:
> 
>>Tejun Heo wrote:
>>
>>>On Mon, Mar 13, 2006 at 03:29:24AM -0500, Jeff Garzik wrote:
>>>
>>>
>>>>Tejun Heo wrote:
>>>>
>>>>
>>>>>Add per-dev pio/mwdma/udma_mask.  All transfer mode limits used to be
>>>>>applied to ap->*_mask which unnecessarily restricted other devices
>>>>>sharing the port.  This change will also benefit later EH speed down
>>>>>and hotplug.
>>>>>
>>>>>Signed-off-by: Tejun Heo <htejun@gmail.com>
>>>>
>>>>I don't see much value in the separation.  Rather than 3 separate masks, 
>>>>it seems like this patch would be simplified if you simply added 
>>>>dev->xfer_mask.
>>>>
>>>
>>>
>>>The thing is that ap->*_mask's are separated the same way and all
>>>masking constants are defined as such.  e.g.
>>>
>>>	ap->udma_mask &= ATA_UDMA5;
>>>or
>>>	ap->udma_mask &= ATA_UDMA_MASK_40C;
>>>
>>>Making dev->*_mask's the same enables share those constants and code
>>>convention.  So, things to consider here are...
>>>
>>>1. Port xfer masks are defined as three separate masks.
>>>
>>>2. All the constants are defined according to that.
>>>
>>>3. Three separate masks are easier to deal with for LLDD's.
>>
>>Separate masks is better for the LLDD interface, but the packed version 
>>seems superior for internal libata use.
> 
> 
> Yeah, dev->*_mask's will be used by LLDD's.  Actually, in patch #4 of
> this series, sata_sil's ->dev_config() does exactly that.  Also, mode
> mask filtering can be done by diddling dev->*_mask's.
> 
> 
>>No reason why ATA_UDMA_MASK_40C can't simply operate on a packed 
>>xfer_mask variable, for example.
> 
> 
> Because it's used to filter ap->*_mask and to allow LLDD's use the
> same constants on dev->*_mask's.
> 
> Do you think LLDD's shouldn't access dev->*_mask's?

When I spoke of "LLDD API", I largely meant the pio_mask/etc. in 
ata_port_info.  That's the easiest way to present such information to 
driver maintainers.

OTOH, at runtime ->dev_config() and friends should probably just 
manipulate dev->xfer_mask.

	Jeff

Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask

[Tejun Heo]

On Mon, Mar 13, 2006 at 05:13:36AM -0500, Jeff Garzik wrote:
> Tejun Heo wrote:
> >On Mon, Mar 13, 2006 at 04:52:28AM -0500, Jeff Garzik wrote:
> >
> >>Tejun Heo wrote:
> >>
> >>>On Mon, Mar 13, 2006 at 03:29:24AM -0500, Jeff Garzik wrote:
> >>>
> >>>
> >>>>Tejun Heo wrote:
> >>>>
> >>>>
> >>>>>Add per-dev pio/mwdma/udma_mask.  All transfer mode limits used to be
> >>>>>applied to ap->*_mask which unnecessarily restricted other devices
> >>>>>sharing the port.  This change will also benefit later EH speed down
> >>>>>and hotplug.
> >>>>>
> >>>>>Signed-off-by: Tejun Heo <htejun@gmail.com>
> >>>>
> >>>>I don't see much value in the separation.  Rather than 3 separate 
> >>>>masks, it seems like this patch would be simplified if you simply added 
> >>>>dev->xfer_mask.
> >>>>
> >>>
> >>>
> >>>The thing is that ap->*_mask's are separated the same way and all
> >>>masking constants are defined as such.  e.g.
> >>>
> >>>	ap->udma_mask &= ATA_UDMA5;
> >>>or
> >>>	ap->udma_mask &= ATA_UDMA_MASK_40C;
> >>>
> >>>Making dev->*_mask's the same enables share those constants and code
> >>>convention.  So, things to consider here are...
> >>>
> >>>1. Port xfer masks are defined as three separate masks.
> >>>
> >>>2. All the constants are defined according to that.
> >>>
> >>>3. Three separate masks are easier to deal with for LLDD's.
> >>
> >>Separate masks is better for the LLDD interface, but the packed version 
> >>seems superior for internal libata use.
> >
> >
> >Yeah, dev->*_mask's will be used by LLDD's.  Actually, in patch #4 of
> >this series, sata_sil's ->dev_config() does exactly that.  Also, mode
> >mask filtering can be done by diddling dev->*_mask's.
> >
> >
> >>No reason why ATA_UDMA_MASK_40C can't simply operate on a packed 
> >>xfer_mask variable, for example.
> >
> >
> >Because it's used to filter ap->*_mask and to allow LLDD's use the
> >same constants on dev->*_mask's.
> >
> >Do you think LLDD's shouldn't access dev->*_mask's?
> 
> When I spoke of "LLDD API", I largely meant the pio_mask/etc. in 
> ata_port_info.  That's the easiest way to present such information to 
> driver maintainers.
> 
> OTOH, at runtime ->dev_config() and friends should probably just 
> manipulate dev->xfer_mask.

I see.

So, ata_port_info uses pio/mwdma/udma_mask and ata_port and ata_device
use single xfer_mask and all the masking constants are converted to
mask single xfer_mask.  Sounds good to you?

-- 
tejun

Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask

[Jeff Garzik]

Tejun Heo wrote:
> On Mon, Mar 13, 2006 at 05:13:36AM -0500, Jeff Garzik wrote:
> 
>>Tejun Heo wrote:
>>
>>>On Mon, Mar 13, 2006 at 04:52:28AM -0500, Jeff Garzik wrote:
>>>
>>>
>>>>Tejun Heo wrote:
>>>>
>>>>
>>>>>On Mon, Mar 13, 2006 at 03:29:24AM -0500, Jeff Garzik wrote:
>>>>>
>>>>>
>>>>>
>>>>>>Tejun Heo wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>>Add per-dev pio/mwdma/udma_mask.  All transfer mode limits used to be
>>>>>>>applied to ap->*_mask which unnecessarily restricted other devices
>>>>>>>sharing the port.  This change will also benefit later EH speed down
>>>>>>>and hotplug.
>>>>>>>
>>>>>>>Signed-off-by: Tejun Heo <htejun@gmail.com>
>>>>>>
>>>>>>I don't see much value in the separation.  Rather than 3 separate 
>>>>>>masks, it seems like this patch would be simplified if you simply added 
>>>>>>dev->xfer_mask.
>>>>>>
>>>>>
>>>>>
>>>>>The thing is that ap->*_mask's are separated the same way and all
>>>>>masking constants are defined as such.  e.g.
>>>>>
>>>>>	ap->udma_mask &= ATA_UDMA5;
>>>>>or
>>>>>	ap->udma_mask &= ATA_UDMA_MASK_40C;
>>>>>
>>>>>Making dev->*_mask's the same enables share those constants and code
>>>>>convention.  So, things to consider here are...
>>>>>
>>>>>1. Port xfer masks are defined as three separate masks.
>>>>>
>>>>>2. All the constants are defined according to that.
>>>>>
>>>>>3. Three separate masks are easier to deal with for LLDD's.
>>>>
>>>>Separate masks is better for the LLDD interface, but the packed version 
>>>>seems superior for internal libata use.
>>>
>>>
>>>Yeah, dev->*_mask's will be used by LLDD's.  Actually, in patch #4 of
>>>this series, sata_sil's ->dev_config() does exactly that.  Also, mode
>>>mask filtering can be done by diddling dev->*_mask's.
>>>
>>>
>>>
>>>>No reason why ATA_UDMA_MASK_40C can't simply operate on a packed 
>>>>xfer_mask variable, for example.
>>>
>>>
>>>Because it's used to filter ap->*_mask and to allow LLDD's use the
>>>same constants on dev->*_mask's.
>>>
>>>Do you think LLDD's shouldn't access dev->*_mask's?
>>
>>When I spoke of "LLDD API", I largely meant the pio_mask/etc. in 
>>ata_port_info.  That's the easiest way to present such information to 
>>driver maintainers.
>>
>>OTOH, at runtime ->dev_config() and friends should probably just 
>>manipulate dev->xfer_mask.
> 
> 
> I see.
> 
> So, ata_port_info uses pio/mwdma/udma_mask and ata_port and ata_device
> use single xfer_mask and all the masking constants are converted to
> mask single xfer_mask.  Sounds good to you?

Well, if I read him correctly, Alan seems to have weighted in on the 
side of pio/mwdma/udma_mask.

Maybe we could go with your original patches' direction for now, and 
then move to a consolidated xfer_mask internally later.

	Jeff

Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask

[Alan Cox]

On Llu, 2006-03-20 at 20:56 -0500, Jeff Garzik wrote:
> > So, ata_port_info uses pio/mwdma/udma_mask and ata_port and ata_device
> > use single xfer_mask and all the masking constants are converted to
> > mask single xfer_mask.  Sounds good to you?
> 
> Well, if I read him correctly, Alan seems to have weighted in on the 
> side of pio/mwdma/udma_mask.
> 
> Maybe we could go with your original patches' direction for now, and 
> then move to a consolidated xfer_mask internally later.


What I've done for the filter side of things is kept the combined mask
as what is filtered by the driver, and that looks tidy. What came out
ugly that I objected to was the horribly mess you got when declaring all
the driver modes available.

[PATCH 1/4] libata: implement ata_unpack_xfermask()

[Tejun Heo]

Implement ata_unpack_xfermask().

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

70ca1eb15f59fa81dc3dd8b8eaff40f7b4e07c95
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 42d43b5..96954c7 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -252,6 +252,29 @@ static unsigned int ata_pack_xfermask(un
 		((udma_mask << ATA_SHIFT_UDMA) & ATA_MASK_UDMA);
 }
 
+/**
+ *	ata_unpack_xfermask - Unpack xfer_mask into pio, mwdma and udma masks
+ *	@xfer_mask: xfer_mask to unpack
+ *	@pio_mask: resulting pio_mask
+ *	@mwdma_mask: resulting mwdma_mask
+ *	@udma_mask: resulting udma_mask
+ *
+ *	Unpack @xfer_mask into @pio_mask, @mwdma_mask and @udma_mask.
+ *	Any NULL distination masks will be ignored.
+ */
+static void ata_unpack_xfermask(unsigned int xfer_mask,
+				unsigned int *pio_mask,
+				unsigned int *mwdma_mask,
+				unsigned int *udma_mask)
+{
+	if (pio_mask)
+		*pio_mask = (xfer_mask & ATA_MASK_PIO) >> ATA_SHIFT_PIO;
+	if (mwdma_mask)
+		*mwdma_mask = (xfer_mask & ATA_MASK_MWDMA) >> ATA_SHIFT_MWDMA;
+	if (udma_mask)
+		*udma_mask = (xfer_mask & ATA_MASK_UDMA) >> ATA_SHIFT_UDMA;
+}
+
 static const struct ata_xfer_ent {
 	unsigned int shift, bits;
 	u8 base;
-- 
1.2.4

Re: [PATCH 1/4] libata: implement ata_unpack_xfermask()

[Jeff Garzik]

Tejun Heo wrote:
> Implement ata_unpack_xfermask().
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK presuming it is still needed after comments...

[PATCH 4/4] libata: make per-dev transfer mode limits per-dev

[Tejun Heo]

Now that each ata_device has xfer masks, per-dev limits can be made
per-dev instead of per-port.  Make per-dev limits per-dev.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    2 +-
 drivers/scsi/sata_sil.c    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

c54ae650a5eeee6dc67e32b52be23d29f8e4d16f
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index beedc4b..8db5458 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1350,7 +1350,7 @@ static int ata_dev_configure(struct ata_
 		if (print_info)
 			printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
 			       ap->id, dev->devno);
-		ap->udma_mask &= ATA_UDMA5;
+		dev->udma_mask &= ATA_UDMA5;
 		dev->max_sectors = ATA_MAX_SECTORS;
 	}
 
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index 4f2a67e..3746f97 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -372,7 +372,7 @@ static void sil_dev_config(struct ata_po
 	if (quirks & SIL_QUIRK_UDMA5MAX) {
 		printk(KERN_INFO "ata%u(%u): applying Maxtor errata fix %s\n",
 		       ap->id, dev->devno, model_num);
-		ap->udma_mask &= ATA_UDMA5;
+		dev->udma_mask &= ATA_UDMA5;
 		return;
 	}
 }
-- 
1.2.4

Re: [PATCH 4/4] libata: make per-dev transfer mode limits per-dev

[Jeff Garzik]

Tejun Heo wrote:
> Now that each ata_device has xfer masks, per-dev limits can be made
> per-dev instead of per-port.  Make per-dev limits per-dev.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Doesn't this patch break stuff?  As you said in a previous patch, the 
per-dev masks aren't used yet?

	Jeff

Re: [PATCH 4/4] libata: make per-dev transfer mode limits per-dev

[Tejun Heo]

On Mon, Mar 13, 2006 at 03:30:41AM -0500, Jeff Garzik wrote:
> Tejun Heo wrote:
> >Now that each ata_device has xfer masks, per-dev limits can be made
> >per-dev instead of per-port.  Make per-dev limits per-dev.
> >
> >Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> Doesn't this patch break stuff?  As you said in a previous patch, the 
> per-dev masks aren't used yet?
> 

No, it doesn't break stuff.  per-dev masks are applied in
ata_dev_xfermask().

@@ -2667,6 +2670,8 @@ static unsigned int ata_dev_xfermask(str
 		struct ata_device *d = &ap->device[i];
 		if (!ata_dev_present(d))
 			continue;
+		xfer_mask &= ata_pack_xfermask(d->pio_mask, d->mwdma_mask,
+					       d->udma_mask);
 		xfer_mask &= ata_id_xfermask(d->id);
 		if (ata_dma_blacklisted(d))
 			xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);

-- 
tejun

Re: [PATCHSET] libata: implement per-dev xfer masks

[Alan Cox]

On Sul, 2006-03-12 at 16:02 +0900, Tejun Heo wrote:
> I marked with comment where the mode filtering for low level drivers
> should be done but I'm not sure how the interface should look like.

Currently the libata pata patch code calls a filter function if present
which returns an updated bitmap for the mode type. I did that because we
need to combine the cable, bridge, core and chip specific rules neatly
and not have one forcing a mode the other cannot do.

> ->pre_set_mode(ap) is okay but then the LLDD should iterate over all
> devices.  Maybe ->pre_set_mode(ap, dev) or ->filter_xfer_mask(ap, dev)?

Providing

1.	The proposed speeds are set for both devices on the cable before the
call.
2.	We reselect the speed of both devices on changedown or hotplug

then we should be just fine.

Re: [PATCHSET] libata: implement per-dev xfer masks

[Tejun Heo]

On Sun, Mar 12, 2006 at 01:37:26PM +0000, Alan Cox wrote:
> On Sul, 2006-03-12 at 16:02 +0900, Tejun Heo wrote:
> > I marked with comment where the mode filtering for low level drivers
> > should be done but I'm not sure how the interface should look like.
> 
> Currently the libata pata patch code calls a filter function if present
> which returns an updated bitmap for the mode type. I did that because we
> need to combine the cable, bridge, core and chip specific rules neatly
> and not have one forcing a mode the other cannot do.

Understood.

> > ->pre_set_mode(ap) is okay but then the LLDD should iterate over all
> > devices.  Maybe ->pre_set_mode(ap, dev) or ->filter_xfer_mask(ap, dev)?
> 
> Providing
> 
> 1.	The proposed speeds are set for both devices on the cable before the
> call.
> 2.	We reselect the speed of both devices on changedown or hotplug
> 
> then we should be just fine.

Yeap, both will hold.  I'll just leave it as it is such that filtering
callback can be added later when merging PATA code.  One concern I
have about the originally proposed ->mode_filter(ap, dev, mask, shift)
interface is that if a LLDD wants to reject all UDMA and limit some of
MWDMA, there is no way to do so.  I think it's better and easier to
filter all mode masks at once.

Thanks.

-- 
tejun

Re: [PATCHSET] libata: implement per-dev xfer masks

[Alan Cox]

On Llu, 2006-03-13 at 15:12 +0900, Tejun Heo wrote:
> callback can be added later when merging PATA code.  One concern I
> have about the originally proposed ->mode_filter(ap, dev, mask, shift)
> interface is that if a LLDD wants to reject all UDMA and limit some of
> MWDMA, there is no way to do so.  I think it's better and easier to
> filter all mode masks at once.


The current code is called three times once for PIO once for MWDMA and
once for UDMA (since we don't do SWDMA yet). Doing it as one seems
sensible enough, it just didn't fit how the old code worked.


I also agree strongly we need ap-> and dev-> masks in the same format
throughout libata and the drivers or there are going to be disasters