libata: implement per-dev xfer masks

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

Quoted 2 lines · Tejun Heo

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.

Quoted 2 lines

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

Implement ata_unpack_xfermask().

Signed-off-by Tejun Heo <[email protected]>

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 <[email protected]>

---

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

drivers/scsi/libata-core.c +22 -3

@@ -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;

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 <[email protected]>

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 <[email protected]>

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]

Quoted 5 lines · 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.

Quoted 1 line

Signed-off-by Tejun Heo <[email protected]>

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.

Quoted 4 lines · 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 <[email protected]>

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

Quoted 11 lines · Jeff Garzik

Quoted 6 lines · 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 <[email protected]>

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.

Quoted 27 lines · Tejun Heo

Quoted 7 lines · Jeff Garzik

Quoted 5 lines · 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.

Quoted 6 lines

Quoted 1 line

Signed-off-by Tejun Heo <[email protected]>

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

Quoted 5 lines

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.

Quoted 29 lines · Jeff Garzik

Quoted 27 lines · Tejun Heo

Quoted 12 lines · Jeff Garzik

Quoted 6 lines · 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 <[email protected]>

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

Quoted 7 lines

Quoted 5 lines

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.

Quoted 1 line

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?

Quoted 55 lines · Tejun Heo

Quoted 31 lines · Jeff Garzik

Quoted 29 lines · Tejun Heo

Quoted 8 lines · Jeff Garzik

Quoted 5 lines · 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.

Quoted 6 lines

Quoted 1 line

Signed-off-by Tejun Heo <[email protected]>

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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.

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.

Quoted 62 lines · Jeff Garzik

Quoted 50 lines · Tejun Heo

Quoted 32 lines · Jeff Garzik

Quoted 29 lines · Tejun Heo

Quoted 4 lines · Jeff Garzik

Tejun Heo wrote:

Quoted 9 lines

Quoted 6 lines

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 <[email protected]>

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

Quoted 7 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 1 line

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.

Quoted 1 line

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?

Quoted 3 lines

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.