I've written these two patches when I was searching why my old iPod
cannot be mounted on my PC.
During my investigations, I've compared the fw0 config_rom I can access from
a powermac and the config_rom I have on a PC. It appears that the big-endian
property of the firewire bus is not respected here.
Then I was able to see that the 5 first words of the iPod config_rom were
read correctly and the followings not and fail on an ack timeout.
In fact, after the 5 first words, the max speed of the device is changed
from the lowest value to the real max speed of the device, and this does
not work with my iPod. I don't know why, I suspect an incompatibility between
my firewire card and the iPod. As I'm not an expert, the only solution
I found is to allow the user to force the max device speed at firewire-core
level (in my case, force_speed is 0 -> FW100)
[PATCH 1/2] firewire: firewire is a big-endian bus
[PATCH 2/2] firewire: add a parameter to force the speed of the
So, dump config_rom data as big-endian values.
The value given by /sys/bus/firewire/devices/fw0 were correctly
given on a big-endian host (like powermac) not on a little-endian host
(like PC), for instance:
00000000 87 a4 04 04 34 39 33 31 22 a2 00 f0 33 22 11 00 |....4931"...3"..|
00000010 66 66 66 33 0b dd 05 00 c0 83 00 0c 1e 0d d0 03 |fff3............|
00000020 03 00 00 81 01 00 00 17 08 00 00 81 b7 4c 06 00 |.............L..|
00000030 00 00 00 00 00 00 00 00 75 6e 69 4c 69 46 20 78 |........uniLiF x|
00000040 69 77 65 72 00 00 65 72 1c ff 03 00 00 00 00 00 |iwer..er........|
00000050 00 00 00 00 75 6a 75 4a |....ujuJ|
00000058
instead of:
00000000 04 04 a4 87 31 33 39 34 f0 00 a2 22 00 11 22 33 |....1394...".."3|
00000010 33 66 66 66 00 05 dd 0b 0c 00 83 c0 03 d0 0d 1e |3fff............|
00000020 81 00 00 03 17 00 00 01 81 00 00 08 00 06 4c b7 |..............L.|
00000030 00 00 00 00 00 00 00 00 4c 69 6e 75 78 20 46 69 |........Linux Fi|
00000040 72 65 77 69 72 65 00 00 00 03 ff 1c 00 00 00 00 |rewire..........|
00000050 00 00 00 00 4a 75 6a 75 |....Juju|
00000058
This patch corrects this.
Signed-off-by: Laurent Vivier <[email protected]>
---
drivers/firewire/core-device.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index f9e3aee..5245567 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -399,14 +399,14 @@ static ssize_t config_rom_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct fw_device *device = fw_device(dev);
- size_t length;
+ size_t i;
down_read(&fw_device_rwsem);
- length = device->config_rom_length * 4;
- memcpy(buf, device->config_rom, length);
+ for (i = 0; i < device->config_rom_length; i++)
+ ((u32 *)buf)[i] = be32_to_cpu(device->config_rom[i]);
up_read(&fw_device_rwsem);
- return length;
+ return i * 4;
}
static ssize_t guid_show(struct device *dev,
--
1.9.1
I was trying to use my old iPod mini firewire (first generation) with
a new firewire card I put in my PC (VIA Technologies, Inc. VT6306/7/8),
but the iPod was not mounted and failed with the following error:
reading config rom failed: no ack
It appears that the configuration rom cannot be read after the
device max speed is set to something else than SCODE_100.
According to the iPod configuration ROM, it should support SCODE_400.
This patch adds a a parameter (force_speed) to the firewire-core module
to be able to set the max speed to use with the firewire devices.
Signed-off-by: Laurent Vivier <[email protected]>
---
drivers/firewire/core-device.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 5245567..a075827 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -44,6 +44,17 @@
#include "core.h"
+static int force_speed = -1;
+module_param_named(force_speed, force_speed, int, 0644);
+MODULE_PARM_DESC(force_speed, "Force device speed (default = -1"
+ ", FW100 = " __stringify(SCODE_100)
+ ", FW200 = " __stringify(SCODE_200)
+ ", FW400 = " __stringify(SCODE_400)
+ ", FW800 = " __stringify(SCODE_800)
+ ", FW1600 = " __stringify(SCODE_1600)
+ ", FW3200 = " __stringify(SCODE_3200)
+ ", FWBETA = " __stringify(SCODE_BETA));
+
void fw_csr_iterator_init(struct fw_csr_iterator *ci, const u32 *p)
{
ci->p = p + 1;
@@ -555,6 +566,8 @@ static int read_config_rom(struct fw_device *device, int generation)
}
device->max_speed = device->node->max_speed;
+ if (force_speed != -1)
+ device->max_speed = force_speed & 0xf;
/*
* Determine the speed of
--
1.9.1
On Tue, 2015-04-21 at 02:36 +0200, Laurent Vivier wrote:
> So, dump config_rom data as big-endian values.
>
> The value given by /sys/bus/firewire/devices/fw0 were correctly
> given on a big-endian host (like powermac) not on a little-endian host
> (like PC), for instance:
[]
> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
[]
> @@ -399,14 +399,14 @@ static ssize_t config_rom_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct fw_device *device = fw_device(dev);
> - size_t length;
> + size_t i;
>
> down_read(&fw_device_rwsem);
> - length = device->config_rom_length * 4;
> - memcpy(buf, device->config_rom, length);
> + for (i = 0; i < device->config_rom_length; i++)
> + ((u32 *)buf)[i] = be32_to_cpu(device->config_rom[i]);
Is buf guaranteed to be appropriately aligned on a u32?
On Apr 21 Laurent Vivier wrote:
> So, dump config_rom data as big-endian values.
>
> The value given by /sys/bus/firewire/devices/fw0 were correctly
> given on a big-endian host (like powermac) not on a little-endian host
> (like PC), for instance:
>
> 00000000 87 a4 04 04 34 39 33 31 22 a2 00 f0 33 22 11 00 |....4931"...3"..|
> 00000010 66 66 66 33 0b dd 05 00 c0 83 00 0c 1e 0d d0 03 |fff3............|
> 00000020 03 00 00 81 01 00 00 17 08 00 00 81 b7 4c 06 00 |.............L..|
> 00000030 00 00 00 00 00 00 00 00 75 6e 69 4c 69 46 20 78 |........uniLiF x|
> 00000040 69 77 65 72 00 00 65 72 1c ff 03 00 00 00 00 00 |iwer..er........|
> 00000050 00 00 00 00 75 6a 75 4a |....ujuJ|
> 00000058
>
> instead of:
>
> 00000000 04 04 a4 87 31 33 39 34 f0 00 a2 22 00 11 22 33 |....1394...".."3|
> 00000010 33 66 66 66 00 05 dd 0b 0c 00 83 c0 03 d0 0d 1e |3fff............|
> 00000020 81 00 00 03 17 00 00 01 81 00 00 08 00 06 4c b7 |..............L.|
> 00000030 00 00 00 00 00 00 00 00 4c 69 6e 75 78 20 46 69 |........Linux Fi|
> 00000040 72 65 77 69 72 65 00 00 00 03 ff 1c 00 00 00 00 |rewire..........|
> 00000050 00 00 00 00 4a 75 6a 75 |....Juju|
> 00000058
>
> This patch corrects this.
>
> Signed-off-by: Laurent Vivier <[email protected]>
As defined in Documentation/ABI/stable/sysfs-bus-firewire we deliberately
export the Configuration ROM as an array of host-endian quadlets. As a
stable kernel ABI, this will not be changed.
(A python script called crpp which transforms this kind of binary data into
a richly annotated hexdump is available as part of the jujuutils package,
or standalone from http://me.in-berlin.de/~s5r6/linux1394/utils/.)
> ---
> drivers/firewire/core-device.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
> index f9e3aee..5245567 100644
> --- a/drivers/firewire/core-device.c
> +++ b/drivers/firewire/core-device.c
> @@ -399,14 +399,14 @@ static ssize_t config_rom_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct fw_device *device = fw_device(dev);
> - size_t length;
> + size_t i;
>
> down_read(&fw_device_rwsem);
> - length = device->config_rom_length * 4;
> - memcpy(buf, device->config_rom, length);
> + for (i = 0; i < device->config_rom_length; i++)
> + ((u32 *)buf)[i] = be32_to_cpu(device->config_rom[i]);
> up_read(&fw_device_rwsem);
>
> - return length;
> + return i * 4;
> }
>
> static ssize_t guid_show(struct device *dev,
--
Stefan Richter
-=====-===== -=-- =-=-=
http://arcgraph.de/sr/
Laurent Vivier wrote:
> I was trying to use my old iPod mini firewire (first generation) with
> a new firewire card I put in my PC (VIA Technologies, Inc. VT6306/7/8),
> but the iPod was not mounted and failed with the following error:
> reading config rom failed: no ack
> It appears that the configuration rom cannot be read after the
> device max speed is set to something else than SCODE_100.
>
> According to the iPod configuration ROM, it should support SCODE_400.
>
> This patch adds a a parameter (force_speed) to the firewire-core module
> to be able to set the max speed to use with the firewire devices.
Why a module parameter? Why not automatically apply this workaround to
this specific device?
Regards,
Clemens
On Apr 20 Joe Perches wrote:
> On Tue, 2015-04-21 at 02:36 +0200, Laurent Vivier wrote:
> > So, dump config_rom data as big-endian values.
> >
> > The value given by /sys/bus/firewire/devices/fw0 were correctly
> > given on a big-endian host (like powermac) not on a little-endian host
> > (like PC), for instance:
> []
> > diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
> []
> > @@ -399,14 +399,14 @@ static ssize_t config_rom_show(struct device *dev,
> > struct device_attribute *attr, char *buf)
> > {
> > struct fw_device *device = fw_device(dev);
> > - size_t length;
> > + size_t i;
> >
> > down_read(&fw_device_rwsem);
> > - length = device->config_rom_length * 4;
> > - memcpy(buf, device->config_rom, length);
> > + for (i = 0; i < device->config_rom_length; i++)
> > + ((u32 *)buf)[i] = be32_to_cpu(device->config_rom[i]);
>
> Is buf guaranteed to be appropriately aligned on a u32?
Good catch. Though as far as I know, the buffer is page-sized and thereby
certainly page-aligned. This could be looked up in the sysfs core code.
BTW, theoretically speaking it should be cpu_to_be32() here, and the buffer
pointer should be annotated as a big endian pointer. (But as I noted in
the previous reply, the patch will not be applied either way.)
--
Stefan Richter
-=====-===== -=-- =-=-=
http://arcgraph.de/sr/
On Apr 21 Clemens Ladisch wrote:
> Laurent Vivier wrote:
> > I was trying to use my old iPod mini firewire (first generation) with
> > a new firewire card I put in my PC (VIA Technologies, Inc. VT6306/7/8),
> > but the iPod was not mounted and failed with the following error:
> > reading config rom failed: no ack
> > It appears that the configuration rom cannot be read after the
> > device max speed is set to something else than SCODE_100.
> >
> > According to the iPod configuration ROM, it should support SCODE_400.
> >
> > This patch adds a a parameter (force_speed) to the firewire-core module
> > to be able to set the max speed to use with the firewire devices.
>
> Why a module parameter? Why not automatically apply this workaround to
> this specific device?
I doubt that this iPod model needs the workaround. I suspect the
particular hardware combination needs one.
--
Stefan Richter
-=====-===== -=-- =-=-=
http://arcgraph.de/sr/
On Apr 21 Laurent Vivier wrote:
> I was trying to use my old iPod mini firewire (first generation) with
> a new firewire card I put in my PC (VIA Technologies, Inc. VT6306/7/8),
> but the iPod was not mounted and failed with the following error:
> reading config rom failed: no ack
> It appears that the configuration rom cannot be read after the
> device max speed is set to something else than SCODE_100.
Does the card have an internal power connector? This is usually a
4-pin molex or 4-pin floppy power socket. (And if yes, is it directly
connected to the PC's power supply unit?) I am asking because 1394 bus
power is unreliable if drawn from a PCI slot or PCIe slot, and all kinds
of malfunctions of bus powered 1394 devices have been observed on PCI/PCIe
1394 cards without dedicated power input.
> According to the iPod configuration ROM, it should support SCODE_400.
>
> This patch adds a a parameter (force_speed) to the firewire-core module
> to be able to set the max speed to use with the firewire devices.
>
> Signed-off-by: Laurent Vivier <[email protected]>
> ---
> drivers/firewire/core-device.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
> index 5245567..a075827 100644
> --- a/drivers/firewire/core-device.c
> +++ b/drivers/firewire/core-device.c
> @@ -44,6 +44,17 @@
>
> #include "core.h"
>
> +static int force_speed = -1;
> +module_param_named(force_speed, force_speed, int, 0644);
> +MODULE_PARM_DESC(force_speed, "Force device speed (default = -1"
> + ", FW100 = " __stringify(SCODE_100)
> + ", FW200 = " __stringify(SCODE_200)
> + ", FW400 = " __stringify(SCODE_400)
> + ", FW800 = " __stringify(SCODE_800)
> + ", FW1600 = " __stringify(SCODE_1600)
> + ", FW3200 = " __stringify(SCODE_3200)
> + ", FWBETA = " __stringify(SCODE_BETA));
> +
> void fw_csr_iterator_init(struct fw_csr_iterator *ci, const u32 *p)
> {
> ci->p = p + 1;
> @@ -555,6 +566,8 @@ static int read_config_rom(struct fw_device *device, int generation)
> }
>
> device->max_speed = device->node->max_speed;
> + if (force_speed != -1)
> + device->max_speed = force_speed & 0xf;
>
> /*
> * Determine the speed of
The parameter looks interesting, but in this form offers a few surprises
to unsuspecting users.
IMO there should be stricter input validation, perhaps like so:
int user_speed = ACCESS_ONCE(force_speed);
if (user_speed >= SCODE_100 && user_speed <= SCODE_3200)
device->max_speed = user_speed;
Second, I wonder whether it is wise to accept speeds greater than the
local node's and remote node's hardware supports. (And greater than
repeater nodes between local and remote node, but in that case the only
harm done is that all requests will fail.) At least we would have to audit
a few places in our drivers which directly or indirectly depend on the
transmission speed.
Third, SCODE_800 and SCODE_BETA expand to equal values. This does not
make a lot of sense within the module parameter, hence SCODE_BETA should
be left out --- or it should be represented by a different value and its
semantics should be made clear.
Fourth, right after your patch sets the user-specified speed,
firewire-core proceeds to modify the speed based on a probing loop, if
certain conditions are met. Maybe this speed probe should be skipped if
the user selected a desired speed. Or there should be a dedicated
parameter value which means "firewire-core, please always perform your
built-in speed probe".
--
Stefan Richter
-=====-===== -=-- =-=-=
http://arcgraph.de/sr/
Le 21/04/2015 15:13, Stefan Richter a ?crit :
> On Apr 21 Laurent Vivier wrote:
>> So, dump config_rom data as big-endian values.
>>
>> The value given by /sys/bus/firewire/devices/fw0 were correctly
>> given on a big-endian host (like powermac) not on a little-endian host
>> (like PC), for instance:
>>
>> 00000000 87 a4 04 04 34 39 33 31 22 a2 00 f0 33 22 11 00 |....4931"...3"..|
>> 00000010 66 66 66 33 0b dd 05 00 c0 83 00 0c 1e 0d d0 03 |fff3............|
>> 00000020 03 00 00 81 01 00 00 17 08 00 00 81 b7 4c 06 00 |.............L..|
>> 00000030 00 00 00 00 00 00 00 00 75 6e 69 4c 69 46 20 78 |........uniLiF x|
>> 00000040 69 77 65 72 00 00 65 72 1c ff 03 00 00 00 00 00 |iwer..er........|
>> 00000050 00 00 00 00 75 6a 75 4a |....ujuJ|
>> 00000058
>>
>> instead of:
>>
>> 00000000 04 04 a4 87 31 33 39 34 f0 00 a2 22 00 11 22 33 |....1394...".."3|
>> 00000010 33 66 66 66 00 05 dd 0b 0c 00 83 c0 03 d0 0d 1e |3fff............|
>> 00000020 81 00 00 03 17 00 00 01 81 00 00 08 00 06 4c b7 |..............L.|
>> 00000030 00 00 00 00 00 00 00 00 4c 69 6e 75 78 20 46 69 |........Linux Fi|
>> 00000040 72 65 77 69 72 65 00 00 00 03 ff 1c 00 00 00 00 |rewire..........|
>> 00000050 00 00 00 00 4a 75 6a 75 |....Juju|
>> 00000058
>>
>> This patch corrects this.
>>
>> Signed-off-by: Laurent Vivier <[email protected]>
> As defined in Documentation/ABI/stable/sysfs-bus-firewire we deliberately
> export the Configuration ROM as an array of host-endian quadlets. As a
> stable kernel ABI, this will not be changed.
OK, I understand.
>
> (A python script called crpp which transforms this kind of binary data into
> a richly annotated hexdump is available as part of the jujuutils package,
> or standalone from http://me.in-berlin.de/~s5r6/linux1394/utils/.)
Thank you for the information.
Regards,
Laurent
>
>> ---
>> drivers/firewire/core-device.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
>> index f9e3aee..5245567 100644
>> --- a/drivers/firewire/core-device.c
>> +++ b/drivers/firewire/core-device.c
>> @@ -399,14 +399,14 @@ static ssize_t config_rom_show(struct device *dev,
>> struct device_attribute *attr, char *buf)
>> {
>> struct fw_device *device = fw_device(dev);
>> - size_t length;
>> + size_t i;
>>
>> down_read(&fw_device_rwsem);
>> - length = device->config_rom_length * 4;
>> - memcpy(buf, device->config_rom, length);
>> + for (i = 0; i < device->config_rom_length; i++)
>> + ((u32 *)buf)[i] = be32_to_cpu(device->config_rom[i]);
>> up_read(&fw_device_rwsem);
>>
>> - return length;
>> + return i * 4;
>> }
>>
>> static ssize_t guid_show(struct device *dev,
>
>
Le 21/04/2015 16:33, Stefan Richter a ?crit :
> On Apr 21 Laurent Vivier wrote:
>> I was trying to use my old iPod mini firewire (first generation) with
>> a new firewire card I put in my PC (VIA Technologies, Inc. VT6306/7/8),
>> but the iPod was not mounted and failed with the following error:
>> reading config rom failed: no ack
>> It appears that the configuration rom cannot be read after the
>> device max speed is set to something else than SCODE_100.
> Does the card have an internal power connector? This is usually a
> 4-pin molex or 4-pin floppy power socket. (And if yes, is it directly
> connected to the PC's power supply unit?) I am asking because 1394 bus
> power is unreliable if drawn from a PCI slot or PCIe slot, and all kinds
> of malfunctions of bus powered 1394 devices have been observed on PCI/PCIe
> 1394 cards without dedicated power input.
No, there is no power socket on the card (nor user's manual), for a 5
euro card it's not a surprise...
http://www.amazon.fr/gp/product/B00AZEQEM4
>> According to the iPod configuration ROM, it should support SCODE_400.
>>
>> This patch adds a a parameter (force_speed) to the firewire-core module
>> to be able to set the max speed to use with the firewire devices.
>>
>> Signed-off-by: Laurent Vivier <[email protected]>
>> ---
>> drivers/firewire/core-device.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
>> index 5245567..a075827 100644
>> --- a/drivers/firewire/core-device.c
>> +++ b/drivers/firewire/core-device.c
>> @@ -44,6 +44,17 @@
>>
>> #include "core.h"
>>
>> +static int force_speed = -1;
>> +module_param_named(force_speed, force_speed, int, 0644);
>> +MODULE_PARM_DESC(force_speed, "Force device speed (default = -1"
>> + ", FW100 = " __stringify(SCODE_100)
>> + ", FW200 = " __stringify(SCODE_200)
>> + ", FW400 = " __stringify(SCODE_400)
>> + ", FW800 = " __stringify(SCODE_800)
>> + ", FW1600 = " __stringify(SCODE_1600)
>> + ", FW3200 = " __stringify(SCODE_3200)
>> + ", FWBETA = " __stringify(SCODE_BETA));
>> +
>> void fw_csr_iterator_init(struct fw_csr_iterator *ci, const u32 *p)
>> {
>> ci->p = p + 1;
>> @@ -555,6 +566,8 @@ static int read_config_rom(struct fw_device *device, int generation)
>> }
>>
>> device->max_speed = device->node->max_speed;
>> + if (force_speed != -1)
>> + device->max_speed = force_speed & 0xf;
>>
>> /*
>> * Determine the speed of
> The parameter looks interesting, but in this form offers a few surprises
> to unsuspecting users.
>
> IMO there should be stricter input validation, perhaps like so:
>
> int user_speed = ACCESS_ONCE(force_speed);
>
> if (user_speed >= SCODE_100 && user_speed <= SCODE_3200)
> device->max_speed = user_speed;
>
> Second, I wonder whether it is wise to accept speeds greater than the
> local node's and remote node's hardware supports. (And greater than
> repeater nodes between local and remote node, but in that case the only
> harm done is that all requests will fail.) At least we would have to audit
> a few places in our drivers which directly or indirectly depend on the
> transmission speed.
>
> Third, SCODE_800 and SCODE_BETA expand to equal values. This does not
> make a lot of sense within the module parameter, hence SCODE_BETA should
> be left out --- or it should be represented by a different value and its
> semantics should be made clear.
>
> Fourth, right after your patch sets the user-specified speed,
> firewire-core proceeds to modify the speed based on a probing loop, if
> certain conditions are met. Maybe this speed probe should be skipped if
> the user selected a desired speed. Or there should be a dedicated
> parameter value which means "firewire-core, please always perform your
> built-in speed probe".
OK, I will rework this one and forgot the other.
Thank you for your comments.
Regards,
Laurent