Re: [PATCH v2 1/2] drm/dp: Add callbacks to make using DP AUX bus properly easier

[Doug Anderson]

Hi,

On Wed, May 4, 2022 at 3:41 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> > - We had forgotten a EXPORT_SYMBOL_GPL on the non "devm" populate
> >    function.
>
> This can go to a separate patch, so that the fix can be backported to
> earlier kernels. Please don't forget the Fixes: tag.

Sure. Will do for v3.


> > -EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_ep_devices);
> > +EXPORT_SYMBOL_GPL(of_dp_aux_depopulate_ep_device);
>
> Small note about the name. What if we change that to something more
> future-proof? Something like of_dp_aux_depopulate_bus() (and similarly
> rename other calls)?

Will do for v3.

> > +     /*
> > +      * If no parent "of_node", no "aux-bus" child node, or no available
> > +      * children then we're done. Call the callback (if needed) and return.
> > +      *
> > +      * NOTE: we intentionally pass the return code from done_probing
> > +      * directly out here. eDP controller drivers may want to support
> > +      * panels from old device trees where the panel was an independent
> > +      * platform device. In that case it's expected that done_probing()
> > +      * might need to return -EPROBE_DEFER to our caller.
> > +      */
> > +     if (!np) {
> > +             if (done_probing)
> > +                     return done_probing(aux);
>
> I see your point here (and that it makes code simpler). However I'm a
> little bit uneasy here. What if code this more explicitly in the
> drivers? Like the following:
>
> if (!dev_has_aux_bus()) {
>         ret = panel_ready(....);
> } else {
>         ...
>         ret = of_dp_aux_populate_ep_device(dp_aux, panel_ready);
>         ....;
> }

Yeah, I had considered that and was about 50-50. You think I should
change it? Is it really easier to understand if we break it up like
this? I'll wait for a response from you, but if I don't hear anything
then I'll change this as you suggest.


> This way you won't have to worry about the EPROBE_DEFER. Or you'd rather
> forbid it explicitly. Why? Consider the following scenario:
>
> dp_driver_probe()
>    /* This creates new devices */
>    done_probing returns -EPROBE_DEFER
>    /* device registration is unwound */
>    dp_driver_probe returns -EPROBE_DEFER
>
> However as the state of devices was chagned, the dp_driver_probe() can
> be called again and again, ending up with the the same probe loop that
> we are trying to solve.

Actually, I'm not sure we'd necessarily end up the loop we're trying
to solve. Let's see. If the panel probe itself doesn't create any
sub-devices and neither does done_probing() then done_probing()
returning -EPROBE_DEFER shouldn't cause any looping, right? It would
look just as if the panel returned -EPROBE_DEFER.

So I guess one could argue that _perhaps_ we don't need to forbid
-EPROBE_DEFER from done_probing()? It'd probably work OK (we'd
eventually retry probing the panel and call done_probing() once more
devices were added), but it'd be ugly and the system would report
(/sys/kernel/debug/devices_deferred) that it was the panel that
deferred even though it was this extra callback.

I'm going to go ahead and say this is too hacky, though. Also as long
as Linux still has the probe loop when you create devices and return
-EPROBE_DEFER we can get stuck because the panel _can_ create
sub-devices. It can do this with DP AUX backlight.

So I guess the summary is: yes, I'm confident that we should forbid
-EPROBE_DEFER from being returned by done_probing() when called by
dp_aux_ep_probe()

-Doug