Re: [PATCH] drm/irq: Add irq as false detection

[Daniel Vetter]

On Mon, Nov 2, 2020 at 1:40 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 02.11.20 um 13:09 schrieb Thomas Zimmermann:
> > Hi
> >
> > Am 02.11.20 um 12:19 schrieb Tian Tao:
> >> Add the detection of false for irq, so that the EINVAL is not
> >> returned when dev->irq_enabled is false.
> >>
> >> Signed-off-by: Tian Tao <tiantao6@xxxxxxxxxxxxx>
> >> ---
> >>  drivers/gpu/drm/drm_irq.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> >> index 09d6e9e..7537a3d 100644
> >> --- a/drivers/gpu/drm/drm_irq.c
> >> +++ b/drivers/gpu/drm/drm_irq.c
> >> @@ -172,6 +172,9 @@ int drm_irq_uninstall(struct drm_device *dev)
> >>      bool irq_enabled;
> >>      int i;
> >>
> >> +    if (!dev->irq_enabled || !dev)
> >> +            return 0;
> >> +
> >
> > Dereferencing a pointer before NULL-checking it, is Yoda programming. :)
> > I'd just drop the test for !dev and assume that dev is always valid.
> >
> > I suggest to change the int return value to void and return nothing.
> >
> > Re-reading the actual implementation of this function, this location
> > might be too early. Further below there already is a test for
> > irq_enabled. Put a drm_WARN_ON around it and it should be fine. This way
> > the vblank handlers will be disabled and erroneous callers will see a
> > warning in the kernel's log.
>
> In addition to these changes, you could also add a managed
> implementation of drm_irq_install(). The canonical name should be
> devm_drm_irq_install(). The function would call drm_irq_install() and
> register a cleanup handler via devm_add_action(). Example code is at [1].
>
> KMS drivers, such as hibmc, would then not have to bother about
> uninstalling the DRM irq.

Yup, devm_ is the right fix here imo, not trying to make the uninstall
hook resilient against drivers which can't keep track of stuff. So if
that's all there is, imo this patch is a bad idea, since we have
proper tools to make driver unloading easier on driver author's
nowadays.
-Daniel

> Best regards
> Thomas
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_drv.c#n664
>
>
> >
> > Best regards
> > Thomas
> >
> >>      irq_enabled = dev->irq_enabled;
> >>      dev->irq_enabled = false;
> >>
> >>
> >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch