* Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer @ 2018-04-04 11:41 Olli Salonen 2018-04-08 17:44 ` Olli Salonen 2018-04-09 9:14 ` Peter Zijlstra 0 siblings, 2 replies; 9+ messages in thread From: Olli Salonen @ 2018-04-04 11:41 UTC (permalink / raw) To: Nibble Max, peterz; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer 2018-04-04 11:41 Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer Olli Salonen @ 2018-04-08 17:44 ` Olli Salonen 2018-04-09 9:14 ` Peter Zijlstra 1 sibling, 0 replies; 9+ messages in thread From: Olli Salonen @ 2018-04-08 17:44 UTC (permalink / raw) To: Nibble Max, peterz; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer 2018-04-04 11:41 Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer Olli Salonen 2018-04-08 17:44 ` Olli Salonen @ 2018-04-09 9:14 ` Peter Zijlstra 2018-04-18 4:49 ` Olli Salonen 1 sibling, 1 reply; 9+ messages in thread From: Peter Zijlstra @ 2018-04-09 9:14 UTC (permalink / raw) To: Olli Salonen; +Cc: Nibble Max, linux-media, mchehab, wsa 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer 2018-04-09 9:14 ` Peter Zijlstra @ 2018-04-18 4:49 ` Olli Salonen 2018-04-18 8:49 ` Antti Palosaari 0 siblings, 1 reply; 9+ messages in thread From: Olli Salonen @ 2018-04-18 4:49 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Nibble Max, linux-media, Mauro Carvalho Chehab, wsa 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. > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer 2018-04-18 4:49 ` Olli Salonen @ 2018-04-18 8:49 ` Antti Palosaari 2018-04-27 14:25 ` Olli Salonen 0 siblings, 1 reply; 9+ messages in thread From: Antti Palosaari @ 2018-04-18 8:49 UTC (permalink / raw) To: Olli Salonen, Peter Zijlstra Cc: Nibble Max, linux-media, Mauro Carvalho Chehab, wsa 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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer 2018-04-18 8:49 ` Antti Palosaari @ 2018-04-27 14:25 ` Olli Salonen 2018-04-27 16:33 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 9+ messages in thread From: Olli Salonen @ 2018-04-27 14:25 UTC (permalink / raw) To: Antti Palosaari Cc: Peter Zijlstra, Nibble Max, linux-media, Mauro Carvalho Chehab, wsa 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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer 2018-04-27 14:25 ` Olli Salonen @ 2018-04-27 16:33 ` Mauro Carvalho Chehab 2018-04-28 6:01 ` Olli Salonen 0 siblings, 1 reply; 9+ messages in thread From: Mauro Carvalho Chehab @ 2018-04-27 16:33 UTC (permalink / raw) To: Olli Salonen Cc: Antti Palosaari, Peter Zijlstra, Nibble Max, linux-media, wsa 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer 2018-04-27 16:33 ` Mauro Carvalho Chehab @ 2018-04-28 6:01 ` Olli Salonen 2018-05-10 11:04 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 9+ messages in thread From: Olli Salonen @ 2018-04-28 6:01 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Antti Palosaari, Peter Zijlstra, Nibble Max, linux-media, wsa 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer 2018-04-28 6:01 ` Olli Salonen @ 2018-05-10 11:04 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 9+ messages in thread From: Mauro Carvalho Chehab @ 2018-05-10 11:04 UTC (permalink / raw) To: Olli Salonen Cc: Antti Palosaari, Peter Zijlstra, Nibble Max, linux-media, wsa 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-05-10 11:04 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-04 11:41 Regression: DVBSky S960 USB tuner doesn't work in 4.10 or newer Olli Salonen 2018-04-08 17:44 ` Olli Salonen 2018-04-09 9:14 ` Peter Zijlstra 2018-04-18 4:49 ` Olli Salonen 2018-04-18 8:49 ` Antti Palosaari 2018-04-27 14:25 ` Olli Salonen 2018-04-27 16:33 ` Mauro Carvalho Chehab 2018-04-28 6:01 ` Olli Salonen 2018-05-10 11:04 ` Mauro Carvalho Chehab
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.