On Thu, Apr 18, 2024 at 11:06:27AM +0200, Geert Uytterhoeven wrote:
> Hi Biju,
>
> On Mon, Feb 19, 2024 at 7:05 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > Move request_irq() to rzg2l_cru_video_register(), in order to avoid
> > requesting IRQ during device start which happens frequently.
> >
> > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
>
> > @@ -1011,6 +1000,7 @@ void rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru)
> > {
> > media_device_unregister(&cru->mdev);
> > video_unregister_device(&cru->vdev);
> > + free_irq(cru->image_conv_irq, cru);
> > }
> >
> > int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> > @@ -1018,6 +1008,13 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> > struct video_device *vdev = &cru->vdev;
> > int ret;
> >
> > + ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq,
> > + IRQF_SHARED, KBUILD_MODNAME, cru);
> > + if (ret) {
> > + dev_err(cru->dev, "failed to request irq\n");
> > + return ret;
> > + }
> > +
> > if (video_is_registered(&cru->vdev)) {
>
> How can this happen? Perhaps rzg2l_cru_video_register() can be called
> multiple times through the rzg2l_cru_group_notify_complete() notifier?
The notifier completion handler shouldn't be called multiple times, no.
There's however a possibility (I think) that a subdev could disappear of
the device is unbound from its driver. If the device is later rebound,
the notifier completion handler could be called again.
The issue is that rzg2l_cru_video_unregister() is called from .remove().
I think a better fix would be to request the IRQ at probe time (or did
we discuss that previously and concluded it could cause issues ?). I
would also argue that the video devices should be registered at probe
time, not in the notifier completion handler.
> If that is true, the request_irq() should be moved after this block,
> just before the call to video_register_device() below.
>
> > struct media_entity *entity;
> >
> > @@ -1032,14 +1029,18 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> > ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> > if (ret) {
> > dev_err(cru->dev, "Failed to register video device\n");
> > - return ret;
> > + goto err_request_irq;
> > }
> >
> > ret = media_device_register(&cru->mdev);
> > - if (ret) {
> > - video_unregister_device(&cru->vdev);
> > - return ret;
> > - }
> > + if (ret)
> > + goto err_video_unregister;
> >
> > return 0;
> > +
> > +err_video_unregister:
> > + video_unregister_device(&cru->vdev);
> > +err_request_irq:
> > + free_irq(cru->image_conv_irq, cru);
> > + return ret;
> > }
--
Regards,
Laurent Pinchart
