Re: [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
>> ---
>> 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
signature.asc
Description: OpenPGP digital signature
Re: [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
> ---
> 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] firewire: add a parameter to force the speed of the devices.
On Apr 21 Clemens Ladisch wrote: > 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. > > > > 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. > > Why a module parameter? Why not automatically apply this workaround to > this specific device? I doubt that this iPod model needs the workaround. I suspect the particular hardware combination needs one. -- Stefan Richter -=-= -=-- =-=-= http://arcgraph.de/sr/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] firewire: add a parameter to force the speed of the devices.
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. > > 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. Why a module parameter? Why not automatically apply this workaround to this specific device? Regards, Clemens -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] firewire: add a parameter to force the speed of the devices.
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. 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. Why a module parameter? Why not automatically apply this workaround to this specific device? Regards, Clemens -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] firewire: add a parameter to force the speed of the devices.
On Apr 21 Clemens Ladisch wrote: 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. 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. Why a module parameter? Why not automatically apply this workaround to this specific device? I doubt that this iPod model needs the workaround. I suspect the particular hardware combination needs one. -- Stefan Richter -=-= -=-- =-=-= http://arcgraph.de/sr/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [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 laur...@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/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [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 laur...@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
signature.asc
Description: OpenPGP digital signature
[PATCH 2/2] firewire: add a parameter to force the speed of the devices.
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.
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
---
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
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] firewire: add a parameter to force the speed of the devices.
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.
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 laur...@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
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
