[PATCH RFC 0/2] ds3000 firmware loading improvements

[PATCH RFC 0/2] ds3000 firmware loading improvements

[Rémi Cardona]

Hi all,

Please consider these 2 patches as an RFC, especially the first one.

The first patch is something I've found while trying to wrap my head
around the driver and I could find no legitimate reason to keep this
lock since commit b9bf2ea, as dvb_frontend_init() is now only called in
a single thread within dvb_frontend_thread().

Here's a little context for the second patch: we (SmartJog) have found
a few TeVii S470/471 cards where the ds3000 front-end *sometimes*
reports that it already has a loaded firmware (the 0xb2 register) yet
the card does not really work. That's why I've added the printk() in
the second patch to display when such a case happens.

We thus have an extra patch that adds a module parameter to force the
firmware loading, regardless of the 0xb2 register's state. This patch
allows us to use cards that would otherwise fail. Would such a patch be
of interest?

Would a patch that completely removes the register check (and thus
always loads the firmware at init time) be a preferred alternative?

Thanks for any pointers/reviews.

Rémi Cardona

-- 
SmartJog | T: +33 1 5868 6229
27 Blvd Hippolyte Marquès, 94200 Ivry-sur-Seine, France
www.smartjog.com | a TDF Group company

[PATCH 1/2] [media] ds3000: Remove useless 'locking'

[Rémi Cardona]

Since b9bf2eafaad9c1ef02fb3db38c74568be601a43a, the function
ds3000_firmware_ondemand() is called only once during init. This
locking scheme may have been useful when the firmware was loaded at
each tune.

Furthermore, it looks like this 'lock' was put in to prevent concurrent
access (and not recursion as the comments suggest). However, this open-
coded mechanism is anything but race-free and should have used a proper
mutex.

Signed-off-by: Rémi Cardona <remi.cardona@smartjog.com>
---
 drivers/media/dvb/frontends/ds3000.c |    9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/media/dvb/frontends/ds3000.c b/drivers/media/dvb/frontends/ds3000.c
index 4c8ac26..066870a 100644
--- a/drivers/media/dvb/frontends/ds3000.c
+++ b/drivers/media/dvb/frontends/ds3000.c
@@ -233,7 +233,6 @@ struct ds3000_state {
 	struct i2c_adapter *i2c;
 	const struct ds3000_config *config;
 	struct dvb_frontend frontend;
-	u8 skip_fw_load;
 	/* previous uncorrected block counter for DVB-S2 */
 	u16 prevUCBS2;
 };
@@ -395,8 +394,6 @@ static int ds3000_firmware_ondemand(struct dvb_frontend *fe)
 	if (ds3000_readreg(state, 0xb2) <= 0)
 		return ret;
 
-	if (state->skip_fw_load)
-		return 0;
 	/* Load firmware */
 	/* request the firmware, this will block until someone uploads it */
 	printk(KERN_INFO "%s: Waiting for firmware upload (%s)...\n", __func__,
@@ -410,9 +407,6 @@ static int ds3000_firmware_ondemand(struct dvb_frontend *fe)
 		return ret;
 	}
 
-	/* Make sure we don't recurse back through here during loading */
-	state->skip_fw_load = 1;
-
 	ret = ds3000_load_firmware(fe, fw);
 	if (ret)
 		printk("%s: Writing firmware to device failed\n", __func__);
@@ -422,9 +416,6 @@ static int ds3000_firmware_ondemand(struct dvb_frontend *fe)
 	dprintk("%s: Firmware upload %s\n", __func__,
 			ret == 0 ? "complete" : "failed");
 
-	/* Ensure firmware is always loaded if required */
-	state->skip_fw_load = 0;
-
 	return ret;
 }
 
-- 
1.7.10.4

Re: [PATCH 1/2] [media] ds3000: Remove useless 'locking'

[Rémi Cardona]

On 08/30/2012 11:36 AM, Rémi Cardona wrote:
> Since b9bf2eafaad9c1ef02fb3db38c74568be601a43a, the function
> ds3000_firmware_ondemand() is called only once during init. This
> locking scheme may have been useful when the firmware was loaded at
> each tune.
> 
> Furthermore, it looks like this 'lock' was put in to prevent concurrent
> access (and not recursion as the comments suggest). However, this open-
> coded mechanism is anything but race-free and should have used a proper
> mutex.
> 
> Signed-off-by: Rémi Cardona <remi.cardona@smartjog.com>
> ---
>  drivers/media/dvb/frontends/ds3000.c |    9 ---------
>  1 file changed, 9 deletions(-)

Ping on that patch. Could anyone take a look at it?

Many thanks,

Rémi

[PATCH 2/2] [media] ds3000: properly report firmware loading issues

[Rémi Cardona]

ds3000_readreg() returns negative values in case of i2c failures. The
old code would simply return 0 when failing to read the 0xb2 register,
misleading ds3000_initfe() into believing that the firmware had been
correctly loaded.

Signed-off-by: Rémi Cardona <remi.cardona@smartjog.com>
---
 drivers/media/dvb/frontends/ds3000.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb/frontends/ds3000.c b/drivers/media/dvb/frontends/ds3000.c
index 066870a..4c774c4 100644
--- a/drivers/media/dvb/frontends/ds3000.c
+++ b/drivers/media/dvb/frontends/ds3000.c
@@ -391,8 +391,14 @@ static int ds3000_firmware_ondemand(struct dvb_frontend *fe)
 
 	dprintk("%s()\n", __func__);
 
-	if (ds3000_readreg(state, 0xb2) <= 0)
+	ret = ds3000_readreg(state, 0xb2);
+	if (ret == 0) {
+		printk(KERN_INFO "%s: Firmware already uploaded, skipping\n",
+			__func__);
 		return ret;
+	} else if (ret < 0) {
+		return ret;
+	}
 
 	/* Load firmware */
 	/* request the firmware, this will block until someone uploads it */
-- 
1.7.10.4

Re: [PATCH 2/2] [media] ds3000: properly report firmware loading issues

[Antti Palosaari]

On 08/30/2012 12:36 PM, Rémi Cardona wrote:
> ds3000_readreg() returns negative values in case of i2c failures. The
> old code would simply return 0 when failing to read the 0xb2 register,
> misleading ds3000_initfe() into believing that the firmware had been
> correctly loaded.
>
> Signed-off-by: Rémi Cardona <remi.cardona@smartjog.com>
> ---
>   drivers/media/dvb/frontends/ds3000.c |    8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/dvb/frontends/ds3000.c b/drivers/media/dvb/frontends/ds3000.c
> index 066870a..4c774c4 100644
> --- a/drivers/media/dvb/frontends/ds3000.c
> +++ b/drivers/media/dvb/frontends/ds3000.c
> @@ -391,8 +391,14 @@ static int ds3000_firmware_ondemand(struct dvb_frontend *fe)
>
>   	dprintk("%s()\n", __func__);
>
> -	if (ds3000_readreg(state, 0xb2) <= 0)
> +	ret = ds3000_readreg(state, 0xb2);
> +	if (ret == 0) {
> +		printk(KERN_INFO "%s: Firmware already uploaded, skipping\n",
> +			__func__);
>   		return ret;
> +	} else if (ret < 0) {
> +		return ret;
> +	}
>
>   	/* Load firmware */
>   	/* request the firmware, this will block until someone uploads it */
>

As I understand firmware downloading failure is coming from the fact 
that register read fails => fails to detect if firmware is already 
running or not.

Original behavior to expect firmware is loaded and running when register 
read fails is very stupid and your fix seems much better.

So first priority should be try fix that issue with register read. Is it 
coming from the USB stack (eg. error 110 timeout) or some other error 
coming from the fact chip answers wrong?

Do you see other register I/O failing too?

Does adding few usec sleep help?


regards
Antti


-- 
http://palosaari.fi/

Re: [PATCH 2/2] [media] ds3000: properly report firmware loading issues

[Rémi Cardona]

Hi Antti,

On 08/30/2012 03:39 PM, Antti Palosaari wrote:
> As I understand firmware downloading failure is coming from the fact
> that register read fails => fails to detect if firmware is already
> running or not.

Well we actually see 2 cases:

 - the register read failure (when ds3000_readreg() returns negative
values). This case is fairly rare, and no changes we've done to the
driver allowed us to make those cards work.

 - the register read returning 0. Looking at the current code, it looks
like the 0xb2 register is supposed to mean that a firmware is loaded.
This case is fairly common: we've had many cards randomly saying that a
firmware was loaded when none had been. Often, a simple reboot will do
the trick. But sometimes, forcing the firmware upload (ie, bypassing the
0xb2 register check) allows the stubborn cards to function properly.

> Original behavior to expect firmware is loaded and running when register
> read fails is very stupid and your fix seems much better.

Well, this patch should not really change the behavior. It just
propagates register read errors to ds3000_initfe(). It'll just fail earlier.

> So first priority should be try fix that issue with register read. Is it
> coming from the USB stack (eg. error 110 timeout) or some other error
> coming from the fact chip answers wrong?

The cards we're using are PCIe (and not the ones with an embedded USB
controller).

> Do you see other register I/O failing too?

I'll see if I can get you an answer for that, since the cards are
shipped with the appliance we send to our customers. Remote debugging is
somewhat tricky.

> Does adding few usec sleep help?

I'm not quite sure where to add those sleeps. In the register
reading/writing functions? 10us? 100us?

Many thanks

Rémi

Re: [PATCH 2/2] [media] ds3000: properly report firmware loading issues

[Antti Palosaari]

On 08/30/2012 06:21 PM, Rémi Cardona wrote:
> Hi Antti,
>
> On 08/30/2012 03:39 PM, Antti Palosaari wrote:
>> As I understand firmware downloading failure is coming from the fact
>> that register read fails => fails to detect if firmware is already
>> running or not.
>
> Well we actually see 2 cases:
>
>   - the register read failure (when ds3000_readreg() returns negative
> values). This case is fairly rare, and no changes we've done to the
> driver allowed us to make those cards work.

hmm, looks like ds3000_readreg() logic is still a little bit broken. It 
checks count of sent messages and compares it to 2. But if I2C-adapter 
sends only 1 message or 3 (which should not be possible) function return 
that count instead of -EREMOTEIO. OK, quite rare situation, but one 
point more to fail if I2C-adapter has also bug.

But that happens for return value 0 too. Could it be the issue? 
I2C-adapter returns 0 for some reason? Bug in I2C-adapter with bug in 
ds3000_readreg() implementation?

>   - the register read returning 0. Looking at the current code, it looks
> like the 0xb2 register is supposed to mean that a firmware is loaded.
> This case is fairly common: we've had many cards randomly saying that a
> firmware was loaded when none had been. Often, a simple reboot will do
> the trick. But sometimes, forcing the firmware upload (ie, bypassing the
> 0xb2 register check) allows the stubborn cards to function properly.
>
>> Original behavior to expect firmware is loaded and running when register
>> read fails is very stupid and your fix seems much better.
>
> Well, this patch should not really change the behavior. It just
> propagates register read errors to ds3000_initfe(). It'll just fail earlier.
>
>> So first priority should be try fix that issue with register read. Is it
>> coming from the USB stack (eg. error 110 timeout) or some other error
>> coming from the fact chip answers wrong?
>
> The cards we're using are PCIe (and not the ones with an embedded USB
> controller).

The idea of my question was to ask where those errors are coming from (I 
spoke mistakenly about USB because I usually play with USB devices).

You basically see two different kind of errors, 1) bus communication 
fails, eg. usb timeouts. 2) chips returns error status. Later cases the 
error could come from the this could come from the firmware if chip uses 
firmware or from the silicon. It could be from the I2C-adapter firmware.

>> Do you see other register I/O failing too?
>
> I'll see if I can get you an answer for that, since the cards are
> shipped with the appliance we send to our customers. Remote debugging is
> somewhat tricky.
>
>> Does adding few usec sleep help?
>
> I'm not quite sure where to add those sleeps. In the register
> reading/writing functions? 10us? 100us?

Add sleep after the each operation. Good place to add sleep is 
I2C-adapter. I2C-adapters usually supports two different operations, 
write and read + write using repeated START condition. Former us used 
typically for register write and later for register read.

500us is good choice. If it is only that one register read which causes 
problems, how about repeating it?

>
> Many thanks
>
> Rémi
>


-- 
http://palosaari.fi/

Re: [PATCH 2/2] [media] ds3000: properly report firmware loading issues

[Rémi Cardona]

On 08/30/2012 06:00 PM, Antti Palosaari wrote:
> hmm, looks like ds3000_readreg() logic is still a little bit broken. It
> checks count of sent messages and compares it to 2. But if I2C-adapter
> sends only 1 message or 3 (which should not be possible) function return
> that count instead of -EREMOTEIO. OK, quite rare situation, but one
> point more to fail if I2C-adapter has also bug.

You're absolutely right. The logic is completely awkward. I'm currently
working on a new patch series that will clean up all the register
reading/writing functions to properly check i2c_transfer()'s return code.

> But that happens for return value 0 too. Could it be the issue?
> I2C-adapter returns 0 for some reason? Bug in I2C-adapter with bug in
> ds3000_readreg() implementation?
[...]
> You basically see two different kind of errors, 1) bus communication
> fails, eg. usb timeouts. 2) chips returns error status. Later cases the
> error could come from the this could come from the firmware if chip uses
> firmware or from the silicon. It could be from the I2C-adapter firmware.

Right now, I just have no idea. Right now, the overwhelming majority of
the cards we tested with a recent kernel (about 60 units) are working
just fine with the code as-is.

My main (if not sole) objective for now is to make it easier for us (but
hopefully for others too) to find defective cards/chips.

> Add sleep after the each operation. Good place to add sleep is
> I2C-adapter. I2C-adapters usually supports two different operations,
> write and read + write using repeated START condition. Former us used
> typically for register write and later for register read.
> 
> 500us is good choice. If it is only that one register read which causes
> problems, how about repeating it?

I'll first see how our units are behaving with the upcoming patches but
I'll definitely keep this in mind as a next step.

Thanks again,

Rémi

Re: Re: [PATCH 2/2] [media] ds3000: properly report firmware loadingissues

[nibble.max]

As remember that there is a fault in the tuner register read function in ds3000.c file.
It will cause the read back value wrong.
ds3000.c file, 
static int ds3000_tuner_readreg(struct ds3000_state *state, u8 reg)
{
.......
ds3000_writereg(state, 0x03, 0x12);
.......
}
in DVBSky code, m88ds3103.c file,
static int m88ds3103_tuner_readreg(struct m88ds3103_state *state, u8 reg)
{
........
m88ds3103_writereg(state, 0x03, 0x11);
........
}
DVBSky code can read back the value correctly. 
As known that m88ds3103.c also supports the older tuner(m88ts2020) and demodulator(m88ds3000/3002).

2012-08-31 16:18:38 nibble.max@gmail.com
>On 08/30/2012 06:21 PM, Rémi Cardona wrote:
>> Hi Antti,
>>
>> On 08/30/2012 03:39 PM, Antti Palosaari wrote:
>>> As I understand firmware downloading failure is coming from the fact
>>> that register read fails => fails to detect if firmware is already
>>> running or not.
>>
>> Well we actually see 2 cases:
>>
>>   - the register read failure (when ds3000_readreg() returns negative
>> values). This case is fairly rare, and no changes we've done to the
>> driver allowed us to make those cards work.
>
>hmm, looks like ds3000_readreg() logic is still a little bit broken. It 
>checks count of sent messages and compares it to 2. But if I2C-adapter 
>sends only 1 message or 3 (which should not be possible) function return 
>that count instead of -EREMOTEIO. OK, quite rare situation, but one 
>point more to fail if I2C-adapter has also bug.
>
>But that happens for return value 0 too. Could it be the issue? 
>I2C-adapter returns 0 for some reason? Bug in I2C-adapter with bug in 
>ds3000_readreg() implementation?
>
>>   - the register read returning 0. Looking at the current code, it looks
>> like the 0xb2 register is supposed to mean that a firmware is loaded.
>> This case is fairly common: we've had many cards randomly saying that a
>> firmware was loaded when none had been. Often, a simple reboot will do
>> the trick. But sometimes, forcing the firmware upload (ie, bypassing the
>> 0xb2 register check) allows the stubborn cards to function properly.
>>
>>> Original behavior to expect firmware is loaded and running when register
>>> read fails is very stupid and your fix seems much better.
>>
>> Well, this patch should not really change the behavior. It just
>> propagates register read errors to ds3000_initfe(). It'll just fail earlier.
>>
>>> So first priority should be try fix that issue with register read. Is it
>>> coming from the USB stack (eg. error 110 timeout) or some other error
>>> coming from the fact chip answers wrong?
>>
>> The cards we're using are PCIe (and not the ones with an embedded USB
>> controller).
>
>The idea of my question was to ask where those errors are coming from (I 
>spoke mistakenly about USB because I usually play with USB devices).
>
>You basically see two different kind of errors, 1) bus communication 
>fails, eg. usb timeouts. 2) chips returns error status. Later cases the 
>error could come from the this could come from the firmware if chip uses 
>firmware or from the silicon. It could be from the I2C-adapter firmware.
>
>>> Do you see other register I/O failing too?
>>
>> I'll see if I can get you an answer for that, since the cards are
>> shipped with the appliance we send to our customers. Remote debugging is
>> somewhat tricky.
>>
>>> Does adding few usec sleep help?
>>
>> I'm not quite sure where to add those sleeps. In the register
>> reading/writing functions? 10us? 100us?
>
>Add sleep after the each operation. Good place to add sleep is 
>I2C-adapter. I2C-adapters usually supports two different operations, 
>write and read + write using repeated START condition. Former us used 
>typically for register write and later for register read.
>
>500us is good choice. If it is only that one register read which causes 
>problems, how about repeating it?
>
>>
>> Many thanks
>>
>> Rémi
>>
>
>
>-- 
>http://palosaari.fi/
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] ds3000: properly report firmware loadingissues

[Rémi Cardona]

Hi Max,

On 08/31/2012 10:29 AM, nibble.max wrote:
> As remember that there is a fault in the tuner register read function in ds3000.c file.
> It will cause the read back value wrong.

Well, using 0x12 works out for most of the cards we have in the wild.
Not knowing what 0x11 / 0x12 means, I'd be wary of suggesting such a
change myself. Once I isolate the faulty cards, I'll try using 0x11 on
them to see if it changes anything.

Thanks for the pointer,

Rémi

Re: Re: [PATCH 2/2] [media] ds3000: properly report firmware loadingissues

[nibble.max]

>Hi Max,
>
>On 08/31/2012 10:29 AM, nibble.max wrote:
>> As remember that there is a fault in the tuner register read function in ds3000.c file.
>> It will cause the read back value wrong.
>
>Well, using 0x12 works out for most of the cards we have in the wild.
>Not knowing what 0x11 / 0x12 means, I'd be wary of suggesting such a
>change myself. Once I isolate the faulty cards, I'll try using 0x11 on
>them to see if it changes anything.
This value is related with i2c operation through demod i2c bus to access tuner i2c bus. tuner i2c bus is bridges with demod i2c bus. This value tells the demod that how many i2c stop will be detected. The i2c bus to tuner will be closed by the demod automatically, as soon as the demod detects such many i2c stops.
0x11 means there is only one i2c stop during tuner i2c operation.
0x12 means there are two i2c stop during tuner i2c operation.
In cx23885 i2c implement and flag=I2C_M_RD, for reading there is only one stop.
yes, 0x12 seems working since the operation after tuner reading operation is not critical.
Max
>                                                                             
>Thanks for the pointer,
>
>Rémi