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.
    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/


    \
     
     \ /
      Last update: 2015-04-21 16:41    [W:2.800 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site