lkml.org 
[lkml]   [2015]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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]
\
 
 \ /
  Last update: 2015-04-21 19:01    [W:0.086 / U:5.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site