linux-kernel - Re: [PATCH 1/3] media: rcar-vin: Fold interrupt helpers into only callers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250616182830.GB12825@pendragon.ideasonboard.com>
Date: Mon, 16 Jun 2025 21:28:30 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	linux-media@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] media: rcar-vin: Fold interrupt helpers into only
 callers

On Sun, Jun 15, 2025 at 02:48:39PM +0200, Niklas Söderlund wrote:
> On 2025-06-15 00:23:14 +0300, Laurent Pinchart wrote:
> > On Sat, Jun 14, 2025 at 04:15:43PM +0200, Niklas Söderlund wrote:
> > > The call sites using the interrupt helper functions have all been
> > > reworked to only one for each. Fold echo of them into the only call
> > > sites left.
> > > 
> > > While at it rename the variable holding the current interrupt status to
> > > make the code easier to read.
> > > 
> > > There is no functional change.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
> > > ---
> > >  .../platform/renesas/rcar-vin/rcar-dma.c      | 27 +++++--------------
> > >  1 file changed, 6 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > index 5c08ee2c9807..585b8b3dcfd8 100644
> > > --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> > > @@ -912,21 +912,6 @@ static int rvin_setup(struct rvin_dev *vin)
> > >  	return 0;
> > >  }
> > >  
> > > -static void rvin_disable_interrupts(struct rvin_dev *vin)
> > > -{
> > > -	rvin_write(vin, 0, VNIE_REG);
> > > -}
> > > -
> > > -static u32 rvin_get_interrupt_status(struct rvin_dev *vin)
> > > -{
> > > -	return rvin_read(vin, VNINTS_REG);
> > > -}
> > > -
> > > -static void rvin_ack_interrupt(struct rvin_dev *vin)
> > > -{
> > > -	rvin_write(vin, rvin_read(vin, VNINTS_REG), VNINTS_REG);
> > > -}
> > > -
> > >  static bool rvin_capture_active(struct rvin_dev *vin)
> > >  {
> > >  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> > > @@ -1049,22 +1034,22 @@ static void rvin_capture_stop(struct rvin_dev *vin)
> > >  static irqreturn_t rvin_irq(int irq, void *data)
> > >  {
> > >  	struct rvin_dev *vin = data;
> > > -	u32 int_status, vnms;
> > > +	u32 status, vnms;
> > >  	int slot;
> > >  	unsigned int handled = 0;
> > >  	unsigned long flags;
> > >  
> > >  	spin_lock_irqsave(&vin->qlock, flags);
> > >  
> > > -	int_status = rvin_get_interrupt_status(vin);
> > > -	if (!int_status)
> > > +	status = rvin_read(vin, VNINTS_REG);
> > > +	if (!status)
> > >  		goto done;
> > >  
> > > -	rvin_ack_interrupt(vin);
> > > +	rvin_write(vin, status, VNINTS_REG);
> > 
> > Actually there is a functional change here. Before this change, if an
> > interrupt occured between reading VNINTS_REG in
> > rvin_get_interrupt_status() and reading it again in
> > rvin_ack_interrupt(), it would get lost. This patch fixes a possible
> > bug.
> 
> Indeed, thanks for pointing it out. I will update the commit message to 
> mention this.

Let me make a proposal.

---
The call sites using the interrupt helper functions have all been
reworked to only one for each. Fold echo of them into the only call
sites left.

This fixes a possible interrupt loss in case an interrupt occurs between
reading VNINTS_REG in rvin_get_interrupt_status() and reading it again
in rvin_ack_interrupt().

While at it rename the variable holding the current interrupt status to
make the code easier to read.
---

Feel free to add a Fixes: tag if applicable (and a Cc:
stable@...r.kernel.org in that case).

> > With an updated commit message,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
> > 
> > >  	handled = 1;
> > >  
> > >  	/* Nothing to do if nothing was captured. */
> > > -	if (!(int_status & VNINTS_FIS))
> > > +	if (!(status & VNINTS_FIS))
> > >  		goto done;
> > >  
> > >  	/* Nothing to do if not running. */
> > > @@ -1417,7 +1402,7 @@ void rvin_stop_streaming(struct rvin_dev *vin)
> > >  	rvin_set_stream(vin, 0);
> > >  
> > >  	/* disable interrupts */
> > > -	rvin_disable_interrupts(vin);
> > > +	rvin_write(vin, 0, VNIE_REG);
> > >  
> > >  	/* Return unprocessed buffers from hardware. */
> > >  	for (unsigned int i = 0; i < HW_BUFFER_NUM; i++) {

-- 
Regards,

Laurent Pinchart