Message-ID: <55368105.3000305@Vivier.EU>
Date: Tue, 21 Apr 2015 18:55:33 +0200
From: Laurent Vivier <Laurent@Vivier.EU>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0
MIME-Version: 1.0
To: Stefan Richter <stefanr@s5r6.in-berlin.de>
CC: linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] firewire: add a parameter to force the speed of the
 devices.
References: <1429576576-1637-1-git-send-email-laurent@vivier.eu>	<1429576576-1637-3-git-send-email-laurent@vivier.eu> <20150421163311.1d546b13@kant>
In-Reply-To: <20150421163311.1d546b13@kant>
Content-Type: multipart/signed; micalg=pgp-sha1;
 protocol="application/pgp-signature";
 boundary="IE0OdI8f3JdKgdPNkpbA6sMPKCgpthvna"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4868
Lines: 132

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--IE0OdI8f3JdKgdPNkpbA6sMPKCgpthvna
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable

Le 21/04/2015 16:33, Stefan Richter a =E9crit :
> 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 kind=
s
> of malfunctions of bus powered 1394 devices have been observed on PCI/P=
CIe
> 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 modul=
e
>> 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-de=
vice.c
>> index 5245567..a075827 100644
>> --- a/drivers/firewire/core-device.c
>> +++ b/drivers/firewire/core-device.c
>> @@ -44,6 +44,17 @@
>> =20
>>  #include "core.h"
>> =20
>> +static int force_speed =3D -1;
>> +module_param_named(force_speed, force_speed, int, 0644);
>> +MODULE_PARM_DESC(force_speed, "Force device speed (default =3D -1"
>> +	", FW100 =3D " __stringify(SCODE_100)
>> +	", FW200 =3D " __stringify(SCODE_200)
>> +	", FW400 =3D " __stringify(SCODE_400)
>> +	", FW800 =3D " __stringify(SCODE_800)
>> +	", FW1600 =3D " __stringify(SCODE_1600)
>> +	", FW3200 =3D " __stringify(SCODE_3200)
>> +	", FWBETA =3D " __stringify(SCODE_BETA));
>> +
>>  void fw_csr_iterator_init(struct fw_csr_iterator *ci, const u32 *p)
>>  {
>>  	ci->p =3D p + 1;
>> @@ -555,6 +566,8 @@ static int read_config_rom(struct fw_device *devic=
e, int generation)
>>  	}
>> =20
>>  	device->max_speed =3D device->node->max_speed;
>> +	if (force_speed !=3D -1)
>> +		device->max_speed =3D force_speed & 0xf;
>> =20
>>  	/*
>>  	 * Determine the speed of
> The parameter looks interesting, but in this form offers a few surprise=
s
> to unsuspecting users.
>
> IMO there should be stricter input validation, perhaps like so:
>
> 	int user_speed =3D ACCESS_ONCE(force_speed);
>
> 	if (user_speed >=3D SCODE_100 && user_speed <=3D SCODE_3200)
> 		device->max_speed =3D 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 a=
udit
> 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 shoul=
d
> be left out --- or it should be represented by a different value and it=
s
> 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 i=
f
> 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


--IE0OdI8f3JdKgdPNkpbA6sMPKCgpthvna
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAlU2gQUACgkQNKT2yavzbFPv7QCg4pnerYRaI/cNTb8FoaXTR6wS
7ZQAnjRvlMsVSr2bUD1SxgGJe+DRWjV1
=4e0l
-----END PGP SIGNATURE-----

--IE0OdI8f3JdKgdPNkpbA6sMPKCgpthvna--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/