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

[Sean Young <sean@mess.org>]

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

Thanks,

Sean