Re: [PATCH 2/2] firewire: add a parameter to force the speed of the devices.

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

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/