Messages in this thread | | | Date | Tue, 21 Apr 2015 18:55:33 +0200 | From | Laurent Vivier <> | Subject | Re: [PATCH 2/2] firewire: add a parameter to force the speed of the devices. |
| |
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
[unhandled content-type:application/pgp-signature] | |