Re: Issue with cec_register_adapter calling request_module() from an async context when called from intel_dp_detect

[Hans de Goede <hdegoede@redhat.com>]

Hi,

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 ...

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.

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.

Regards,

Hans