Following up:
[PATCH 1/7] firewire: sbp2: fix payload limit at S1600 and S3200
[PATCH 2/7] firewire: sbp2: define some magic numbers as macros
[PATCH 3/7] ieee1394: sbp2: fix payload limit at S1600 and S3200
[PATCH 4/7] ieee1394: sbp2: don't assume zero model_id or firmware_revision if there is none
[PATCH 5/7] ieee1394: sbp2: follow up to "ieee1394: inherit ud vendor_id from node vendor_id"
[PATCH RFT 6/7] ieee1394: sbp2: add workarounds for 2nd and 3rd generation iPods
[PATCH RFT 7/7] firewire: sbp2: add workarounds for 2nd and 3rd generation iPods
drivers/firewire/fw-sbp2.c | 73 +++++++++++++++++++++----------------
drivers/ieee1394/sbp2.c | 60 +++++++++++++++++++-----------
drivers/ieee1394/sbp2.h | 1
3 files changed, 83 insertions(+), 51 deletions(-)
--
Stefan Richter
-=====-==--= ---= ==---
http://arcgraph.de/sr/
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/fw-sbp2.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -311,14 +311,16 @@ struct sbp2_command_orb {
dma_addr_t page_table_bus;
};
+#define SBP2_ROM_VALUE_WILDCARD ~0 /* match all */
+#define SBP2_ROM_VALUE_MISSING 0xff000000 /* not present in the unit dir. */
+
/*
* List of devices with known bugs.
*
* The firmware_revision field, masked with 0xffff00, is the best
* indicator for the type of bridge chip of a device. It yields a few
* false positives but this did not break correctly behaving devices
- * so far. We use ~0 as a wildcard, since the 24 bit values we get
- * from the config rom can never match that.
+ * so far.
*/
static const struct {
u32 firmware_revision;
@@ -340,22 +342,22 @@ static const struct {
},
/* Initio bridges, actually only needed for some older ones */ {
.firmware_revision = 0x000200,
- .model = ~0,
+ .model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_INQUIRY_36,
},
/* PL-3507 bridge with Prolific firmware */ {
.firmware_revision = 0x012800,
- .model = ~0,
+ .model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_POWER_CONDITION,
},
/* Symbios bridge */ {
.firmware_revision = 0xa0b800,
- .model = ~0,
+ .model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_128K_MAX_TRANS,
},
/* Datafab MD2-FW2 with Symbios/LSILogic SYM13FW500 bridge */ {
.firmware_revision = 0x002600,
- .model = ~0,
+ .model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_128K_MAX_TRANS,
},
@@ -1087,7 +1089,7 @@ static void sbp2_init_workarounds(struct
continue;
if (sbp2_workarounds_table[i].model != model &&
- sbp2_workarounds_table[i].model != ~0)
+ sbp2_workarounds_table[i].model != SBP2_ROM_VALUE_WILDCARD)
continue;
w |= sbp2_workarounds_table[i].workarounds;
@@ -1137,14 +1139,13 @@ static int sbp2_probe(struct device *dev
fw_device_get(device);
fw_unit_get(unit);
- /* Initialize to values that won't match anything in our table. */
- firmware_revision = 0xff000000;
- model = 0xff000000;
-
/* implicit directory ID */
tgt->directory_id = ((unit->directory - device->config_rom) * 4
+ CSR_CONFIG_ROM) & 0xffffff;
+ firmware_revision = SBP2_ROM_VALUE_MISSING;
+ model = SBP2_ROM_VALUE_MISSING;
+
if (sbp2_scan_unit_dir(tgt, unit->directory, &model,
&firmware_revision) < 0)
goto fail_tgt_put;
--
Stefan Richter
-=====-==--= ---= ==---
http://arcgraph.de/sr/
This makes sbp2 behave more like firewire-sbp2 which reports 0xff000000
as immediate value if there are no unit directory entries for model_id
or firmware_revision.
It does not reduce matches with the currently existing quirks table; the
only zero entry there is for a device which actually does have a zero
model_id. It only changes how model_id and firmware_revision are logged
if they are missing.
Other functionally unrelated changes: The model_id member of quirks
list entries is renamed to model; the value (but not teh effect) of
SBP2_ROM_VALUE_WILDCARD is changed. Now this part of the source is
identical with firewire-sbp2 for easier maintenance.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/sbp2.c | 38 ++++++++++++++++++++------------------
1 file changed, 20 insertions(+), 18 deletions(-)
Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c
+++ linux/drivers/ieee1394/sbp2.c
@@ -347,8 +347,8 @@ static struct scsi_host_template sbp2_sh
.sdev_attrs = sbp2_sysfs_sdev_attrs,
};
-/* for match-all entries in sbp2_workarounds_table */
-#define SBP2_ROM_VALUE_WILDCARD 0x1000000
+#define SBP2_ROM_VALUE_WILDCARD ~0 /* match all */
+#define SBP2_ROM_VALUE_MISSING 0xff000000 /* not present in the unit dir. */
/*
* List of devices with known bugs.
@@ -359,60 +359,60 @@ static struct scsi_host_template sbp2_sh
*/
static const struct {
u32 firmware_revision;
- u32 model_id;
+ u32 model;
unsigned workarounds;
} sbp2_workarounds_table[] = {
/* DViCO Momobay CX-1 with TSB42AA9 bridge */ {
.firmware_revision = 0x002800,
- .model_id = 0x001010,
+ .model = 0x001010,
.workarounds = SBP2_WORKAROUND_INQUIRY_36 |
SBP2_WORKAROUND_MODE_SENSE_8 |
SBP2_WORKAROUND_POWER_CONDITION,
},
/* DViCO Momobay FX-3A with TSB42AA9A bridge */ {
.firmware_revision = 0x002800,
- .model_id = 0x000000,
+ .model = 0x000000,
.workarounds = SBP2_WORKAROUND_DELAY_INQUIRY |
SBP2_WORKAROUND_POWER_CONDITION,
},
/* Initio bridges, actually only needed for some older ones */ {
.firmware_revision = 0x000200,
- .model_id = SBP2_ROM_VALUE_WILDCARD,
+ .model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_INQUIRY_36,
},
/* PL-3507 bridge with Prolific firmware */ {
.firmware_revision = 0x012800,
- .model_id = SBP2_ROM_VALUE_WILDCARD,
+ .model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_POWER_CONDITION,
},
/* Symbios bridge */ {
.firmware_revision = 0xa0b800,
- .model_id = SBP2_ROM_VALUE_WILDCARD,
+ .model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_128K_MAX_TRANS,
},
/* Datafab MD2-FW2 with Symbios/LSILogic SYM13FW500 bridge */ {
.firmware_revision = 0x002600,
- .model_id = SBP2_ROM_VALUE_WILDCARD,
+ .model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_128K_MAX_TRANS,
},
/* iPod 4th generation */ {
.firmware_revision = 0x0a2700,
- .model_id = 0x000021,
+ .model = 0x000021,
.workarounds = SBP2_WORKAROUND_FIX_CAPACITY,
},
/* iPod mini */ {
.firmware_revision = 0x0a2700,
- .model_id = 0x000022,
+ .model = 0x000022,
.workarounds = SBP2_WORKAROUND_FIX_CAPACITY,
},
/* iPod mini */ {
.firmware_revision = 0x0a2700,
- .model_id = 0x000023,
+ .model = 0x000023,
.workarounds = SBP2_WORKAROUND_FIX_CAPACITY,
},
/* iPod Photo */ {
.firmware_revision = 0x0a2700,
- .model_id = 0x00007e,
+ .model = 0x00007e,
.workarounds = SBP2_WORKAROUND_FIX_CAPACITY,
}
};
@@ -1341,13 +1341,15 @@ static void sbp2_parse_unit_directory(st
struct csr1212_keyval *kv;
struct csr1212_dentry *dentry;
u64 management_agent_addr;
- u32 unit_characteristics, firmware_revision;
+ u32 unit_characteristics, firmware_revision, model;
unsigned workarounds;
int i;
management_agent_addr = 0;
unit_characteristics = 0;
- firmware_revision = 0;
+ firmware_revision = SBP2_ROM_VALUE_MISSING;
+ model = ud->flags & UNIT_DIRECTORY_MODEL_ID ?
+ ud->model_id : SBP2_ROM_VALUE_MISSING;
csr1212_for_each_dir_entry(ud->ne->csr, kv, ud->ud_kv, dentry) {
switch (kv->key.id) {
@@ -1388,9 +1390,9 @@ static void sbp2_parse_unit_directory(st
sbp2_workarounds_table[i].firmware_revision !=
(firmware_revision & 0xffff00))
continue;
- if (sbp2_workarounds_table[i].model_id !=
+ if (sbp2_workarounds_table[i].model !=
SBP2_ROM_VALUE_WILDCARD &&
- sbp2_workarounds_table[i].model_id != ud->model_id)
+ sbp2_workarounds_table[i].model != model)
continue;
workarounds |= sbp2_workarounds_table[i].workarounds;
break;
@@ -1403,7 +1405,7 @@ static void sbp2_parse_unit_directory(st
NODE_BUS_ARGS(ud->ne->host, ud->ne->nodeid),
workarounds, firmware_revision,
ud->vendor_id ? ud->vendor_id : ud->ne->vendor_id,
- ud->model_id);
+ model);
/* We would need one SCSI host template for each target to adjust
* max_sectors on the fly, therefore warn only. */
--
Stefan Richter
-=====-==--= ---= ==---
http://arcgraph.de/sr/
According to https://bugs.launchpad.net/bugs/294391
- 3rd generation iPods need the "fix capacity" workaround after all
(apparently they crash after the last sector was accessed),
- 2nd generation iPods need the "128 kB maximum request size"
workaround.
Furthermore, Kristian H?gsberg mentioned that 2nd generation iPods
treat page tables (scatter-gather lists) as data buffers, i.e. ignore
the page_table_present bit of command block ORBs. If this is true, then
the 128 kB limit is not a sufficient workaround; instead we have to
guarantee that all requests have only a single s/g element. This could
be up to 64 kByte - 4 Bytes in size. But I expect the block layer to
typically emit much smaller elements than that, alas.
Alas both iPod generations feature the same model ID in the config ROM,
hence we can only define a shared quirks list entry for them. This is
bad since
- the "fix capacity" workaround prevents access to the very last
sector if a device does not actually have this bug,
- the "no page tables" workaround dramatically reduces throughput,
e.g. from 25 to 9 MB/s without gap count optimization, and from 36
11 MB/s with gap count optimization (as per hdparm with a regular
3.5" S400 disk on an i686 PC without IOMMU). Whether this matters
with the already slow iPod disk is not yet known.
This patch needs testing by somebody who has the hardware:
- Do 2nd and 3rd gen. iPods actually have a model_id == 0, or do they
have in fact no model_id at all?
- Does the "fix capacity" workaround harm usage of 2nd gen. iPods?
- Is the "no page tables" workaround really necessary for 2nd gen.
iPods, or is the 128k limit sufficient as reported for the Ubuntu
8.10 kernel?
- How badly does the "no page tables" workaround affect throughput of
3rd gen. iPods which don't need it?
A side note: Apple computers in target mode (or at least an x86 Mac
mini) don't have firmware_version and model_id, hence none of the iPod
quirks list entries is active for them.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/fw-sbp2.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -92,6 +92,10 @@ MODULE_PARM_DESC(exclusive_login, "Exclu
* sd_mod on suspend, resume, and shutdown (if manage_start_stop is on).
* Some disks need this to spin down or to resume properly.
*
+ * - no page tables
+ * Limit data buffers to a single scatter/ gather element for buggy devices
+ * which ignore the page_table_present bit in command block ORBs.
+ *
* - override internal blacklist
* Instead of adding to the built-in blacklist, use only the workarounds
* specified in the module load parameter.
@@ -104,6 +108,7 @@ MODULE_PARM_DESC(exclusive_login, "Exclu
#define SBP2_WORKAROUND_DELAY_INQUIRY 0x10
#define SBP2_INQUIRY_DELAY 12
#define SBP2_WORKAROUND_POWER_CONDITION 0x20
+#define SBP2_WORKAROUND_NO_PAGE_TABLES 0x40
#define SBP2_WORKAROUND_OVERRIDE 0x100
static int sbp2_param_workarounds;
@@ -116,6 +121,7 @@ MODULE_PARM_DESC(workarounds, "Work arou
", delay inquiry = " __stringify(SBP2_WORKAROUND_DELAY_INQUIRY)
", set power condition in start stop unit = "
__stringify(SBP2_WORKAROUND_POWER_CONDITION)
+ ", no page tables = " __stringify(SBP2_WORKAROUND_NO_PAGE_TABLES)
", override internal blacklist = " __stringify(SBP2_WORKAROUND_OVERRIDE)
", or a combination)");
@@ -325,7 +331,7 @@ struct sbp2_command_orb {
static const struct {
u32 firmware_revision;
u32 model;
- unsigned int workarounds;
+ unsigned workarounds;
} sbp2_workarounds_table[] = {
/* DViCO Momobay CX-1 with TSB42AA9 bridge */ {
.firmware_revision = 0x002800,
@@ -360,15 +366,17 @@ static const struct {
.model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_128K_MAX_TRANS,
},
-
/*
- * There are iPods (2nd gen, 3rd gen) with model_id == 0, but
- * these iPods do not feature the read_capacity bug according
- * to one report. Read_capacity behaviour as well as model_id
- * could change due to Apple-supplied firmware updates though.
+ * iPod 2nd generation: needs no-page-tables workaround
+ * iPod 3rd generation: needs fix-capacity workaround
*/
-
- /* iPod 4th generation. */ {
+ {
+ .firmware_revision = 0x0a2700,
+ .model = 0x000000,
+ .workarounds = SBP2_WORKAROUND_NO_PAGE_TABLES |
+ SBP2_WORKAROUND_FIX_CAPACITY,
+ },
+ /* iPod 4th generation */ {
.firmware_revision = 0x0a2700,
.model = 0x000021,
.workarounds = SBP2_WORKAROUND_FIX_CAPACITY,
@@ -1540,6 +1548,9 @@ static int sbp2_scsi_slave_configure(str
if (lu->tgt->workarounds & SBP2_WORKAROUND_128K_MAX_TRANS)
blk_queue_max_sectors(sdev->request_queue, 128 * 1024 / 512);
+ if (lu->tgt->workarounds & SBP2_WORKAROUND_NO_PAGE_TABLES)
+ blk_queue_max_hw_segments(sdev->request_queue, 1);
+
blk_queue_max_segment_size(sdev->request_queue, SBP2_MAX_SEG_SIZE);
return 0;
--
Stefan Richter
-=====-==--= ---= ==---
http://arcgraph.de/sr/
According to https://bugs.launchpad.net/bugs/294391
- 3rd generation iPods need the "fix capacity" workaround after all
(apparently they crash after the last sector was accessed),
- 2nd generation iPods need the "128 kB maximum request size"
workaround.
Furthermore, Kristian H?gsberg mentioned that 2nd generation iPods
treat page tables (scatter-gather lists) as data buffers, i.e. ignore
the page_table_present bit of command block ORBs. If this is true, then
the 128 kB limit is not a sufficient workaround; instead we have to
guarantee that all requests have only a single s/g element. This could
be up to 64 kByte - 4 Bytes in size. But I expect the block layer to
typically emit much smaller elements than that, alas.
Alas both iPod generations feature the same model ID in the config ROM,
hence we can only define a shared quirks list entry for them. This is
bad since
- the "fix capacity" workaround prevents access to the very last
sector if a device does not actually have this bug,
- the "no page tables" workaround dramatically reduces throughput,
e.g. from 25 to 9 MB/s without gap count optimization, and from 36
11 MB/s with gap count optimization (as per hdparm with a regular
3.5" S400 disk on an i686 PC without IOMMU). Whether this matters
with the already slow iPod disk is not yet known.
This patch needs testing by somebody who has the hardware:
- Do 2nd and 3rd gen. iPods actually have a model_id == 0, or do they
have in fact no model_id at all?
- Does the "fix capacity" workaround harm usage of 2nd gen. iPods?
- Is the "no page tables" workaround really necessary for 2nd gen.
iPods, or is the 128k limit sufficient as reported for the Ubuntu
8.10 kernel?
- How badly does the "no page tables" workaround affect throughput of
3rd gen. iPods which don't need it?
A side note: Apple computers in target mode (or at least an x86 Mac
mini) don't have firmware_version and model_id, hence none of the iPod
quirks list entries is active for them.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/sbp2.c | 17 +++++++++++++++++
drivers/ieee1394/sbp2.h | 1 +
2 files changed, 18 insertions(+)
Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c
+++ linux/drivers/ieee1394/sbp2.c
@@ -191,6 +191,10 @@ MODULE_PARM_DESC(exclusive_login, "Exclu
* sd_mod on suspend, resume, and shutdown (if manage_start_stop is on).
* Some disks need this to spin down or to resume properly.
*
+ * - no page tables
+ * Limit data buffers to a single scatter/ gather element for buggy devices
+ * which ignore the page_table_present bit in command block ORBs.
+ *
* - override internal blacklist
* Instead of adding to the built-in blacklist, use only the workarounds
* specified in the module load parameter.
@@ -206,6 +210,7 @@ MODULE_PARM_DESC(workarounds, "Work arou
", delay inquiry = " __stringify(SBP2_WORKAROUND_DELAY_INQUIRY)
", set power condition in start stop unit = "
__stringify(SBP2_WORKAROUND_POWER_CONDITION)
+ ", no page tables = " __stringify(SBP2_WORKAROUND_NO_PAGE_TABLES)
", override internal blacklist = " __stringify(SBP2_WORKAROUND_OVERRIDE)
", or a combination)");
@@ -395,6 +400,16 @@ static const struct {
.model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_128K_MAX_TRANS,
},
+ /*
+ * iPod 2nd generation: needs no-page-tables workaround
+ * iPod 3rd generation: needs fix-capacity workaround
+ */
+ {
+ .firmware_revision = 0x0a2700,
+ .model = 0x000000,
+ .workarounds = SBP2_WORKAROUND_NO_PAGE_TABLES |
+ SBP2_WORKAROUND_FIX_CAPACITY,
+ },
/* iPod 4th generation */ {
.firmware_revision = 0x0a2700,
.model = 0x000021,
@@ -2011,6 +2026,8 @@ static int sbp2scsi_slave_configure(stru
sdev->start_stop_pwr_cond = 1;
if (lu->workarounds & SBP2_WORKAROUND_128K_MAX_TRANS)
blk_queue_max_sectors(sdev->request_queue, 128 * 1024 / 512);
+ if (lu->workarounds & SBP2_WORKAROUND_NO_PAGE_TABLES)
+ blk_queue_max_hw_segments(sdev->request_queue, 1);
blk_queue_max_segment_size(sdev->request_queue, SBP2_MAX_SEG_SIZE);
return 0;
Index: linux/drivers/ieee1394/sbp2.h
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.h
+++ linux/drivers/ieee1394/sbp2.h
@@ -335,6 +335,7 @@ enum sbp2lu_state_types {
#define SBP2_WORKAROUND_DELAY_INQUIRY 0x10
#define SBP2_INQUIRY_DELAY 12
#define SBP2_WORKAROUND_POWER_CONDITION 0x20
+#define SBP2_WORKAROUND_NO_PAGE_TABLES 0x40
#define SBP2_WORKAROUND_OVERRIDE 0x100
#endif /* SBP2_H */
--
Stefan Richter
-=====-==--= ---= ==---
http://arcgraph.de/sr/
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/sbp2.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c
+++ linux/drivers/ieee1394/sbp2.c
@@ -1403,8 +1403,7 @@ static void sbp2_parse_unit_directory(st
"(firmware_revision 0x%06x, vendor_id 0x%06x,"
" model_id 0x%06x)",
NODE_BUS_ARGS(ud->ne->host, ud->ne->nodeid),
- workarounds, firmware_revision,
- ud->vendor_id ? ud->vendor_id : ud->ne->vendor_id,
+ workarounds, firmware_revision, ud->vendor_id,
model);
/* We would need one SCSI host template for each target to adjust
--
Stefan Richter
-=====-==--= ---= ==---
http://arcgraph.de/sr/
1394-2008 clause 16.3.4.1 (1394b-2002 clause 16.3.1.1) defines tighter
limits than 1394-2008 clause 6.2.2.3 (1394a-2000 clause 6.2.2.3).
Our previously too large limit doesn't matter though if the controller
reports its max_receive correctly.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/sbp2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c
+++ linux/drivers/ieee1394/sbp2.c
@@ -256,7 +256,7 @@ static int sbp2_set_busy_timeout(struct
static int sbp2_max_speed_and_size(struct sbp2_lu *);
-static const u8 sbp2_speedto_max_payload[] = { 0x7, 0x8, 0x9, 0xA, 0xB, 0xC };
+static const u8 sbp2_speedto_max_payload[] = { 0x7, 0x8, 0x9, 0xa, 0xa, 0xa };
static DEFINE_RWLOCK(sbp2_hi_logical_units_lock);
--
Stefan Richter
-=====-==--= ---= ==---
http://arcgraph.de/sr/
Stefan Richter wrote:
> According to https://bugs.launchpad.net/bugs/294391
> - 3rd generation iPods need the "fix capacity" workaround after all
> (apparently they crash after the last sector was accessed),
Bug update: This workaround alone does not prevent I/O errors later on.
--
Stefan Richter
-=====-==--= ---= ==---
http://arcgraph.de/sr/
1394-2008 clause 16.3.4.1 (1394b-2002 clause 16.3.1.1) defines tighter
limits than 1394-2008 clause 6.2.2.3 (1394a-2000 clause 6.2.2.3).
Our previously too large limit doesn't matter though if the controller
reports its max_receive correctly.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/fw-sbp2.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -168,6 +168,7 @@ struct sbp2_target {
int address_high;
unsigned int workarounds;
unsigned int mgt_orb_timeout;
+ unsigned int max_payload;
int dont_block; /* counter for each logical unit */
int blocked; /* ditto */
@@ -1150,6 +1151,15 @@ static int sbp2_probe(struct device *dev
sbp2_init_workarounds(tgt, model, firmware_revision);
+ /*
+ * At S100 we can do 512 bytes per packet, at S200 1024 bytes,
+ * and so on up to 4096 bytes. The SBP-2 max_payload field
+ * specifies the max payload size as 2 ^ (max_payload + 2), so
+ * if we set this to max_speed + 7, we get the right value.
+ */
+ tgt->max_payload = min(device->max_speed + 7, 10U);
+ tgt->max_payload = min(tgt->max_payload, device->card->max_receive - 1);
+
/* Do the login in a workqueue so we can easily reschedule retries. */
list_for_each_entry(lu, &tgt->lu_list, link)
sbp2_queue_work(lu, DIV_ROUND_UP(HZ, 5));
@@ -1426,7 +1436,6 @@ static int sbp2_scsi_queuecommand(struct
struct sbp2_logical_unit *lu = cmd->device->hostdata;
struct fw_device *device = fw_device(lu->tgt->unit->device.parent);
struct sbp2_command_orb *orb;
- unsigned int max_payload;
int generation, retval = SCSI_MLQUEUE_HOST_BUSY;
/*
@@ -1454,17 +1463,9 @@ static int sbp2_scsi_queuecommand(struct
orb->done = done;
orb->cmd = cmd;
- orb->request.next.high = cpu_to_be32(SBP2_ORB_NULL);
- /*
- * At speed 100 we can do 512 bytes per packet, at speed 200,
- * 1024 bytes per packet etc. The SBP-2 max_payload field
- * specifies the max payload size as 2 ^ (max_payload + 2), so
- * if we set this to max_speed + 7, we get the right value.
- */
- max_payload = min(device->max_speed + 7,
- device->card->max_receive - 1);
+ orb->request.next.high = cpu_to_be32(SBP2_ORB_NULL);
orb->request.misc = cpu_to_be32(
- COMMAND_ORB_MAX_PAYLOAD(max_payload) |
+ COMMAND_ORB_MAX_PAYLOAD(lu->tgt->max_payload) |
COMMAND_ORB_SPEED(device->max_speed) |
COMMAND_ORB_NOTIFY);
--
Stefan Richter
-=====-==--= ---= ==---
http://arcgraph.de/sr/
On Saturday 24 January 2009 13:46:44 Stefan Richter wrote:
> According to https://bugs.launchpad.net/bugs/294391
> - 3rd generation iPods need the "fix capacity" workaround after all
> (apparently they crash after the last sector was accessed),
I've got a 3rd-gen iPod contributed to the testing stash here, but its...
Well, quirky. Was originally a 40G that someone put a 60G drive into,
and it fails to function when hooked to both a Linux box and a Mac OS X
box via FireWire, but works w/both when hooked up via USB...
In other words, its pretty much worthless for FireWire testing.
> - 2nd generation iPods need the "128 kB maximum request size"
> workaround.
I do have a functional 2nd-gen (its Kristian's actually). From what I've
seen so far, it does work just fine w/only the 128k max req workaround
set.
> Furthermore, Kristian H?gsberg mentioned that 2nd generation iPods
> treat page tables (scatter-gather lists) as data buffers, i.e. ignore
> the page_table_present bit of command block ORBs. If this is true, then
> the 128 kB limit is not a sufficient workaround; instead we have to
> guarantee that all requests have only a single s/g element. This could
> be up to 64 kByte - 4 Bytes in size. But I expect the block layer to
> typically emit much smaller elements than that, alas.
So far, the 128k bit does in practice seem to be sufficient. Of course,
I haven't done anything like run iozone against this, let alone tried
shoving a bunch of music onto it...
> Alas both iPod generations feature the same model ID in the config ROM,
> hence we can only define a shared quirks list entry for them. This is
> bad since
> - the "fix capacity" workaround prevents access to the very last
> sector if a device does not actually have this bug,
> - the "no page tables" workaround dramatically reduces throughput,
> e.g. from 25 to 9 MB/s without gap count optimization, and from 36
> 11 MB/s with gap count optimization (as per hdparm with a regular
> 3.5" S400 disk on an i686 PC without IOMMU). Whether this matters
> with the already slow iPod disk is not yet known.
I was able to dd the entire 2nd-gen ipod at 12.5MB/s when using just the
128k workaround (i.e. workarounds=0x101).
# dd if=/dev/sdc of=/dev/null
19531260+0 records in
19531260+0 records out
10000005120 bytes (10 GB) copied, 798.382 s, 12.5 MB/s
> This patch needs testing by somebody who has the hardware:
> - Do 2nd and 3rd gen. iPods actually have a model_id == 0, or do they
> have in fact no model_id at all?
So far as I can decipher (using Kristian's csr-dump utility), they really
do have model_id == 0.
Would it make any sense to try keying off any additional values in the
config rom to better restrict where workarounds are applied?
Of course, there's not much to go on... The only things I can see different
between the 2nd and 3rd gen ones here are the bib header, the guid
(obviously), the unit directory length and crc and the management orb
timeout value. If the orb timeout values are consistent across the same
model, that might be the best shot...
> - Does the "fix capacity" workaround harm usage of 2nd gen. iPods?
Doesn't affect dd'ing the disk, doubtful it'd affect actual usage, unless
someone had a completely filled iPod (workarounds=0x109).
# dd if=/dev/sdc of=/dev/null
19531259+0 records in
19531259+0 records out
10000004608 bytes (10 GB) copied, 811.963 s, 12.3 MB/s
> - Is the "no page tables" workaround really necessary for 2nd gen.
> iPods, or is the 128k limit sufficient as reported for the Ubuntu
> 8.10 kernel?
Looks to me like the no page tables workaround isn't needed. I could
have sworn the defaults w/this patch (workarounds 0x48) was actually
causing dd to fail last night, but I haven't been able to reproduce
that failure, now that I'm trying (maybe I this ipod mixed up with one
of the others in the pile...). It certainly does slaughter performance
though. I halted this dd after getting tired of waiting:
# dd if=/dev/sdc of=/dev/null
^C1139425+0 records in
1139424+0 records out
583385088 bytes (583 MB) copied, 1220.87 s, 478 kB/s
Yeah, lets not do the no page tables workaround. :)
> - How badly does the "no page tables" workaround affect throughput of
> 3rd gen. iPods which don't need it?
Can't tell, since this one doesn't behave via FireWire. Wonder if it
would behave better if I transplanted the 40G drive from a 4th-gen into
it... Where's my sledge hammer?...
--
Jarod Wilson
[email protected]
On Wednesday 28 January 2009 17:18:41 Jarod Wilson wrote:
> > ? - Is the "no page tables" workaround really necessary for 2nd gen.
> > ? ? iPods, or is the 128k limit sufficient as reported for the Ubuntu
> > ? ? 8.10 kernel?
>
> Looks to me like the no page tables workaround isn't needed. I could
> have sworn the defaults w/this patch (workarounds 0x48) was actually
> causing dd to fail last night, but I haven't been able to reproduce
> that failure, now that I'm trying (maybe I this ipod mixed up with one
> of the others in the pile...).
Yep, I'm a dummy. The failure I was thinking of was w/the completely
flaky 3rd-gen iPod, just went back and looked at logs.
So far as I can tell, I'm thinking 128k limit + fix capacity should be
okay for both the 2nd and 3rd-gen iPods, and if someone comes up with
evidence that the 2nd-gen does something badly w/the fix capacity
workaround enabled, then we revisit trying to apply the workarounds
more narrowly somehow.
--
Jarod Wilson
[email protected]
Jarod Wilson wrote:
> On Saturday 24 January 2009 13:46:44 Stefan Richter wrote:
>> - Do 2nd and 3rd gen. iPods actually have a model_id == 0, or do they
>> have in fact no model_id at all?
>
> So far as I can decipher (using Kristian's csr-dump utility), they really
> do have model_id == 0.
If model_id weren't there, the older ieee1394/sbp2 driver would log
model_id 0 (and match against quirks list entries with ID 0), while
firewire-sbp2 logs model_id 0xff000000 (which is an impossible value and
thus indicates to those who want to write a new quirks list entry that
there is actually no model_id...). I will soon push the patch which
makes sbp2 behave like firewire-sbp2 in this regard, to eliminate a
potential source of conflicting user reports.
--
Stefan Richter
-=====-==--= ---= ===-=
http://arcgraph.de/sr/
According to https://bugs.launchpad.net/bugs/294391
- 3rd generation iPods need the "fix capacity" workaround after all
(apparently they crash after the last sector was accessed),
- 2nd generation iPods need the "128 kB maximum request size"
workaround.
Alas both iPod generations feature the same model ID in the config ROM,
hence we can only define a shared quirks list entry for them. Luckily
the fix capacity workaround did not show a negative effect in Jarod's
tests with 2nd gen. iPod.
A side note: Apple computers in target mode (or at least an x86 Mac
mini) don't have firmware_version and model_id, hence none of the iPod
quirks list entries is active for them.
Tested-by: Jarod Wilson <[email protected]>
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/firewire/fw-sbp2.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
Index: linux/drivers/firewire/fw-sbp2.c
===================================================================
--- linux.orig/drivers/firewire/fw-sbp2.c
+++ linux/drivers/firewire/fw-sbp2.c
@@ -360,15 +360,17 @@ static const struct {
.model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_128K_MAX_TRANS,
},
-
/*
- * There are iPods (2nd gen, 3rd gen) with model_id == 0, but
- * these iPods do not feature the read_capacity bug according
- * to one report. Read_capacity behaviour as well as model_id
- * could change due to Apple-supplied firmware updates though.
+ * iPod 2nd generation: needs 128k max transfer size workaround
+ * iPod 3rd generation: needs fix capacity workaround
*/
-
- /* iPod 4th generation. */ {
+ {
+ .firmware_revision = 0x0a2700,
+ .model = 0x000000,
+ .workarounds = SBP2_WORKAROUND_128K_MAX_TRANS |
+ SBP2_WORKAROUND_FIX_CAPACITY,
+ },
+ /* iPod 4th generation */ {
.firmware_revision = 0x0a2700,
.model = 0x000021,
.workarounds = SBP2_WORKAROUND_FIX_CAPACITY,
--
Stefan Richter
-=====-==--= ---= ===-=
http://arcgraph.de/sr/
as per https://bugs.launchpad.net/bugs/294391. These got one sample of
each iPod generation going. However there still occurred I/O stalls
with the 3rd generation iPod which remain undiagnosed at the time of
this writing.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/sbp2.c | 10 ++++++++++
1 file changed, 10 insertions(+)
Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c
+++ linux/drivers/ieee1394/sbp2.c
@@ -395,6 +395,16 @@ static const struct {
.model = SBP2_ROM_VALUE_WILDCARD,
.workarounds = SBP2_WORKAROUND_128K_MAX_TRANS,
},
+ /*
+ * iPod 2nd generation: needs 128k max transfer size workaround
+ * iPod 3rd generation: needs fix capacity workaround
+ */
+ {
+ .firmware_revision = 0x0a2700,
+ .model = 0x000000,
+ .workarounds = SBP2_WORKAROUND_128K_MAX_TRANS |
+ SBP2_WORKAROUND_FIX_CAPACITY,
+ },
/* iPod 4th generation */ {
.firmware_revision = 0x0a2700,
.model = 0x000021,
--
Stefan Richter
-=====-==--= ---= ===-=
http://arcgraph.de/sr/
On Wednesday 28 January 2009 18:11:32 Stefan Richter wrote:
> Jarod Wilson wrote:
> > On Saturday 24 January 2009 13:46:44 Stefan Richter wrote:
> >> - Do 2nd and 3rd gen. iPods actually have a model_id == 0, or do they
> >> have in fact no model_id at all?
> >
> > So far as I can decipher (using Kristian's csr-dump utility), they really
> > do have model_id == 0.
>
> If model_id weren't there, the older ieee1394/sbp2 driver would log
> model_id 0 (and match against quirks list entries with ID 0), while
> firewire-sbp2 logs model_id 0xff000000 (which is an impossible value and
> thus indicates to those who want to write a new quirks list entry that
> there is actually no model_id...). I will soon push the patch which
> makes sbp2 behave like firewire-sbp2 in this regard, to eliminate a
> potential source of conflicting user reports.
Okay, so yeah, they definitely both have model_id == 0.
firewire_sbp2: Workarounds for fw2.0: 0x48 (firmware_revision 0x0a2700,
model_id 0x000000).
On a related note... The fix capacity workaround... I think I need to
observe this failure myself... My own 4th-gen ipod, when connected to a
Mac OS X system, reports the exact capacity I get if the fix capacity
workaround is disabled. Do they just know not to try to access that
last block, or is the need for this workaround a myth? :)
--
Jarod Wilson
[email protected]
On Wednesday 28 January 2009 18:11:59 Stefan Richter wrote:
> According to https://bugs.launchpad.net/bugs/294391
> - 3rd generation iPods need the "fix capacity" workaround after all
> (apparently they crash after the last sector was accessed),
> - 2nd generation iPods need the "128 kB maximum request size"
> workaround.
>
> Alas both iPod generations feature the same model ID in the config ROM,
> hence we can only define a shared quirks list entry for them. Luckily
> the fix capacity workaround did not show a negative effect in Jarod's
> tests with 2nd gen. iPod.
>
> A side note: Apple computers in target mode (or at least an x86 Mac
> mini) don't have firmware_version and model_id, hence none of the iPod
> quirks list entries is active for them.
>
> Tested-by: Jarod Wilson <[email protected]>
> Signed-off-by: Stefan Richter <[email protected]>
Looks golden to me, and I've wedged this into the latest Fedora rawhide
kernels. Go 'head and add this too:
Acked-by: Jarod Wilson <[email protected]>
Thanks much!
--
Jarod Wilson
[email protected]
On Wednesday 28 January 2009 18:13:20 Stefan Richter wrote:
> as per https://bugs.launchpad.net/bugs/294391. These got one sample of
> each iPod generation going. However there still occurred I/O stalls
> with the 3rd generation iPod which remain undiagnosed at the time of
> this writing.
>
> Signed-off-by: Stefan Richter <[email protected]>
Same workarounds do the trick on the new stack, should be good to go
here too.
Acked-by: Jarod Wilson <[email protected]>
--
Jarod Wilson
[email protected]
(adding Cc: Alan Stern just in case, because he knows much more about
all this)
Jarod Wilson wrote:
> On a related note... The fix capacity workaround... I think I need to
> observe this failure myself... My own 4th-gen ipod, when connected to a
> Mac OS X system, reports the exact capacity I get if the fix capacity
> workaround is disabled. Do they just know not to try to access that
> last block, or is the need for this workaround a myth? :)
Well, before the report [1] from the Ubuntu of a what appeared to be a
regression of Ubuntu 8.10 vs. its parent or grandparent release, we
thought we knew that iPod 2nd + 3rd gen were _not_ affected, because
long ago an owner of these iPods told us so.
Obviously they changed something in Ubuntu 8.10 which suddenly made 3rd
gen iPods vulnerable too. If the report is accurate, then this is /not/
the READ CAPACITY 10 issue where device's capacity report is off by one.
(The reported capacity without workaround is an even number and thus
possibly correct.) Rather, this is apparently the kind of the bug where
an access which includes the last sector corrupts some state in the
firmware, causing the firmware to error out on subsequent accesses. The
actual IO errors in the reporter's log happen at low LBAs, not at LBAs
at the end.
Anyway; to our moderate surprise after years of successful usage of
these devices under Linux, we learn that a current Linux suddenly needs
a quirk fix for them. IMO they always needed it but they just weren't
exposed to the dangerous access patterns, until userland or the kernel
was changed in an as yet unidentified respect.
Back to the question whether the 4th gen iPod really needs it. Well,
I'm quite sure that we added it because it was proven to be necessary.
At least that's what I remember about it; if we really wanted to we
could probably find evidence in list archives. I don't remember which
particular type of last sector bug it was in case of these newer iPod
models. Coincidentally, those dual interface iPods also need the
workaround when used through usb-storage; see
drivers/usb/storage/unusual_devs.h.
Now, what does OS X do with those iPods? I don't know. Maybe they have
quirks lists like we do. There could be something to be found in the
Darwin sources. But I consider it more likely that OS X simply does not
do the kind of accesses that Linux does which tend to crash all those
crap firmwares left and right.
Evidently, at least popular Windows incarnations don't do such accesses,
at least not in normal usage. Otherwise this class of firmware bugs
could simply not exist in mass market devices.
[1] https://bugs.launchpad.net/bugs/294391
--
Stefan Richter
-=====-==--= ---= ===-=
http://arcgraph.de/sr/
On Thu, 29 Jan 2009, Stefan Richter wrote:
> (adding Cc: Alan Stern just in case, because he knows much more about
> all this)
>
> Jarod Wilson wrote:
> > On a related note... The fix capacity workaround... I think I need to
> > observe this failure myself... My own 4th-gen ipod, when connected to a
> > Mac OS X system, reports the exact capacity I get if the fix capacity
> > workaround is disabled. Do they just know not to try to access that
> > last block, or is the need for this workaround a myth? :)
>
> Well, before the report [1] from the Ubuntu of a what appeared to be a
> regression of Ubuntu 8.10 vs. its parent or grandparent release, we
> thought we knew that iPod 2nd + 3rd gen were _not_ affected, because
> long ago an owner of these iPods told us so.
>
> Obviously they changed something in Ubuntu 8.10 which suddenly made 3rd
> gen iPods vulnerable too. If the report is accurate, then this is /not/
> the READ CAPACITY 10 issue where device's capacity report is off by one.
> (The reported capacity without workaround is an even number and thus
> possibly correct.)
Don't make that assumption. One of the small number of devices which
really did have an odd number of sectors was an early iPod.
> Rather, this is apparently the kind of the bug where
> an access which includes the last sector corrupts some state in the
> firmware, causing the firmware to error out on subsequent accesses. The
> actual IO errors in the reporter's log happen at low LBAs, not at LBAs
> at the end.
Are you aware of the last_sector_bug flag? It prevents sectors near
the end of the device from being accessed using anything other than
single-sector reads or writes. Some devices don't like it if, for
example, you do a 2-sector read that includes the last sector.
> Now, what does OS X do with those iPods? I don't know. Maybe they have
> quirks lists like we do. There could be something to be found in the
> Darwin sources. But I consider it more likely that OS X simply does not
> do the kind of accesses that Linux does which tend to crash all those
> crap firmwares left and right.
>
> Evidently, at least popular Windows incarnations don't do such accesses,
> at least not in normal usage. Otherwise this class of firmware bugs
> could simply not exist in mass market devices.
Linux uses the last-sector accesses to check whether or not the device
is part of a RAID. Apparently Windows and OS X don't support the kinds
of RAID that need this.
Alan Stern
Alan Stern wrote:
> On Thu, 29 Jan 2009, Stefan Richter wrote:
>> Obviously they changed something in Ubuntu 8.10 which suddenly made 3rd
>> gen iPods vulnerable too. If the report is accurate, then this is /not/
>> the READ CAPACITY 10 issue where device's capacity report is off by one.
>> (The reported capacity without workaround is an even number and thus
>> possibly correct.)
>
> Don't make that assumption. One of the small number of devices which
> really did have an odd number of sectors was an early iPod.
Ah, interesting.
(Jarod, didn't you see OS X report the same size on that 3rd gen. iPod
as Linux without quirk flag, or did I dream this? In any case this
could of course simply mean that OS X uses a wrongly reported size...)
>> Rather, this is apparently the kind of the bug where
>> an access which includes the last sector corrupts some state in the
>> firmware, causing the firmware to error out on subsequent accesses. The
>> actual IO errors in the reporter's log happen at low LBAs, not at LBAs
>> at the end.
>
> Are you aware of the last_sector_bug flag? It prevents sectors near
> the end of the device from being accessed using anything other than
> single-sector reads or writes. Some devices don't like it if, for
> example, you do a 2-sector read that includes the last sector.
Now that you mention it I do remember. I would need to ask the reporter
of the Ubuntu bug who has a fully working 3rd gen iPod (unlike Jarod's
3rd gen iPod which doesn't work reliably anymore) to compile a kernel
from patched sources if I really wanted to investigate whether the
last_sector_bug flag fixes the issue too. The fix_capacity flag can be
easily enabled for FireWire disks by users of binary kernel packages at
runtime, so that's what I had the reporter try.
Hmm, I'm beginning to suspect that I should duplicate usb-storage's
if (sdev->type == TYPE_DISK)
sdev->last_sector_bug = 1;
into the FireWire drivers. There aren't any fundamental downsides to
this, are there?
>> Now, what does OS X do with those iPods? I don't know. Maybe they have
>> quirks lists like we do. There could be something to be found in the
>> Darwin sources. But I consider it more likely that OS X simply does not
>> do the kind of accesses that Linux does which tend to crash all those
>> crap firmwares left and right.
>>
>> Evidently, at least popular Windows incarnations don't do such accesses,
>> at least not in normal usage. Otherwise this class of firmware bugs
>> could simply not exist in mass market devices.
>
> Linux uses the last-sector accesses to check whether or not the device
> is part of a RAID. Apparently Windows and OS X don't support the kinds
> of RAID that need this.
Or they don't support these kinds of RAID over certain transports like
USB and FireWire...?
--
Stefan Richter
-=====-==--= ---= ===-=
http://arcgraph.de/sr/
On Thu, 29 Jan 2009, Stefan Richter wrote:
> Hmm, I'm beginning to suspect that I should duplicate usb-storage's
>
> if (sdev->type == TYPE_DISK)
> sdev->last_sector_bug = 1;
>
> into the FireWire drivers. There aren't any fundamental downsides to
> this, are there?
No, not really. Accesses to the last few sectors will be slightly
slower, but those sectors don't get used much anyway and the slowdown
isn't very big. In theory there might be a device which doesn't like
single-sector accesses near the end... but what can you do about
something like that? If the device doesn't let you read sectors one at
a time, it's not worth worrying about.
> > Linux uses the last-sector accesses to check whether or not the device
> > is part of a RAID. Apparently Windows and OS X don't support the kinds
> > of RAID that need this.
>
> Or they don't support these kinds of RAID over certain transports like
> USB and FireWire...?
Maybe. I don't know any of the details.
Alan Stern
On Thursday 29 January 2009 16:28:57 Stefan Richter wrote:
> Alan Stern wrote:
> > On Thu, 29 Jan 2009, Stefan Richter wrote:
> >> Obviously they changed something in Ubuntu 8.10 which suddenly made 3rd
> >> gen iPods vulnerable too. If the report is accurate, then this is /not/
> >> the READ CAPACITY 10 issue where device's capacity report is off by one.
> >> (The reported capacity without workaround is an even number and thus
> >> possibly correct.)
> >
> > Don't make that assumption. One of the small number of devices which
> > really did have an odd number of sectors was an early iPod.
>
> Ah, interesting.
>
> (Jarod, didn't you see OS X report the same size on that 3rd gen. iPod
> as Linux without quirk flag, or did I dream this? In any case this
> could of course simply mean that OS X uses a wrongly reported size...)
I don't think I looked at the 3rd-gen iPod's reported size under OS X,
the place I saw OS X size matching Linux size sans-quirk-flag was with
a 20GB 4th-gen iPod. I can certainly check the 3rd-gen too though.
--
Jarod Wilson
[email protected]