[Intel-gfx] Issue with cec_register_adapter calling request_module() from an async context when called from intel_dp_detect

[Hans de Goede]

Hi,

On 2/18/21 5:38 PM, Sean Young wrote:
> Hi Hans,
> 
> On Thu, Feb 18, 2021 at 04:33:38PM +0100, Hans de Goede wrote:
>> On 2/17/21 5:29 PM, Hans Verkuil wrote:
>>> On 17/02/2021 16:11, Sean Young wrote:
>>>> Hi,
>>>>
>>>> On Wed, Feb 17, 2021 at 04:04:11PM +0100, Hans de Goede wrote:
>>>>> On 2/17/21 3:32 PM, Sean Young wrote:
>>>>>> On Wed, Feb 17, 2021 at 01:41:46PM +0100, Hans Verkuil wrote:
>>>>>>> Hi Hans,
>>>>>>>
>>>>>>> On 17/02/2021 13:24, Hans de Goede wrote:
>>>>>>>> <resend with the linux-media list added to the Cc>
>>>>>>>>
>>>>>>>> Hi Hans,
>>>>>>>>
>>>>>>>> Fedora has a (opt-in) system to automatically collect backtraces from software
>>>>>>>> crashing on users systems.
>>>>>>>>
>>>>>>>> This includes collecting kernel backtraces (including once triggered by
>>>>>>>> WARN macros) while looking a the top 10 of the most reported backtrace during the
>>>>>>>> last 2 weeks report from ABRT: https://retrace.fedoraproject.org/faf/problems/
>>>>>>>>
>>>>>>>> I noticed the following backtrace:
>>>>>>>> https://retrace.fedoraproject.org/faf/problems/8150/
>>>>>>>> which has been reported 170000 times by Fedora users who have opted-in during the
>>>>>>>> last 14 days.
>>>>>>>>
>>>>>>>> The issue here is that cec_register_adapter ends up calling request_module()
>>>>>>>> from an async context, triggering this warn in kernel/kmod.c __request_module():
>>>>>>>>
>>>>>>>>         /*
>>>>>>>>          * We don't allow synchronous module loading from async.  Module
>>>>>>>>          * init may invoke async_synchronize_full() which will end up
>>>>>>>>          * waiting for this task which already is waiting for the module
>>>>>>>>          * loading to complete, leading to a deadlock.
>>>>>>>>          */
>>>>>>>>         WARN_ON_ONCE(wait && current_is_async());
>>>>>>>>
>>>>>>>> The call-path leading to this goes like this:
>>>>>>>>
>>>>>>>>  ? kvasprintf+0x6d/0xa0
>>>>>>>>  ? kobject_set_name_vargs+0x6f/0x90
>>>>>>>>  rc_map_get+0x30/0x60
>>>>>>>
>>>>>>> It's not CEC, it is rc_map_get that calls request_module() for rc-cec.ko.
>>>>>>>
>>>>>>> I've added Sean Young to the CC list.
>>>>>>>
>>>>>>> Sean, is it possible to treat rc-cec as a built-in if MEDIA_CEC_RC is set?
>>>>>>>
>>>>>>> I think this issue is very specific to CEC. I would not expect to see this
>>>>>>> with any other rc keymap.
>>>>>>
>>>>>> So CEC creates an RC device with a keymap (cec keymap, of course) and then
>>>>>> the keymap needs to be loaded. We certainly don't want all keymaps as
>>>>>> builtins, that would be a waste.
>>>>>>
>>>>>> The cec keymap is scanned once to build a map from cec codes to linux
>>>>>> keycodes; making it builtin is not ideal, and makes the build system a
>>>>>> bit messy.
>>>>>>
>>>>>> I don't think we can load the keymap later, user space may start remapping
>>>>>> the keymap from udev.
>>>>>>
>>>>>> Possibly we could create the cec or rc device later but this could be a bit
>>>>>> messy.
>>>>>>
>>>>>> Could CEC specify:
>>>>>>
>>>>>> #if IS_ENABLED(CONFIG_MEDIA_CEC_RC)
>>>>>> MODULE_SOFTDEP("rc-cec")
>>>>>> #endif
>>>>>
>>>>> That would need to be:
>>>>>
>>>>> MODULE_SOFTDEP("pre: rc-cec")
>>>>>
>>>>> I see that the drm_kms_helper and i915 drivers both depend on the cec module already,
>>>>> so yes if that module will request for rc-cec to be loaded before it is loaded
>>>>> (and thus before i915 is loaded) then that should work around this.
>>>>>
>>>>> Assuming the user is using a module-loader which honors the softdep...
>>>>>
>>>>> Also this assumes that rc_map_get is smart enough to not call request_module()
>>>>> if the module is already loaded, is that the case ?
>>>>
>>>> Yes, see rc_map_get().
>>>
>>> I tried this. It works if CONFIG_RC_CORE is set to m, but setting it to
>>> y resulted in the same problem. It looks like MODULE_SOFTDEP only works if rc_main
>>> is a module as well.
>>
>> Yeah that is a known limit of module softdeps, they only work inside modules ...
> 
> Yes, I assume this is the problem.
> 
>> Still, assuming there is no easy other fix, we could still use this somehow.
>>
>> I do see that at least Fedora actually has CONFIG_RC_CORE=y for some reason.
> 
> This is to make BPF IR decoding possible.
> 
>> I guess we could maybe add the softdep to the CONFIG_RC_MAP module or
>> maybe to the module which contains the code enabled by CONFIG_DRM_DP_CEC ?
>>
>> At least Fedora has all drm stuff as modules and also has CONFIG_RC_MAP=m,
>>
>> I know this is not a real fix but a workaround to get rid of 170,000
>> backtraces / 14 days being reported by (opted-in) systems running the
>> Fedora generic kernel config would be welcome regardless of it being the
>> "perfect" fix.
> 
> Of course, I totally agree that a solution is needed.
> 
> How about:
> 
>  1) Use MODULE_SOFTDEP("rc-cec"); 
> 
>  2) If it's compiled as a module, rc-cec should be builtin

I assume with 2. you meant to write "If it is NOT compiled as a module, ..." ?

That sounds good to me. The Kconfig for this likely won't be pretty, but
that is often the case with more complex dependencies in Kconfig.

Note with my Fedora hat (Fedora fedora :) on I'm already happy with just
1) assuming it ends up somewhere which the Fedora kernel-config does
built as a module, such as say the module which contains the code
which gets enabled by CONFIG_DRM_DP_CEC.  But having 2. also would
certainly be nice.

Regards,

Hans