Hi Laurent,
Thanks for the feedback.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Monday, August 26, 2024 10:43 AM
> Subject: Re: [PATCH v3] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to probe()
>
> On Mon, Aug 26, 2024 at 08:08:33AM +0000, Biju Das wrote:
> > On Monday, August 26, 2024 8:27 AM, claudiu beznea wrote:
> > > On 24.08.2024 21:21, Biju Das wrote:
> > > > Move request_irq() to probe(), in order to avoid requesting IRQ
> > > > during device start which happens frequently. As this function is
> > > > in probe(), it is better to replace it with its devm variant for
> > > > managing the resource efficiently.
> > > >
> > > > Suggested-by: Laurent Pinchart
> > > > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > > > Reviewed-by: Laurent Pinchart
> > > > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > ---
> > > > v2->v3:
> > > > * Dropped wrapper function rzg2l_cru_process_irq() and made
> > > > rzg2l_cru_irq() global.
> > > > * Added Rb tag from Laurent.
> > > > v1->v2:
> > > > * Updated commit header and description.
> > > > * Moved rzg2l_cru_irq from rzg2l-video.c->rzg2l-core.c and introduced
> > > > rzg2l_cru_process_irq() in video.c to process irq.
> > > > * Dropped image_conv_irq from struct rzg2l_cru_dev
> > > > * Replaced request_irq with its devm variant.
> > > > ---
> > > > .../media/platform/renesas/rzg2l-cru/rzg2l-core.c | 13
> > > > +++++++++---- .../media/platform/renesas/rzg2l-cru/rzg2l-cru.h | 6 ++----
> > > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 15 ++-------------
> > > > 3 files changed, 13 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > index 280efd2a8185..2a2907beb722 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > > > @@ -242,7 +242,7 @@ static int rzg2l_cru_media_init(struct
> > > > rzg2l_cru_dev *cru) static int rzg2l_cru_probe(struct
> > > > platform_device *pdev) {
> > > > struct rzg2l_cru_dev *cru;
> > > > - int ret;
> > > > + int irq, ret;
> > > >
> > > > cru = devm_kzalloc(&pdev->dev, sizeof(*cru), GFP_KERNEL);
> > > > if (!cru)
> > > > @@ -270,9 +270,14 @@ static int rzg2l_cru_probe(struct platform_device *pdev)
> > > > cru->dev = &pdev->dev;
> > > > cru->info = of_device_get_match_data(&pdev->dev);
> > > >
> > > > - cru->image_conv_irq = platform_get_irq(pdev, 0);
> > > > - if (cru->image_conv_irq < 0)
> > > > - return cru->image_conv_irq;
> > > > + irq = platform_get_irq(pdev, 0);
> > > > + if (irq < 0)
> > > > + return irq;
> > > > +
> > > > + ret = devm_request_irq(&pdev->dev, irq, rzg2l_cru_irq, IRQF_SHARED,
> > > > + KBUILD_MODNAME, cru);
> > >
> > > Because this is requested w/ IRQF_SHARED the free_irq() ->
> > > __free_irq() [1] will call the IRQ handler to simulate an IRQ SHARE
> > > scenario where other device generate an interrupt.
>
> Good point, I had missed that.
>
> > Currently CSI driver is not registered any interrupts and CRU is the single user.
>
> Regardless, the fact that the IRQ is requested with IRQF_SHARED means that the IRQ handler needs to be
> prepared to be called at any time from the point of registration to the point the IRQ is freed. This
> is tested by CONFIG_DEBUG_SHIRQ=y, which you should enable for testing.
For single user, testing CONFIG_DEBUG_SHIRQ=y this does not make any sense. See [1]
[1] https://elixir.bootlin.com/linux/v6.11-rc5/source/kernel/irq/manage.c#L1965
>
> If you don't need to share the interrupt with any other device, you can drop the IRQF_SHARED.
I will drop IRQF_SHARED flags instead as there is single user
and will send RFC for dropping calling IRQ handler for single user
with CONFIG_DEBUG_SHIRQ=y
Please let me know is it fine for you?
Cheers,
Biju
> Otherwise, you will need to fix the issue properly. You can probably wrap the interrupt handling with
> pm_runtime_get_if_in_use() and pm_runtime_put() (hoping those functions can be called from interrupt
> context).
>
> On a side note, I also I wonder if this issue precludesusage of
> devm_request_irq() for shared interrupts, requiring calling free_irq() manually at remove time to
> control the sequence of cleanup operations.
>
> > > > + if (ret)
> > > > + return dev_err_probe(&pdev->dev, ret, "failed to request
> > > > +irq\n");
> > > >
> > > > platform_set_drvdata(pdev, cru);
> > > >
> > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > index a5a99b004322..174760239548 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > > > @@ -8,6 +8,7 @@
> > > > #ifndef __RZG2L_CRU__
> > > > #define __RZG2L_CRU__
> > > >
> > > > +#include <linux/irqreturn.h>
> > > > #include <linux/reset.h>
> > > >
> > > > #include <media/v4l2-async.h>
> > > > @@ -68,8 +69,6 @@ struct rzg2l_cru_ip {
> > > > *
> > > > * @vclk: CRU Main clock
> > > > *
> > > > - * @image_conv_irq: Holds image conversion interrupt number
> > > > - *
> > > > * @vdev: V4L2 video device associated with CRU
> > > > * @v4l2_dev: V4L2 device
> > > > * @num_buf: Holds the current number of buffers enabled
> > > > @@ -105,8 +104,6 @@ struct rzg2l_cru_dev {
> > > >
> > > > struct clk *vclk;
> > > >
> > > > - int image_conv_irq;
> > > > -
> > > > struct video_device vdev;
> > > > struct v4l2_device v4l2_dev;
> > > > u8 num_buf;
> > > > @@ -141,6 +138,7 @@ void rzg2l_cru_dma_unregister(struct
> > > > rzg2l_cru_dev *cru);
> > > >
> > > > int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru); void
> > > > rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru);
> > > > +irqreturn_t rzg2l_cru_irq(int irq, void *data);
> > > >
> > > > const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32
> > > > format);
> > > >
> > > > diff --git
> > > > a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > index b16b8af6e8f8..e80bfb9fc1af 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > @@ -527,7 +527,7 @@ static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru)
> > > > rzg2l_cru_set_stream(cru, 0);
> > > > }
> > > >
> > > > -static irqreturn_t rzg2l_cru_irq(int irq, void *data)
> > > > +irqreturn_t rzg2l_cru_irq(int irq, void *data)
> > > > {
> > > > struct rzg2l_cru_dev *cru = data;
> > > > unsigned int handled = 0;
> > > > @@ -637,13 +637,6 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int
> count
> > > > goto assert_aresetn;
> > > > }
> > > >
> > > > - 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");
> > > > - goto assert_presetn;
> > > > - }
> > > > -
> > > > /* Allocate scratch buffer. */
> > > > cru->scratch = dma_alloc_coherent(cru->dev, cru->format.sizeimage,
> > > > &cru->scratch_phys, GFP_KERNEL); @@ -651,7 +644,7 @@
> > > > static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
> > > > return_unused_buffers(cru, VB2_BUF_STATE_QUEUED);
> > > > dev_err(cru->dev, "Failed to allocate scratch buffer\n");
> > > > ret = -ENOMEM;
> > > > - goto free_image_conv_irq;
> > > > + goto assert_presetn;
> > > > }
> > > >
> > > > cru->sequence = 0;
> > > > @@ -670,9 +663,6 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int
> count
> > > > if (ret)
> > > > dma_free_coherent(cru->dev, cru->format.sizeimage, cru->scratch,
> > > > cru->scratch_phys);
> > > > -free_image_conv_irq:
> > > > - free_irq(cru->image_conv_irq, cru);
> > > > -
> > > > assert_presetn:
> > > > reset_control_assert(cru->presetn);
> > > >
> > > > @@ -698,7 +688,6 @@ static void rzg2l_cru_stop_streaming_vq(struct vb2_queue *vq)
> > > > dma_free_coherent(cru->dev, cru->format.sizeimage,
> > > > cru->scratch, cru->scratch_phys);
> > > >
> > > > - free_irq(cru->image_conv_irq, cru);
> > > > return_unused_buffers(cru, VB2_BUF_STATE_ERROR);
> > > >
> > > > reset_control_assert(cru->presetn);
>
> --
> Regards,
>
> Laurent Pinchart
