Re: [PATCH] media: m88ds3103: serialize reset messages in m88ds3103_set_frontend

From: Antti Palosaari <crope@iki.fi>
To: James Hutchinson <jahutchinson99@googlemail.com>
Cc: linux-media@vger.kernel.org
Subject: Re: [PATCH] media: m88ds3103: serialize reset messages in m88ds3103_set_frontend
Date: Tue, 22 Jan 2019 20:17:33 +0200	[thread overview]
Message-ID: <e2db6e2e-75d0-6d08-a81f-ae73a1e711cc@iki.fi> (raw)
In-Reply-To: <20190122110810.l2zmvyepwswfv3bl@vero4k>

On 1/22/19 1:08 PM, James Hutchinson wrote:
> On Sun, Jan 20, 2019 at 04:43:08PM +0200, Antti Palosaari wrote:
>> On 1/13/19 11:13 PM, James Hutchinson wrote:
>>> Ref: https://bugzilla.kernel.org/show_bug.cgi?id=199323
>>>
>>> Users are experiencing problems with the DVBSky S960/S960C USB devices
>>> since the following commit:
>>>
>>> 9d659ae: ("locking/mutex: Add lock handoff to avoid starvation")
>>>
>>> The device malfunctions after running for an indeterminable period of
>>> time, and the problem can only be cleared by rebooting the machine.
>>>
>>> It is possible to encourage the problem to surface by blocking the
>>> signal to the LNB.
>>>
>>> Further debugging revealed the cause of the problem.
>>>
>>> In the following capture:
>>> - thread #1325 is running m88ds3103_set_frontend
>>> - thread #42 is running ts2020_stat_work
>>>
>>> a> [1325] usb 1-1: dvb_usb_v2_generic_io: >>> 08 68 02 07 80
>>>      [1325] usb 1-1: dvb_usb_v2_generic_io: <<< 08
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: >>> 09 01 01 68 3f
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: <<< 08 ff
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: >>> 08 68 02 03 11
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: <<< 07
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: >>> 09 01 01 60 3d
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: <<< 07 ff
>>> b> [1325] usb 1-1: dvb_usb_v2_generic_io: >>> 08 68 02 07 00
>>>      [1325] usb 1-1: dvb_usb_v2_generic_io: <<< 07
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: >>> 08 68 02 03 11
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: <<< 07
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: >>> 09 01 01 60 21
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: <<< 07 ff
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: >>> 08 68 02 03 11
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: <<< 07
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: >>> 09 01 01 60 66
>>>      [42] usb 1-1: dvb_usb_v2_generic_io: <<< 07 ff
>>>      [1325] usb 1-1: dvb_usb_v2_generic_io: >>> 08 68 02 03 11
>>>      [1325] usb 1-1: dvb_usb_v2_generic_io: <<< 07
>>>      [1325] usb 1-1: dvb_usb_v2_generic_io: >>> 08 60 02 10 0b
>>>      [1325] usb 1-1: dvb_usb_v2_generic_io: <<< 07
>>>
>>> Two i2c messages are sent to perform a reset in m88ds3103_set_frontend:
>>>
>>>     a. 0x07, 0x80
>>>     b. 0x07, 0x00
>>>
>>> However, as shown in the capture, the regmap mutex is being handed over
>>> to another thread (ts2020_stat_work) in between these two messages.
>>>
>>>>  From here, the device responds to every i2c message with an 07 message,
>>> and will only return to normal operation following a power cycle.
>>>
>>> Use regmap_multi_reg_write to group the two reset messages, ensuring
>>> both are processed before the regmap mutex is unlocked.
>>
>> I tried to reproduce that issue with pctv 461e, which has em28xx
>> usb-interface, but without success. Even when I added some sleep between
>> reset commands and increased tuner statistic polling interval such that it
>> polls all the time, it works correctly. Device has tuner is connected to
>> demod i2c bus, which I think is same for your device (it calls demod i2c mux
>> select for every tuner i2c access).
>>
>> Taking into account tests I made it is probably issue with usb-interface i2c
>> adapter instead - for some reason it stops working and starts returning 07
>> error all the time. Did any other I2C command succeed after failure? I mean
>> is there any other i2c client on that bus you could test if it fails too on
>> error situation?
>>
>> All in all, fix should be done to usb-interface i2c adapter if possible
>> unless it has proven issue is somewhere else. You could try to add some
>> sleep or repeat to i2c adapter in order to see if it helps.
>>
>> regards
>> Antti
>>
>> -- 
>> http://palosaari.fi/
> 
> Thanks for taking the time to review my patch.
> 
> My device is the dvbsky usb s960 which is a pretty popular device and hasn't
> been working for several users since commit 9d659ae.
> 
> I did some further investigation and can now see that the issue likely only
> affects adapters which use the m88ds3103_get_agc_pwm function to get the AGC
> from the demodulator as part of ts2020_stat_work.
> 
> This is the 3f message in my original capture, which gets an ff response.
>      [42] usb 1-1: dvb_usb_v2_generic_io: >>> 09 01 01 68 3f
>      [42] usb 1-1: dvb_usb_v2_generic_io: <<< 08 ff
> 
> The m88ds3103_get_agc_pwm function looks to be used by a subset of devices and
> their variants from the dvbsky usb-interface (s960 & s960c), and the cx23885-dvb
> pci-interface (s950, s950c, s952).
> 
> The problem does NOT occur if I disable auto-gain correction by removing the
> following line from dvbsky_s960_attach:
> 
>      ts2020_config.get_agc_pwm = m88ds3103_get_agc_pwm;
> 
> I then have the same experience as you; I can add a sleep between the reset
> commands and increase the tuner statistic polling interval, and it still
> works correctly.
> 
> I can also reproduce the issue on older kernels (pre-commit 9d659ae) by adding
> a sleep between the two reset commands and leaving the agc read enabled.
> 
> Whilst my original patch works around the issue, I'm not sure it's really
> addressing the root cause, and I do wonder whether other areas of the m88ds3103
> module may end up needing to be protected in a similar way.
> 
> Afterall, the ts2020 stat work thread runs every 2000ms, and there's currently
> no guarantee what state the demodulator is going to be in at that time.

Now I can reproduce the issue. It is easy to just add read reg 0x3f 
between reset and it starts failing. And I tested some 100ms sleeps 
there too to leave some time for settle reset, but it does not help. 
Denying any i2c access during reset sounds correct solution.

Anyhow, just to be clear in my understanding locks here are:

regmap_write()
-> demod regmap lock
--> i2c adapter lock
**i2c access here
<-- i2c adapter unlock
<- demod regmap lock

regmap_multi_reg_write()
-> demod regmap lock
--> i2c adapter lock
**i2c access here
<-- i2c adapter unlock
--> i2c adapter lock
**i2c access here
<-- i2c adapter unlock
<- demod regmap lock

So that use regmap_multi_reg_write() prevents any other reg access to 
that device withing demod regmap lock context and fixes issue.

Patch is valid:
Reviewed-by: Antti Palosaari <crope@iki.fi>

regards
Antti

-- 
http://palosaari.fi/