Re: [REGRESSION/bisected] kernel panic after "rmmod cx23885" by "si2157: implement signal strength stats"

[Antti Palosaari <crope@iki.fi>]

On 01/11/2016 12:04 AM, Ernst Martin Witte wrote:
> Hi Antti!
>
> Thx for  your quick answer. Since  I'm not familiar with  linux kernel
> driver  internals,  may  I  ask  few  questions  in  order  to  better
> understand the  intended use  of ..._delayed_work and  the interaction
> between drivers like the cx23885 and the tuner modules?
>
> - Is si2157_sleep not related to "suspended" system power state but to
>    an "unused" state of the tuner physical device?

It is physical chip state where is is not used.

> - Which  mechanism  should  call  si2157_sleep when  e.g.  cx23885  is
>    rmmod'ed?

It is dvb_frontend. You should not be able to remove cx23885 module 
until dvb frontend is stopped. dvb frontend runs thread in order to call 
init, sleep etc.

> - What  I  understood   so  far  is  that  we  should   not  need  the
>    cance_delayed_work_sync in ..._remove  because ..._sleep should have
>    been called before.  Why is  such a cancel_delayed_work call already
>    in  the  kernel  in  rtl2832_remove although  this  module  has  the
>    separate cancel_delayed_work call in rtl23832_sleep?

Look it again. There is 2 different delayed works.

> - If si2157_sleep  was not called and  the device is considered  to be
>    still   active,  which   mechanism  should   prevent  the   call  of
>    si2157_remove or the unloading of module cx23885?

cx23885 should ref count si2157. I think dvb frontend ref counts cx23885.

> - Is it correct that rmmod cx23885  is legitime if none of its devices
>    is in use by any app and if those "links" between si2157 and cx23885
>    are  shut down  correctly?  ...   and  should only  be prevented  if
>    something goes wrong with shutting down those "links"?
>
> - Is the  interpretation correct  that si2157_remove should  be called
>    when the  driver/module si2157  is unloaded?  If yes  - how  does it
>    happen that it gets called when only an rmmod cx23885 is done?

It should be called. cx23885 module releases si2157, which makes driver 
core to call si2157_remove in order to free all resources driver has.

There is 2 different kind of driver ops: one for DVB core and one for 
(I2C) driver core. si2157_remove is driver core op and si2157_sleep is 
coming from DVB core.

> - What    is   the    intended    call   sequence    of   all    those
>    sleep/remove/... callbacks when
>
>    a) an applications closes the device (but driver remains loaded, but
>       presumingly inactive)
>
>    b) the  one or other  module is  rmmod'ed (for me  personally mainly
>       cx23885)

Those dvb core callbacks are higher level and ones from struct 
i2c_driver are lower level. So in order to shutdown device dvb callbacks 
should be called first and after that i2c remove. After I2C remove none 
of callbacks are not valid anymore.

>
> ...  you might  see that  for me  it's mainly  unclear how  si2157 and
> cx23885 are linked in terms of callbacks.
>
> Is there anything  I could try (adding debug statements,  etc.) / test
> in order to get more insight what's going wrong?

Sure, just add manually some printing to see where code is running. 
That's how I also do.

You should examine when dvb core calls sleep and test it is not possible 
to remove cx23885 module until that. And if sleep is not called, why it 
is not called. IIRC there is some module param to prevent dvb frontend 
shutdown, but it should not be used (I think it is left there to 
workaround some bug).

Why you even want to remove whole module? Usually you only remove 
modules manually when you are developing the driver.

regards
Antti

>
> BR and many thanks for your help,
>    Martin
>
>
> On 10.01.2016 05:43, Antti Palosaari wrote:
>> Hello
>> Those timers are activated on frontend init() ja de-activated on sleep().
>> Removing active driver does not sound good and IMHO it should not be even
>> possible. I think it should be find out why it is possible to remove whole
>> driver before it is put to sleep().
>>
>> Antti
>>
>>
>> On 01/09/2016 11:42 PM, Ernst Martin Witte wrote:
>>> Hi again,
>>>
>>> seems  that the  cause  for the  kernel  panic is  a  missing call  to
>>> cancel_delayed_work_sync in  si2157_remove before  the call  to kfree.
>>> After adding cancel_delayed_work_sync(&dev->stat_work), rmmod does not
>>> trigger the kernel panic any more.
>>>
>>> However, very similar issues could be identified also in other modules:
>>>
>>>     ts2020
>>>     af9013
>>>     af9033
>>>     rtl2830
>>>
>>> when looking in drivers/media/tuners and drivers/media/dvb-frontends.
>>>
>>> Therefore,  the submitted  patch  set contains  fixes  also for  those
>>> modules. The submitted patch set is:
>>>
>>>     [PATCH 0/5] [media] cancel_delayed_work_sync before device removal / kfree
>>>
>>> I hope these patches completely fix the issue and are ok for inclusion
>>> in the kernel.
>>>
>>> BR and thx,
>>>     Martin
>>> --
>>> 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
>>>
>>
>> --
>> 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
>>

-- 
http://palosaari.fi/