* [PATCH] media: rzg2l-cru: Replace usleep_range with udelay @ 2025-12-02 16:08 Tommaso Merciai 2025-12-03 2:35 ` Laurent Pinchart 0 siblings, 1 reply; 4+ messages in thread From: Tommaso Merciai @ 2025-12-02 16:08 UTC (permalink / raw) To: tomm.merciai Cc: linux-renesas-soc, biju.das.jz, Tommaso Merciai, Mauro Carvalho Chehab, Laurent Pinchart, Hans Verkuil, Lad Prabhakar, Daniel Scally, Jacopo Mondi, linux-media, linux-kernel, Khai Nguyen, Hao Bui `usleep_range()` should not be used in atomic contexts like between spin_lock_irqsave()/spin_lock_irqrestore() pair inside function rzg2l_cru_stop_image_processing(). That may cause scheduling while atomic bug. Signed-off-by: Khai Nguyen <khai.nguyen.wx@renesas.com> Signed-off-by: Hao Bui <hao.bui.yg@renesas.com> Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> --- drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c index 162e2ace6931..1355bfd186d4 100644 --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c @@ -369,7 +369,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru) if (cru->info->fifo_empty(cru)) break; - usleep_range(10, 20); + udelay(10); } /* Notify that FIFO is not empty here */ @@ -385,7 +385,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru) AMnAXISTPACK_AXI_STOP_ACK) break; - usleep_range(10, 20); + udelay(10); } /* Notify that AXI bus can not stop here */ -- 2.43.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] media: rzg2l-cru: Replace usleep_range with udelay 2025-12-02 16:08 [PATCH] media: rzg2l-cru: Replace usleep_range with udelay Tommaso Merciai @ 2025-12-03 2:35 ` Laurent Pinchart 2025-12-03 8:45 ` Tommaso Merciai 0 siblings, 1 reply; 4+ messages in thread From: Laurent Pinchart @ 2025-12-03 2:35 UTC (permalink / raw) To: Tommaso Merciai Cc: tomm.merciai, linux-renesas-soc, biju.das.jz, Mauro Carvalho Chehab, Hans Verkuil, Lad Prabhakar, Daniel Scally, Jacopo Mondi, linux-media, linux-kernel, Khai Nguyen, Hao Bui Hi Tommaso, Thank you for the patch. On Tue, Dec 02, 2025 at 05:08:41PM +0100, Tommaso Merciai wrote: > `usleep_range()` should not be used in atomic contexts like between > spin_lock_irqsave()/spin_lock_irqrestore() pair inside function > rzg2l_cru_stop_image_processing(). That may cause scheduling while > atomic bug. > > Signed-off-by: Khai Nguyen <khai.nguyen.wx@renesas.com> > Signed-off-by: Hao Bui <hao.bui.yg@renesas.com> > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> > --- > drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > index 162e2ace6931..1355bfd186d4 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > @@ -369,7 +369,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru) > if (cru->info->fifo_empty(cru)) > break; > > - usleep_range(10, 20); > + udelay(10); There's an instance of msleep() earlier in this function, surrounded by spin_unlock_irqrestore() and spin_lock_irqsave(). I wondered if we should do the same here, but that lead to a second question: why does the driver need to cover the whole stop procedure with a spinlock in the first place ? > } > > /* Notify that FIFO is not empty here */ > @@ -385,7 +385,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru) > AMnAXISTPACK_AXI_STOP_ACK) > break; > > - usleep_range(10, 20); > + udelay(10); > } > > /* Notify that AXI bus can not stop here */ -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: rzg2l-cru: Replace usleep_range with udelay 2025-12-03 2:35 ` Laurent Pinchart @ 2025-12-03 8:45 ` Tommaso Merciai 2025-12-03 10:55 ` David Laight 0 siblings, 1 reply; 4+ messages in thread From: Tommaso Merciai @ 2025-12-03 8:45 UTC (permalink / raw) To: Laurent Pinchart Cc: tomm.merciai, linux-renesas-soc, biju.das.jz, Mauro Carvalho Chehab, Hans Verkuil, Lad Prabhakar, Daniel Scally, Jacopo Mondi, linux-media, linux-kernel, Khai Nguyen, Hao Bui Hi Laurent, Thanks for your review. On Wed, Dec 03, 2025 at 11:35:52AM +0900, Laurent Pinchart wrote: > Hi Tommaso, > > Thank you for the patch. > > On Tue, Dec 02, 2025 at 05:08:41PM +0100, Tommaso Merciai wrote: > > `usleep_range()` should not be used in atomic contexts like between > > spin_lock_irqsave()/spin_lock_irqrestore() pair inside function > > rzg2l_cru_stop_image_processing(). That may cause scheduling while > > atomic bug. > > > > Signed-off-by: Khai Nguyen <khai.nguyen.wx@renesas.com> > > Signed-off-by: Hao Bui <hao.bui.yg@renesas.com> > > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> > > --- > > drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > index 162e2ace6931..1355bfd186d4 100644 > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > @@ -369,7 +369,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru) > > if (cru->info->fifo_empty(cru)) > > break; > > > > - usleep_range(10, 20); > > + udelay(10); > > There's an instance of msleep() earlier in this function, surrounded by > spin_unlock_irqrestore() and spin_lock_irqsave(). I wondered if we > should do the same here, but that lead to a second question: why does > the driver need to cover the whole stop procedure with a spinlock in the > first place ? Good point :) Mmm maybe the only critical section into the rzg2l_cru_stop_image_processing() that needs spin_unlock_irqrestore()/spin_lock_irqsave() is: spin_lock_irqsave(&cru->qlock, flags); cru->state = RZG2L_CRU_DMA_STOPPED; spin_unlock_irqrestore(&cru->qlock, flags); Please correct me if I'm wrong. Kind Regards, Tommaso > > > } > > > > /* Notify that FIFO is not empty here */ > > @@ -385,7 +385,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru) > > AMnAXISTPACK_AXI_STOP_ACK) > > break; > > > > - usleep_range(10, 20); > > + udelay(10); > > } > > > > /* Notify that AXI bus can not stop here */ > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: rzg2l-cru: Replace usleep_range with udelay 2025-12-03 8:45 ` Tommaso Merciai @ 2025-12-03 10:55 ` David Laight 0 siblings, 0 replies; 4+ messages in thread From: David Laight @ 2025-12-03 10:55 UTC (permalink / raw) To: Tommaso Merciai Cc: Laurent Pinchart, tomm.merciai, linux-renesas-soc, biju.das.jz, Mauro Carvalho Chehab, Hans Verkuil, Lad Prabhakar, Daniel Scally, Jacopo Mondi, linux-media, linux-kernel, Khai Nguyen, Hao Bui On Wed, 3 Dec 2025 09:45:43 +0100 Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> wrote: > Hi Laurent, > > Thanks for your review. > > On Wed, Dec 03, 2025 at 11:35:52AM +0900, Laurent Pinchart wrote: > > Hi Tommaso, > > > > Thank you for the patch. > > > > On Tue, Dec 02, 2025 at 05:08:41PM +0100, Tommaso Merciai wrote: > > > `usleep_range()` should not be used in atomic contexts like between > > > spin_lock_irqsave()/spin_lock_irqrestore() pair inside function > > > rzg2l_cru_stop_image_processing(). That may cause scheduling while > > > atomic bug. > > > > > > Signed-off-by: Khai Nguyen <khai.nguyen.wx@renesas.com> > > > Signed-off-by: Hao Bui <hao.bui.yg@renesas.com> > > > Signed-off-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com> > > > --- > > > drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > index 162e2ace6931..1355bfd186d4 100644 > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > > @@ -369,7 +369,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru) > > > if (cru->info->fifo_empty(cru)) > > > break; > > > > > > - usleep_range(10, 20); > > > + udelay(10); > > > > There's an instance of msleep() earlier in this function, surrounded by > > spin_unlock_irqrestore() and spin_lock_irqsave(). I wondered if we > > should do the same here, but that lead to a second question: why does > > the driver need to cover the whole stop procedure with a spinlock in the > > first place ? > > Good point :) > Mmm maybe the only critical section into the > rzg2l_cru_stop_image_processing() that needs > spin_unlock_irqrestore()/spin_lock_irqsave() > is: > > spin_lock_irqsave(&cru->qlock, flags); > cru->state = RZG2L_CRU_DMA_STOPPED; > spin_unlock_irqrestore(&cru->qlock, flags); That doesn't look right. You pretty much never need a lock for a single assignment. The most you need is a WRITE_ONCE() - and they are pretty unlikely to matter at all. David > > Please correct me if I'm wrong. > > Kind Regards, > Tommaso > > > > > > } > > > > > > /* Notify that FIFO is not empty here */ > > > @@ -385,7 +385,7 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru) > > > AMnAXISTPACK_AXI_STOP_ACK) > > > break; > > > > > > - usleep_range(10, 20); > > > + udelay(10); > > > } > > > > > > /* Notify that AXI bus can not stop here */ > > > > -- > > Regards, > > > > Laurent Pinchart > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-12-03 10:55 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-12-02 16:08 [PATCH] media: rzg2l-cru: Replace usleep_range with udelay Tommaso Merciai 2025-12-03 2:35 ` Laurent Pinchart 2025-12-03 8:45 ` Tommaso Merciai 2025-12-03 10:55 ` David Laight
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox