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

[Laurent Vivier]

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@xxxxxxxxx>
>> ---
>>  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: signature.asc
Description: OpenPGP digital signature