Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer

Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer

[Olli Salonen]

Hello Peter and Max,

I noticed that when using kernel 4.10 or newer my DVBSky S960 and
S960CI satellite USB TV tuners stopped working properly. Basically,
they will fail at one point when tuning to a channel. This typically
takes less than 100 tuning attempts. For perspective, when performing
a full channel scan on my system, the tuner tunes at least 500 times.
After the tuner fails, I need to reboot the PC (probably unloading the
driver and loading it again would do).

2018-04-04 10:17:36.756 [   INFO] mpegts: 12149H in 4.8E - tuning on
Montage Technology M88DS3103 : DVB-S #0
2018-04-04 10:17:37.159 [  ERROR] diseqc: failed to send diseqc cmd
(e=Connection timed out)
2018-04-04 10:17:37.160 [   INFO] mpegts: 12265H in 4.8E - tuning on
Montage Technology M88DS3103 : DVB-S #0
2018-04-04 10:17:37.535 [  ERROR] diseqc: failed to send diseqc cmd
(e=Connection timed out)

I did a kernel bisect between 4.9 and 4.10. It seems the commit that
breaks my tuner is the following one:

9d659ae14b545c4296e812c70493bfdc999b5c1c is the first bad commit
commit 9d659ae14b545c4296e812c70493bfdc999b5c1c
Author: Peter Zijlstra <peterz@infradead.org>
Date:   Tue Aug 23 14:40:16 2016 +0200

    locking/mutex: Add lock handoff to avoid starvation

I couldn't easily revert that commit only. I can see that the
drivers/media/usb/dvb-usb-v2/dvbsky.c driver does use mutex_lock() and
mutex_lock_interruptible() in a few places.

Do you guys see anything that's obviously wrong in the way the mutexes
are used in dvbsky.c or anything in that particular patch that could
cause this issue?

Thanks and best regards,
-olli

Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer

[Olli Salonen]

I created a report of the issue in Bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=199323

I'd be grateful for any tips on how to debug this further.

Cheers,
-olli

On 4 April 2018 at 14:41, Olli Salonen <olli.salonen@iki.fi> wrote:
> Hello Peter and Max,
>
> I noticed that when using kernel 4.10 or newer my DVBSky S960 and
> S960CI satellite USB TV tuners stopped working properly. Basically,
> they will fail at one point when tuning to a channel. This typically
> takes less than 100 tuning attempts. For perspective, when performing
> a full channel scan on my system, the tuner tunes at least 500 times.
> After the tuner fails, I need to reboot the PC (probably unloading the
> driver and loading it again would do).
>
> 2018-04-04 10:17:36.756 [   INFO] mpegts: 12149H in 4.8E - tuning on
> Montage Technology M88DS3103 : DVB-S #0
> 2018-04-04 10:17:37.159 [  ERROR] diseqc: failed to send diseqc cmd
> (e=Connection timed out)
> 2018-04-04 10:17:37.160 [   INFO] mpegts: 12265H in 4.8E - tuning on
> Montage Technology M88DS3103 : DVB-S #0
> 2018-04-04 10:17:37.535 [  ERROR] diseqc: failed to send diseqc cmd
> (e=Connection timed out)
>
> I did a kernel bisect between 4.9 and 4.10. It seems the commit that
> breaks my tuner is the following one:
>
> 9d659ae14b545c4296e812c70493bfdc999b5c1c is the first bad commit
> commit 9d659ae14b545c4296e812c70493bfdc999b5c1c
> Author: Peter Zijlstra <peterz@infradead.org>
> Date:   Tue Aug 23 14:40:16 2016 +0200
>
>     locking/mutex: Add lock handoff to avoid starvation
>
> I couldn't easily revert that commit only. I can see that the
> drivers/media/usb/dvb-usb-v2/dvbsky.c driver does use mutex_lock() and
> mutex_lock_interruptible() in a few places.
>
> Do you guys see anything that's obviously wrong in the way the mutexes
> are used in dvbsky.c or anything in that particular patch that could
> cause this issue?
>
> Thanks and best regards,
> -olli

Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer

[Peter Zijlstra]

On Wed, Apr 04, 2018 at 02:41:51PM +0300, Olli Salonen wrote:
> Hello Peter and Max,
> 
> I noticed that when using kernel 4.10 or newer my DVBSky S960 and
> S960CI satellite USB TV tuners stopped working properly. Basically,
> they will fail at one point when tuning to a channel. This typically
> takes less than 100 tuning attempts. For perspective, when performing
> a full channel scan on my system, the tuner tunes at least 500 times.
> After the tuner fails, I need to reboot the PC (probably unloading the
> driver and loading it again would do).
> 
> 2018-04-04 10:17:36.756 [   INFO] mpegts: 12149H in 4.8E - tuning on
> Montage Technology M88DS3103 : DVB-S #0
> 2018-04-04 10:17:37.159 [  ERROR] diseqc: failed to send diseqc cmd
> (e=Connection timed out)
> 2018-04-04 10:17:37.160 [   INFO] mpegts: 12265H in 4.8E - tuning on
> Montage Technology M88DS3103 : DVB-S #0
> 2018-04-04 10:17:37.535 [  ERROR] diseqc: failed to send diseqc cmd
> (e=Connection timed out)
> 
> I did a kernel bisect between 4.9 and 4.10. It seems the commit that
> breaks my tuner is the following one:
> 
> 9d659ae14b545c4296e812c70493bfdc999b5c1c is the first bad commit
> commit 9d659ae14b545c4296e812c70493bfdc999b5c1c
> Author: Peter Zijlstra <peterz@infradead.org>
> Date:   Tue Aug 23 14:40:16 2016 +0200
> 
>     locking/mutex: Add lock handoff to avoid starvation
> 
> I couldn't easily revert that commit only. I can see that the
> drivers/media/usb/dvb-usb-v2/dvbsky.c driver does use mutex_lock() and
> mutex_lock_interruptible() in a few places.
> 
> Do you guys see anything that's obviously wrong in the way the mutexes
> are used in dvbsky.c or anything in that particular patch that could
> cause this issue?

Nothing, sorry.. really weird. That driver looks fairly straight forward
with respect to mutex usage (although obviously I have less than 0 clue
on the whole usb media thing).

That it breaks that driver would suggest something funny with it though;
because the kernel has loads and loads of mutexes in and they all appear
to work well with that patch -- in fact, it fixed a reported starvation
case.

The only way for that patch to affect things is if there is contention
on the mutex though; so who or what is also trying to acquire the mutex?

The reported error is a timeout, suggesting that whoever is contending
on the lock is keeping it held too long? I do notice that
dvbsky_stream_ctrl() has an msleep() while holding a mutex.

Do you have any idea which of the 3 (afaict) mutexes in that driver is
failing? Going by the fact that it's failing to send, I'd hazard a guess
it's the i2c mutex, but again, I have less than 0 clues about i2c.

Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer

[Olli Salonen]

Thank you for your response Peter!

Indeed, it seems strange. dvbsky.c driver seems to use mutex_lock in
very much the same way as many other drivers. I've now confirmed that
I can get a 4.10 kernel with working DVBSky S960 by reverting the
following 4 patches:

549bdd3 Revert "locking/mutex: Add lock handoff to avoid starvation"
3210f31 Revert "locking/mutex: Restructure wait loop"
418a170 Revert "locking/mutex: Simplify some ww_mutex code in
__mutex_lock_common()"
0b1fb8f Revert "locking/mutex: Enable optimistic spinning of woken waiter"
c470abd Linux 4.10

The other 3 three I need to revert to be able to revert 9d659ae.

I added some more debugging into the DVBsky driver to better
understand what's happening.

After the point when the fault starts any I2C read from the m88ds3103
demodulator provides answer "07 ff 49 00" or a subset of it. Before
13:59:42 there are no I2C reads returning an answer starting with 07.

Apr 10 13:59:42 nucserver kernel: [ 4744.048206]
dvb_usb_dvbsky:dvbsky_rc_query: usb 1-1:
Apr 10 13:59:42 nucserver kernel: [ 4744.048319]
dvb_usb_dvbsky:dvbsky_usb_generic_rw: usb 1-1: write: 10, read: ff ff
Apr 10 13:59:42 nucserver kernel: [ 4744.080229]
dvb_usb_dvbsky:dvbsky_usb_read_status: usb 1-1:
Apr 10 13:59:42 nucserver kernel: [ 4744.080245]
dvb_usb_dvbsky:dvbsky_i2c_xfer: usb 1-1: num: 2
Apr 10 13:59:42 nucserver kernel: [ 4744.080915]
dvb_usb_dvbsky:dvbsky_usb_generic_rw: usb 1-1: write: 09 01 01 68 0d,
read: 08 01
Apr 10 13:59:42 nucserver kernel: [ 4744.080931]
m88ds3103:m88ds3103_read_status: m88ds3103 4-0068: lock=01 status=00
Apr 10 13:59:42 nucserver kernel: [ 4744.080943]
m88ds3103:m88ds3103_set_frontend: m88ds3103 4-0068: delivery_system=6
modulation=9 frequency=1097000 symbol_rate=23000000 inversion=2
pilot=2 rolloff=0
Apr 10 13:59:42 nucserver kernel: [ 4744.080953]
dvb_usb_dvbsky:dvbsky_i2c_xfer: usb 1-1: num: 1
Apr 10 13:59:42 nucserver kernel: [ 4744.081452]
dvb_usb_dvbsky:dvbsky_usb_generic_rw: usb 1-1: write: 08 68 02 07 80,
read: 08
Apr 10 13:59:42 nucserver kernel: [ 4744.081562]
dvb_usb_dvbsky:dvbsky_i2c_xfer: usb 1-1: num: 2
Apr 10 13:59:42 nucserver kernel: [ 4744.082263]
dvb_usb_dvbsky:dvbsky_usb_generic_rw: usb 1-1: write: 09 01 01 68 3f,
read: 08 ff
Apr 10 13:59:42 nucserver kernel: [ 4744.082287]
dvb_usb_dvbsky:dvbsky_i2c_xfer: usb 1-1: num: 1
Apr 10 13:59:42 nucserver kernel: [ 4744.082706]
dvb_usb_dvbsky:dvbsky_usb_generic_rw: usb 1-1: write: 08 68 02 03 11,
read: 07
Apr 10 13:59:42 nucserver kernel: [ 4744.082715]
dvb_usb_dvbsky:dvbsky_i2c_xfer: usb 1-1: num: 2
Apr 10 13:59:42 nucserver kernel: [ 4744.083119]
dvb_usb_dvbsky:dvbsky_usb_generic_rw: usb 1-1: write: 09 01 01 60 3d,
read: 07 ff
Apr 10 13:59:42 nucserver kernel: [ 4744.083230]
dvb_usb_dvbsky:dvbsky_i2c_xfer: usb 1-1: num: 1
Apr 10 13:59:42 nucserver kernel: [ 4744.083645]
dvb_usb_dvbsky:dvbsky_usb_generic_rw: usb 1-1: write: 08 68 02 07 00,
read: 07
Apr 10 13:59:42 nucserver kernel: [ 4744.083752]
dvb_usb_dvbsky:dvbsky_i2c_xfer: usb 1-1: num: 1
Apr 10 13:59:42 nucserver kernel: [ 4744.084114]
dvb_usb_dvbsky:dvbsky_usb_generic_rw: usb 1-1: write: 08 68 02 03 11,
read: 07
Apr 10 13:59:42 nucserver kernel: [ 4744.084130]
dvb_usb_dvbsky:dvbsky_i2c_xfer: usb 1-1: num: 2
Apr 10 13:59:42 nucserver kernel: [ 4744.084484]
dvb_usb_dvbsky:dvbsky_usb_generic_rw: usb 1-1: write: 09 01 01 60 21,
read: 07 ff

I really cannot see any obvious link between the mutexes and the I2C
operations starting to fail, but indeed the device starts to work as
normal if I revert that patch.

Cheers,
-olli


On 9 April 2018 at 11:14, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Apr 04, 2018 at 02:41:51PM +0300, Olli Salonen wrote:
>> Hello Peter and Max,
>>
>> I noticed that when using kernel 4.10 or newer my DVBSky S960 and
>> S960CI satellite USB TV tuners stopped working properly. Basically,
>> they will fail at one point when tuning to a channel. This typically
>> takes less than 100 tuning attempts. For perspective, when performing
>> a full channel scan on my system, the tuner tunes at least 500 times.
>> After the tuner fails, I need to reboot the PC (probably unloading the
>> driver and loading it again would do).
>>
>> 2018-04-04 10:17:36.756 [   INFO] mpegts: 12149H in 4.8E - tuning on
>> Montage Technology M88DS3103 : DVB-S #0
>> 2018-04-04 10:17:37.159 [  ERROR] diseqc: failed to send diseqc cmd
>> (e=Connection timed out)
>> 2018-04-04 10:17:37.160 [   INFO] mpegts: 12265H in 4.8E - tuning on
>> Montage Technology M88DS3103 : DVB-S #0
>> 2018-04-04 10:17:37.535 [  ERROR] diseqc: failed to send diseqc cmd
>> (e=Connection timed out)
>>
>> I did a kernel bisect between 4.9 and 4.10. It seems the commit that
>> breaks my tuner is the following one:
>>
>> 9d659ae14b545c4296e812c70493bfdc999b5c1c is the first bad commit
>> commit 9d659ae14b545c4296e812c70493bfdc999b5c1c
>> Author: Peter Zijlstra <peterz@infradead.org>
>> Date:   Tue Aug 23 14:40:16 2016 +0200
>>
>>     locking/mutex: Add lock handoff to avoid starvation
>>
>> I couldn't easily revert that commit only. I can see that the
>> drivers/media/usb/dvb-usb-v2/dvbsky.c driver does use mutex_lock() and
>> mutex_lock_interruptible() in a few places.
>>
>> Do you guys see anything that's obviously wrong in the way the mutexes
>> are used in dvbsky.c or anything in that particular patch that could
>> cause this issue?
>
> Nothing, sorry.. really weird. That driver looks fairly straight forward
> with respect to mutex usage (although obviously I have less than 0 clue
> on the whole usb media thing).
>
> That it breaks that driver would suggest something funny with it though;
> because the kernel has loads and loads of mutexes in and they all appear
> to work well with that patch -- in fact, it fixed a reported starvation
> case.
>
> The only way for that patch to affect things is if there is contention
> on the mutex though; so who or what is also trying to acquire the mutex?
>
> The reported error is a timeout, suggesting that whoever is contending
> on the lock is keeping it held too long? I do notice that
> dvbsky_stream_ctrl() has an msleep() while holding a mutex.
>
> Do you have any idea which of the 3 (afaict) mutexes in that driver is
> failing? Going by the fact that it's failing to send, I'd hazard a guess
> it's the i2c mutex, but again, I have less than 0 clues about i2c.
>

Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer

[Antti Palosaari]

On 04/18/2018 07:49 AM, Olli Salonen wrote:
> Thank you for your response Peter!
> 
> Indeed, it seems strange. dvbsky.c driver seems to use mutex_lock in
> very much the same way as many other drivers. I've now confirmed that
> I can get a 4.10 kernel with working DVBSky S960 by reverting the
> following 4 patches:
> 
> 549bdd3 Revert "locking/mutex: Add lock handoff to avoid starvation"
> 3210f31 Revert "locking/mutex: Restructure wait loop"
> 418a170 Revert "locking/mutex: Simplify some ww_mutex code in
> __mutex_lock_common()"
> 0b1fb8f Revert "locking/mutex: Enable optimistic spinning of woken waiter"
> c470abd Linux 4.10

These kind of issues tend to be timing issues very often. Just add some 
sleeps to i2c adapter algo / usb control messages and test.

regards
Antti




-- 
http://palosaari.fi/

Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer

[Olli Salonen]

Thanks for the suggestion Antti.

I've tried to add a delay in various places, but haven't seen any
improvement. However, what I did saw was that if I added an msleep
after the lock:

static int dvbsky_usb_generic_rw(struct dvb_usb_device *d,
                u8 *wbuf, u16 wlen, u8 *rbuf, u16 rlen)
{
        int ret;
        struct dvbsky_state *state = d_to_priv(d);

        mutex_lock(&d->usb_mutex);
        msleep(20);

The error was seen very within a minute. If I increased the msleep to
50, it failed within seconds. This doesn't seem to make sense to me.
This is the only function where usb_mutex is used. If the mutex is
held for a longer time, the next attempt to lock the mutex should just
be delayed a bit, no?

Cheers,
-olli

On 18 April 2018 at 10:49, Antti Palosaari <crope@iki.fi> wrote:
>
> On 04/18/2018 07:49 AM, Olli Salonen wrote:
>>
>> Thank you for your response Peter!
>>
>> Indeed, it seems strange. dvbsky.c driver seems to use mutex_lock in
>> very much the same way as many other drivers. I've now confirmed that
>> I can get a 4.10 kernel with working DVBSky S960 by reverting the
>> following 4 patches:
>>
>> 549bdd3 Revert "locking/mutex: Add lock handoff to avoid starvation"
>> 3210f31 Revert "locking/mutex: Restructure wait loop"
>> 418a170 Revert "locking/mutex: Simplify some ww_mutex code in
>> __mutex_lock_common()"
>> 0b1fb8f Revert "locking/mutex: Enable optimistic spinning of woken waiter"
>> c470abd Linux 4.10
>
>
> These kind of issues tend to be timing issues very often. Just add some
> sleeps to i2c adapter algo / usb control messages and test.
>
> regards
> Antti
>
>
>
>
> --
> http://palosaari.fi/

Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer

[Mauro Carvalho Chehab]

Em Fri, 27 Apr 2018 16:25:08 +0200
Olli Salonen <olli.salonen@iki.fi> escreveu:

> Thanks for the suggestion Antti.
> 
> I've tried to add a delay in various places, but haven't seen any
> improvement. However, what I did saw was that if I added an msleep
> after the lock:
> 
> static int dvbsky_usb_generic_rw(struct dvb_usb_device *d,
>                 u8 *wbuf, u16 wlen, u8 *rbuf, u16 rlen)
> {
>         int ret;
>         struct dvbsky_state *state = d_to_priv(d);
> 
>         mutex_lock(&d->usb_mutex);
>         msleep(20);
> 
> The error was seen very within a minute. If I increased the msleep to
> 50, it failed within seconds. This doesn't seem to make sense to me.
> This is the only function where usb_mutex is used. If the mutex is
> held for a longer time, the next attempt to lock the mutex should just
> be delayed a bit, no?

I don't like the idea of having two mutexes there to protect reading/writing
to data one for "generic" r/w ops, and another one just for streaming
control, with ends by calling the "generic" mutex.

IMHO, I would get rid of one of the mutexes, e. g. something like the
patch below (untested).

Regards,
Mauro

media: dvbsky: use just one mutex for serializing device R/W ops

Right now, there are two mutexes serializing r/w ops: one "generic"
and another one specifically for stream on/off.

Clean it a little bit, getting rid of one of the mutexes.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c b/drivers/media/usb/dvb-usb-v2/dvbsky.c
index 43eb82884555..50553975c39d 100644
--- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
+++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
@@ -31,7 +31,6 @@ MODULE_PARM_DESC(disable_rc, "Disable inbuilt IR receiver.");
 DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 
 struct dvbsky_state {
-	struct mutex stream_mutex;
 	u8 ibuf[DVBSKY_BUF_LEN];
 	u8 obuf[DVBSKY_BUF_LEN];
 	u8 last_lock;
@@ -68,18 +67,17 @@ static int dvbsky_usb_generic_rw(struct dvb_usb_device *d,
 
 static int dvbsky_stream_ctrl(struct dvb_usb_device *d, u8 onoff)
 {
-	struct dvbsky_state *state = d_to_priv(d);
 	int ret;
-	u8 obuf_pre[3] = { 0x37, 0, 0 };
-	u8 obuf_post[3] = { 0x36, 3, 0 };
+	static u8 obuf_pre[3] = { 0x37, 0, 0 };
+	static u8 obuf_post[3] = { 0x36, 3, 0 };
 
-	mutex_lock(&state->stream_mutex);
-	ret = dvbsky_usb_generic_rw(d, obuf_pre, 3, NULL, 0);
+	mutex_lock(&d->usb_mutex);
+	ret = dvb_usbv2_generic_rw_locked(d, obuf_pre, 3, NULL, 0);
 	if (!ret && onoff) {
 		msleep(20);
-		ret = dvbsky_usb_generic_rw(d, obuf_post, 3, NULL, 0);
+		ret = dvb_usbv2_generic_rw_locked(d, obuf_post, 3, NULL, 0);
 	}
-	mutex_unlock(&state->stream_mutex);
+	mutex_unlock(&d->usb_mutex);
 	return ret;
 }
 
@@ -744,8 +742,6 @@ static int dvbsky_init(struct dvb_usb_device *d)
 	if (ret)
 		return ret;
 	*/
-	mutex_init(&state->stream_mutex);
-
 	state->last_lock = 0;
 
 	return 0;



Thanks,
Mauro

Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer

[Olli Salonen]

I did test the patch and while it doesn't seem to introduce any
negative side effects it does not provide a remedy for the original
problem that was seen after introducing
9d659ae14b545c4296e812c70493bfdc999b5c1c (you probably did not expect
that either).


Cheers,
-olli

On 27 April 2018 at 18:33, Mauro Carvalho Chehab
<mchehab+samsung@kernel.org> wrote:
> Em Fri, 27 Apr 2018 16:25:08 +0200
> Olli Salonen <olli.salonen@iki.fi> escreveu:
>
>> Thanks for the suggestion Antti.
>>
>> I've tried to add a delay in various places, but haven't seen any
>> improvement. However, what I did saw was that if I added an msleep
>> after the lock:
>>
>> static int dvbsky_usb_generic_rw(struct dvb_usb_device *d,
>>                 u8 *wbuf, u16 wlen, u8 *rbuf, u16 rlen)
>> {
>>         int ret;
>>         struct dvbsky_state *state = d_to_priv(d);
>>
>>         mutex_lock(&d->usb_mutex);
>>         msleep(20);
>>
>> The error was seen very within a minute. If I increased the msleep to
>> 50, it failed within seconds. This doesn't seem to make sense to me.
>> This is the only function where usb_mutex is used. If the mutex is
>> held for a longer time, the next attempt to lock the mutex should just
>> be delayed a bit, no?
>
> I don't like the idea of having two mutexes there to protect reading/writing
> to data one for "generic" r/w ops, and another one just for streaming
> control, with ends by calling the "generic" mutex.
>
> IMHO, I would get rid of one of the mutexes, e. g. something like the
> patch below (untested).
>
> Regards,
> Mauro
>
> media: dvbsky: use just one mutex for serializing device R/W ops
>
> Right now, there are two mutexes serializing r/w ops: one "generic"
> and another one specifically for stream on/off.
>
> Clean it a little bit, getting rid of one of the mutexes.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
>
> diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> index 43eb82884555..50553975c39d 100644
> --- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> +++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> @@ -31,7 +31,6 @@ MODULE_PARM_DESC(disable_rc, "Disable inbuilt IR receiver.");
>  DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
>
>  struct dvbsky_state {
> -       struct mutex stream_mutex;
>         u8 ibuf[DVBSKY_BUF_LEN];
>         u8 obuf[DVBSKY_BUF_LEN];
>         u8 last_lock;
> @@ -68,18 +67,17 @@ static int dvbsky_usb_generic_rw(struct dvb_usb_device *d,
>
>  static int dvbsky_stream_ctrl(struct dvb_usb_device *d, u8 onoff)
>  {
> -       struct dvbsky_state *state = d_to_priv(d);
>         int ret;
> -       u8 obuf_pre[3] = { 0x37, 0, 0 };
> -       u8 obuf_post[3] = { 0x36, 3, 0 };
> +       static u8 obuf_pre[3] = { 0x37, 0, 0 };
> +       static u8 obuf_post[3] = { 0x36, 3, 0 };
>
> -       mutex_lock(&state->stream_mutex);
> -       ret = dvbsky_usb_generic_rw(d, obuf_pre, 3, NULL, 0);
> +       mutex_lock(&d->usb_mutex);
> +       ret = dvb_usbv2_generic_rw_locked(d, obuf_pre, 3, NULL, 0);
>         if (!ret && onoff) {
>                 msleep(20);
> -               ret = dvbsky_usb_generic_rw(d, obuf_post, 3, NULL, 0);
> +               ret = dvb_usbv2_generic_rw_locked(d, obuf_post, 3, NULL, 0);
>         }
> -       mutex_unlock(&state->stream_mutex);
> +       mutex_unlock(&d->usb_mutex);
>         return ret;
>  }
>
> @@ -744,8 +742,6 @@ static int dvbsky_init(struct dvb_usb_device *d)
>         if (ret)
>                 return ret;
>         */
> -       mutex_init(&state->stream_mutex);
> -
>         state->last_lock = 0;
>
>         return 0;
>
>
>
> Thanks,
> Mauro

Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer

[Mauro Carvalho Chehab]

Em Sat, 28 Apr 2018 08:01:21 +0200
Olli Salonen <olli.salonen@iki.fi> escreveu:

> I did test the patch and while it doesn't seem to introduce any
> negative side effects it does not provide a remedy for the original
> problem that was seen after introducing
> 9d659ae14b545c4296e812c70493bfdc999b5c1c (you probably did not expect
> that either).

Ok, I'll apply it then. Having one less lock makes it cleaner.
> 
> 
> Cheers,
> -olli
> 
> On 27 April 2018 at 18:33, Mauro Carvalho Chehab
> <mchehab+samsung@kernel.org> wrote:
> > Em Fri, 27 Apr 2018 16:25:08 +0200
> > Olli Salonen <olli.salonen@iki.fi> escreveu:
> >
> >> Thanks for the suggestion Antti.
> >>
> >> I've tried to add a delay in various places, but haven't seen any
> >> improvement. However, what I did saw was that if I added an msleep
> >> after the lock:
> >>
> >> static int dvbsky_usb_generic_rw(struct dvb_usb_device *d,
> >>                 u8 *wbuf, u16 wlen, u8 *rbuf, u16 rlen)
> >> {
> >>         int ret;
> >>         struct dvbsky_state *state = d_to_priv(d);
> >>
> >>         mutex_lock(&d->usb_mutex);
> >>         msleep(20);
> >>
> >> The error was seen very within a minute. If I increased the msleep to
> >> 50, it failed within seconds. This doesn't seem to make sense to me.
> >> This is the only function where usb_mutex is used. If the mutex is
> >> held for a longer time, the next attempt to lock the mutex should just
> >> be delayed a bit, no?
> >
> > I don't like the idea of having two mutexes there to protect reading/writing
> > to data one for "generic" r/w ops, and another one just for streaming
> > control, with ends by calling the "generic" mutex.
> >
> > IMHO, I would get rid of one of the mutexes, e. g. something like the
> > patch below (untested).
> >
> > Regards,
> > Mauro
> >
> > media: dvbsky: use just one mutex for serializing device R/W ops
> >
> > Right now, there are two mutexes serializing r/w ops: one "generic"
> > and another one specifically for stream on/off.
> >
> > Clean it a little bit, getting rid of one of the mutexes.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> >
> > diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > index 43eb82884555..50553975c39d 100644
> > --- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > +++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
> > @@ -31,7 +31,6 @@ MODULE_PARM_DESC(disable_rc, "Disable inbuilt IR receiver.");
> >  DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
> >
> >  struct dvbsky_state {
> > -       struct mutex stream_mutex;
> >         u8 ibuf[DVBSKY_BUF_LEN];
> >         u8 obuf[DVBSKY_BUF_LEN];
> >         u8 last_lock;
> > @@ -68,18 +67,17 @@ static int dvbsky_usb_generic_rw(struct dvb_usb_device *d,
> >
> >  static int dvbsky_stream_ctrl(struct dvb_usb_device *d, u8 onoff)
> >  {
> > -       struct dvbsky_state *state = d_to_priv(d);
> >         int ret;
> > -       u8 obuf_pre[3] = { 0x37, 0, 0 };
> > -       u8 obuf_post[3] = { 0x36, 3, 0 };
> > +       static u8 obuf_pre[3] = { 0x37, 0, 0 };
> > +       static u8 obuf_post[3] = { 0x36, 3, 0 };
> >
> > -       mutex_lock(&state->stream_mutex);
> > -       ret = dvbsky_usb_generic_rw(d, obuf_pre, 3, NULL, 0);
> > +       mutex_lock(&d->usb_mutex);
> > +       ret = dvb_usbv2_generic_rw_locked(d, obuf_pre, 3, NULL, 0);
> >         if (!ret && onoff) {
> >                 msleep(20);
> > -               ret = dvbsky_usb_generic_rw(d, obuf_post, 3, NULL, 0);
> > +               ret = dvb_usbv2_generic_rw_locked(d, obuf_post, 3, NULL, 0);
> >         }
> > -       mutex_unlock(&state->stream_mutex);
> > +       mutex_unlock(&d->usb_mutex);
> >         return ret;
> >  }
> >
> > @@ -744,8 +742,6 @@ static int dvbsky_init(struct dvb_usb_device *d)
> >         if (ret)
> >                 return ret;
> >         */
> > -       mutex_init(&state->stream_mutex);
> > -
> >         state->last_lock = 0;
> >
> >         return 0;
> >
> >
> >
> > Thanks,
> > Mauro



Thanks,
Mauro