* [PATCH 2/4] libata: implement ata_dev_init()
2006-03-12 7:02 [PATCHSET] libata: implement per-dev xfer masks Tejun Heo
@ 2006-03-12 7:02 ` Tejun Heo
2006-03-13 6:19 ` Tejun Heo
2006-03-12 7:02 ` [PATCH 1/4] libata: implement ata_unpack_xfermask() Tejun Heo
` (3 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2006-03-12 7:02 UTC (permalink / raw)
To: jgarzik, albertcc, alan, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 2/4] libata: implement ata_dev_init()
2006-03-12 7:02 ` [PATCH 2/4] libata: implement ata_dev_init() Tejun Heo
@ 2006-03-13 6:19 ` Tejun Heo
2006-03-13 8:26 ` Jeff Garzik
0 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2006-03-13 6:19 UTC (permalink / raw)
To: jgarzik, albertcc, alan, linux-ide
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;
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/4] libata: implement ata_dev_init()
2006-03-13 6:19 ` Tejun Heo
@ 2006-03-13 8:26 ` Jeff Garzik
2006-03-13 9:25 ` Tejun Heo
0 siblings, 1 reply; 22+ messages in thread
From: Jeff Garzik @ 2006-03-13 8:26 UTC (permalink / raw)
To: Tejun Heo; +Cc: albertcc, alan, linux-ide
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]
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/4] libata: implement ata_dev_init()
2006-03-13 8:26 ` Jeff Garzik
@ 2006-03-13 9:25 ` Tejun Heo
0 siblings, 0 replies; 22+ messages in thread
From: Tejun Heo @ 2006-03-13 9:25 UTC (permalink / raw)
To: Jeff Garzik; +Cc: albertcc, alan, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/4] libata: implement ata_unpack_xfermask()
2006-03-12 7:02 [PATCHSET] libata: implement per-dev xfer masks Tejun Heo
2006-03-12 7:02 ` [PATCH 2/4] libata: implement ata_dev_init() Tejun Heo
@ 2006-03-12 7:02 ` Tejun Heo
2006-03-13 8:23 ` Jeff Garzik
2006-03-12 7:02 ` [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask Tejun Heo
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2006-03-12 7:02 UTC (permalink / raw)
To: jgarzik, albertcc, alan, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask
2006-03-12 7:02 [PATCHSET] libata: implement per-dev xfer masks Tejun Heo
2006-03-12 7:02 ` [PATCH 2/4] libata: implement ata_dev_init() Tejun Heo
2006-03-12 7:02 ` [PATCH 1/4] libata: implement ata_unpack_xfermask() Tejun Heo
@ 2006-03-12 7:02 ` Tejun Heo
2006-03-13 8:29 ` Jeff Garzik
2006-03-12 7:02 ` [PATCH 4/4] libata: make per-dev transfer mode limits per-dev Tejun Heo
2006-03-12 13:37 ` [PATCHSET] libata: implement per-dev xfer masks Alan Cox
4 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2006-03-12 7:02 UTC (permalink / raw)
To: jgarzik, albertcc, alan, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask
2006-03-12 7:02 ` [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask Tejun Heo
@ 2006-03-13 8:29 ` Jeff Garzik
2006-03-13 9:30 ` Tejun Heo
0 siblings, 1 reply; 22+ messages in thread
From: Jeff Garzik @ 2006-03-13 8:29 UTC (permalink / raw)
To: Tejun Heo; +Cc: albertcc, alan, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask
2006-03-13 8:29 ` Jeff Garzik
@ 2006-03-13 9:30 ` Tejun Heo
2006-03-13 9:52 ` Jeff Garzik
0 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2006-03-13 9:30 UTC (permalink / raw)
To: Jeff Garzik; +Cc: albertcc, alan, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask
2006-03-13 9:30 ` Tejun Heo
@ 2006-03-13 9:52 ` Jeff Garzik
2006-03-13 10:09 ` Tejun Heo
0 siblings, 1 reply; 22+ messages in thread
From: Jeff Garzik @ 2006-03-13 9:52 UTC (permalink / raw)
To: Tejun Heo; +Cc: albertcc, alan, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask
2006-03-13 9:52 ` Jeff Garzik
@ 2006-03-13 10:09 ` Tejun Heo
2006-03-13 10:13 ` Jeff Garzik
0 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2006-03-13 10:09 UTC (permalink / raw)
To: Jeff Garzik; +Cc: albertcc, alan, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask
2006-03-13 10:09 ` Tejun Heo
@ 2006-03-13 10:13 ` Jeff Garzik
2006-03-13 10:24 ` Tejun Heo
0 siblings, 1 reply; 22+ messages in thread
From: Jeff Garzik @ 2006-03-13 10:13 UTC (permalink / raw)
To: Tejun Heo; +Cc: albertcc, alan, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask
2006-03-13 10:13 ` Jeff Garzik
@ 2006-03-13 10:24 ` Tejun Heo
2006-03-21 1:56 ` Jeff Garzik
0 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2006-03-13 10:24 UTC (permalink / raw)
To: Jeff Garzik; +Cc: albertcc, alan, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask
2006-03-13 10:24 ` Tejun Heo
@ 2006-03-21 1:56 ` Jeff Garzik
2006-03-21 10:25 ` Alan Cox
0 siblings, 1 reply; 22+ messages in thread
From: Jeff Garzik @ 2006-03-21 1:56 UTC (permalink / raw)
To: Tejun Heo; +Cc: albertcc, alan, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask
2006-03-21 1:56 ` Jeff Garzik
@ 2006-03-21 10:25 ` Alan Cox
0 siblings, 0 replies; 22+ messages in thread
From: Alan Cox @ 2006-03-21 10:25 UTC (permalink / raw)
To: Jeff Garzik; +Cc: Tejun Heo, albertcc, linux-ide
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/4] libata: make per-dev transfer mode limits per-dev
2006-03-12 7:02 [PATCHSET] libata: implement per-dev xfer masks Tejun Heo
` (2 preceding siblings ...)
2006-03-12 7:02 ` [PATCH 3/4] libata: add per-dev pio/mwdma/udma_mask Tejun Heo
@ 2006-03-12 7:02 ` Tejun Heo
2006-03-13 8:30 ` Jeff Garzik
2006-03-12 13:37 ` [PATCHSET] libata: implement per-dev xfer masks Alan Cox
4 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2006-03-12 7:02 UTC (permalink / raw)
To: jgarzik, albertcc, alan, linux-ide; +Cc: 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/4] libata: make per-dev transfer mode limits per-dev
2006-03-12 7:02 ` [PATCH 4/4] libata: make per-dev transfer mode limits per-dev Tejun Heo
@ 2006-03-13 8:30 ` Jeff Garzik
2006-03-13 9:33 ` Tejun Heo
0 siblings, 1 reply; 22+ messages in thread
From: Jeff Garzik @ 2006-03-13 8:30 UTC (permalink / raw)
To: Tejun Heo; +Cc: albertcc, alan, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/4] libata: make per-dev transfer mode limits per-dev
2006-03-13 8:30 ` Jeff Garzik
@ 2006-03-13 9:33 ` Tejun Heo
0 siblings, 0 replies; 22+ messages in thread
From: Tejun Heo @ 2006-03-13 9:33 UTC (permalink / raw)
To: Jeff Garzik; +Cc: albertcc, alan, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHSET] libata: implement per-dev xfer masks
2006-03-12 7:02 [PATCHSET] libata: implement per-dev xfer masks Tejun Heo
` (3 preceding siblings ...)
2006-03-12 7:02 ` [PATCH 4/4] libata: make per-dev transfer mode limits per-dev Tejun Heo
@ 2006-03-12 13:37 ` Alan Cox
2006-03-13 6:12 ` Tejun Heo
4 siblings, 1 reply; 22+ messages in thread
From: Alan Cox @ 2006-03-12 13:37 UTC (permalink / raw)
To: Tejun Heo; +Cc: jgarzik, albertcc, linux-ide
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCHSET] libata: implement per-dev xfer masks
2006-03-12 13:37 ` [PATCHSET] libata: implement per-dev xfer masks Alan Cox
@ 2006-03-13 6:12 ` Tejun Heo
2006-03-13 11:41 ` Alan Cox
0 siblings, 1 reply; 22+ messages in thread
From: Tejun Heo @ 2006-03-13 6:12 UTC (permalink / raw)
To: Alan Cox; +Cc: jgarzik, albertcc, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCHSET] libata: implement per-dev xfer masks
2006-03-13 6:12 ` Tejun Heo
@ 2006-03-13 11:41 ` Alan Cox
0 siblings, 0 replies; 22+ messages in thread
From: Alan Cox @ 2006-03-13 11:41 UTC (permalink / raw)
To: Tejun Heo; +Cc: jgarzik, albertcc, linux-ide
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
^ permalink raw reply [flat|nested] 22+ messages in thread