Re: [PATCH v2 2/3] drm: Plumb debugfs_init through to panels

[Jani Nikula]

On Tue, 15 Feb 2022, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> Hi,
>
> On Tue, Feb 15, 2022 at 2:20 PM Andrzej Hajda <andrzej.hajda@xxxxxxxxx> wrote:
>>
>> On 15.02.2022 23:09, Javier Martinez Canillas wrote:
>> > Hello Doug,
>> >
>> > On 2/5/22 01:13, Douglas Anderson wrote:
>> >
>> > [snip]
>> >
>> >> +static void panel_bridge_debugfs_init(struct drm_bridge *bridge,
>> >> +                                  struct dentry *root)
>> >> +{
>> >> +    struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>> >> +    struct drm_panel *panel = panel_bridge->panel;
>> >> +
>> >> +    root = debugfs_create_dir("panel", root);
>> > This could return a ERR_PTR(-errno) if the function doesn't succeed.
>> >
>> > I noticed that most kernel code doesn't check the return value though...
>> >
>> >> +    if (panel->funcs->debugfs_init)
>> > Probably if (!(IS_ERR(root) && panel->funcs->debugfs_init) ?
>> >
>> >> +            panel->funcs->debugfs_init(panel, root);
>> >> +}
>> > [snip]
>> >
>> >> @@ -436,6 +436,9 @@ void drm_debugfs_connector_add(struct drm_connector *connector)
>> >>      /* vrr range */
>> >>      debugfs_create_file("vrr_range", S_IRUGO, root, connector,
>> >>                          &vrr_range_fops);
>> > Same here, wonder if the return value should be checked.
>
> My plan (confirmed with Javier over IRC) is to land my patches and we
> can address as needed with follow-up patches.
>
> I actually wrote said follow-up patches and they were ready to go, but
> when I was trying to come up with the right "Fixes" tag I found commit
> b792e64021ec ("drm: no need to check return value of debugfs_create
> functions"). So what's being requested is nearly the opposite of what
> Greg did there.
>
> I thought about perhaps only checking for directories but even that
> type of check was removed by Greg's patch. Further checking shows that
> start_creating() actually has:
>
> if (IS_ERR(parent))
>   return parent;
>
> ...so I guess that explains why it's fine to skip the check even for parents?
>
> Sure enough I confirmed that if I pass `ERR_PTR(-EINVAL)` as the root
> for `panel->funcs->debugfs_init()` that nothing bad seems to happen...

Yeah, the idea is that you don't need to check for debugfs function
return values and you can safely pass error pointers to debugfs
functions. The worst that can happen is you don't get the debugfs, but
hey, it's debugfs so you shouldn't fail anything else because of that
anyway.

BR,
Jani.

>
>
>> I've seen sometimes that file/dir was already created with the same
>> name, reporting error in such case will be helpful.
>
> It sure looks like start_creating() already handles that type of
> reporting... Sure enough, I tried to create the "force" file twice,
> adding no error checking myself, and I see:
>
> debugfs: File 'force' in directory 'eDP-1' already present!
> debugfs: File 'force' in directory 'DP-1' already present!
>
>
> So tl;dr is that I'm going to land the patches and now am _not_
> planning on doing followup patches. However, if I'm confused about any
> of the above then please let me know and I'll dig more / can send
> follow-up patches.
>
> -Doug

-- 
Jani Nikula, Intel Open Source Graphics Center