* [PATCH 0/2] Some firewire minor patches @ 2015-04-21 0:36 Laurent Vivier 2015-04-21 0:36 ` [PATCH 1/2] firewire: firewire is a big-endian bus Laurent Vivier 2015-04-21 0:36 ` [PATCH 2/2] firewire: add a parameter to force the speed of the devices Laurent Vivier 0 siblings, 2 replies; 11+ messages in thread From: Laurent Vivier @ 2015-04-21 0:36 UTC (permalink / raw) To: Stefan Richter; +Cc: linux1394-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] firewire: firewire is a big-endian bus 2015-04-21 0:36 [PATCH 0/2] Some firewire minor patches Laurent Vivier @ 2015-04-21 0:36 ` Laurent Vivier 2015-04-21 1:04 ` Joe Perches 2015-04-21 13:13 ` Stefan Richter 2015-04-21 0:36 ` [PATCH 2/2] firewire: add a parameter to force the speed of the devices Laurent Vivier 1 sibling, 2 replies; 11+ messages in thread From: Laurent Vivier @ 2015-04-21 0:36 UTC (permalink / raw) To: Stefan Richter; +Cc: linux1394-devel, linux-kernel, Laurent Vivier 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 <laurent@vivier.eu> --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] firewire: firewire is a big-endian bus 2015-04-21 0:36 ` [PATCH 1/2] firewire: firewire is a big-endian bus Laurent Vivier @ 2015-04-21 1:04 ` Joe Perches 2015-04-21 14:13 ` Stefan Richter 2015-04-21 13:13 ` Stefan Richter 1 sibling, 1 reply; 11+ messages in thread From: Joe Perches @ 2015-04-21 1:04 UTC (permalink / raw) To: Laurent Vivier; +Cc: Stefan Richter, linux1394-devel, linux-kernel 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] firewire: firewire is a big-endian bus 2015-04-21 1:04 ` Joe Perches @ 2015-04-21 14:13 ` Stefan Richter 0 siblings, 0 replies; 11+ messages in thread From: Stefan Richter @ 2015-04-21 14:13 UTC (permalink / raw) To: Joe Perches; +Cc: Laurent Vivier, linux1394-devel, linux-kernel 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/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] firewire: firewire is a big-endian bus 2015-04-21 0:36 ` [PATCH 1/2] firewire: firewire is a big-endian bus Laurent Vivier 2015-04-21 1:04 ` Joe Perches @ 2015-04-21 13:13 ` Stefan Richter 2015-04-21 16:04 ` Laurent Vivier 1 sibling, 1 reply; 11+ messages in thread From: Stefan Richter @ 2015-04-21 13:13 UTC (permalink / raw) To: Laurent Vivier; +Cc: linux1394-devel, linux-kernel, Joe Perches 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 <laurent@vivier.eu> 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/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] firewire: firewire is a big-endian bus 2015-04-21 13:13 ` Stefan Richter @ 2015-04-21 16:04 ` Laurent Vivier 0 siblings, 0 replies; 11+ messages in thread From: Laurent Vivier @ 2015-04-21 16:04 UTC (permalink / raw) To: Stefan Richter; +Cc: linux1394-devel, linux-kernel, Joe Perches [-- Attachment #1: Type: text/plain, Size: 2879 bytes --] 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 <laurent@vivier.eu> > 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, > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] firewire: add a parameter to force the speed of the devices. 2015-04-21 0:36 [PATCH 0/2] Some firewire minor patches Laurent Vivier 2015-04-21 0:36 ` [PATCH 1/2] firewire: firewire is a big-endian bus Laurent Vivier @ 2015-04-21 0:36 ` Laurent Vivier 2015-04-21 14:03 ` Clemens Ladisch 2015-04-21 14:33 ` Stefan Richter 1 sibling, 2 replies; 11+ messages in thread From: Laurent Vivier @ 2015-04-21 0:36 UTC (permalink / raw) To: Stefan Richter; +Cc: linux1394-devel, linux-kernel, Laurent Vivier 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 <laurent@vivier.eu> --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] firewire: add a parameter to force the speed of the devices. 2015-04-21 0:36 ` [PATCH 2/2] firewire: add a parameter to force the speed of the devices Laurent Vivier @ 2015-04-21 14:03 ` Clemens Ladisch 2015-04-21 14:16 ` Stefan Richter 2015-04-21 14:33 ` Stefan Richter 1 sibling, 1 reply; 11+ messages in thread From: Clemens Ladisch @ 2015-04-21 14:03 UTC (permalink / raw) To: Laurent Vivier, Stefan Richter; +Cc: linux1394-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] firewire: add a parameter to force the speed of the devices. 2015-04-21 14:03 ` Clemens Ladisch @ 2015-04-21 14:16 ` Stefan Richter 0 siblings, 0 replies; 11+ messages in thread From: Stefan Richter @ 2015-04-21 14:16 UTC (permalink / raw) To: Clemens Ladisch; +Cc: Laurent Vivier, linux1394-devel, linux-kernel 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/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] firewire: add a parameter to force the speed of the devices. 2015-04-21 0:36 ` [PATCH 2/2] firewire: add a parameter to force the speed of the devices Laurent Vivier 2015-04-21 14:03 ` Clemens Ladisch @ 2015-04-21 14:33 ` Stefan Richter 2015-04-21 16:55 ` Laurent Vivier 1 sibling, 1 reply; 11+ messages in thread From: Stefan Richter @ 2015-04-21 14:33 UTC (permalink / raw) To: Laurent Vivier; +Cc: linux1394-devel, linux-kernel 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 <laurent@vivier.eu> > --- > 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/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] firewire: add a parameter to force the speed of the devices. 2015-04-21 14:33 ` Stefan Richter @ 2015-04-21 16:55 ` Laurent Vivier 0 siblings, 0 replies; 11+ messages in thread From: Laurent Vivier @ 2015-04-21 16:55 UTC (permalink / raw) To: Stefan Richter; +Cc: linux1394-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4018 bytes --] 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 <laurent@vivier.eu> >> --- >> 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-04-21 16:55 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-21 0:36 [PATCH 0/2] Some firewire minor patches Laurent Vivier 2015-04-21 0:36 ` [PATCH 1/2] firewire: firewire is a big-endian bus Laurent Vivier 2015-04-21 1:04 ` Joe Perches 2015-04-21 14:13 ` Stefan Richter 2015-04-21 13:13 ` Stefan Richter 2015-04-21 16:04 ` Laurent Vivier 2015-04-21 0:36 ` [PATCH 2/2] firewire: add a parameter to force the speed of the devices Laurent Vivier 2015-04-21 14:03 ` Clemens Ladisch 2015-04-21 14:16 ` Stefan Richter 2015-04-21 14:33 ` Stefan Richter 2015-04-21 16:55 ` Laurent Vivier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).