linux-kernel - Re: [PATCH] media: imx-mipi-csis: avoid logging while holding spinlock

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: <20240729093259.GB2320@pendragon.ideasonboard.com>
Date: Mon, 29 Jul 2024 12:32:59 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Matthias Fend <matthias.fend@...end.at>
Cc: Rui Miguel Silva <rmfrfs@...il.com>,
	Martin Kepplinger <martink@...teo.de>,
	Purism Kernel Team <kernel@...i.sm>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, linux-media@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: imx-mipi-csis: avoid logging while holding
 spinlock

On Fri, Jul 26, 2024 at 02:16:07PM +0200, Matthias Fend wrote:
> Hi Laurent,
> 
> Am 26.07.2024 um 12:42 schrieb Laurent Pinchart:
> > Hi Matthias,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Jul 23, 2024 at 08:09:08AM +0200, Matthias Fend wrote:
> >> Refactor mipi_csis_log_counters() to prevent calling dev_info() while
> >> IRQs are disabled. This reduces crucial IRQs off time to a bare minimum.
> > 
> > Should we add
> > 
> > Cc: stable@...r.kernel.org
> > 
> > to get this backported to stable kernels ?
> 
> Hard for me to tell. Since this problem only occurs when you explicitly 
> trigger the debug output from this driver, it probably only affects very 
> few end users. So I'm not sure if a backport would be worth it.

I slept over it and decided not to Cc stable.

> >> Signed-off-by: Matthias Fend <matthias.fend@...end.at>
> >> ---
> >>   drivers/media/platform/nxp/imx-mipi-csis.c | 9 ++++++---
> >>   1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> >> index f49b06978f14..0c34d316ed29 100644
> >> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> >> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> >> @@ -857,18 +857,21 @@ static void mipi_csis_log_counters(struct mipi_csis_device *csis, bool non_error
> >>   {
> >>   	unsigned int num_events = non_errors ? MIPI_CSIS_NUM_EVENTS
> >>   				: MIPI_CSIS_NUM_EVENTS - 8;
> >> +	unsigned int counters[MIPI_CSIS_NUM_EVENTS];
> >>   	unsigned long flags;
> >>   	unsigned int i;
> >>   
> >>   	spin_lock_irqsave(&csis->slock, flags);
> >> +	for (i = 0; i < num_events; ++i)
> >> +		counters[i] =  csis->events[i].counter;
> >> +	spin_unlock_irqrestore(&csis->slock, flags);
> >>   
> >>   	for (i = 0; i < num_events; ++i) {
> >> -		if (csis->events[i].counter > 0 || csis->debug.enable)
> >> +		if (counters[i] > 0 || csis->debug.enable)
> >>   			dev_info(csis->dev, "%s events: %d\n",
> >>   				 csis->events[i].name,
> >> -				 csis->events[i].counter);
> >> +				 counters[i]);
> > 
> > The last two lines now fit in a single line. No need for a v2, I'll
> > rewrap the code myself, and add the Cc line if you think that's a good
> > idea.
> 
> Thanks! Regarding Cc, I would like to leave it up to you; you have the 
> necessary experience and overview to judge that.
> 
> ~Matthias
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > 
> >>   	}
> >> -	spin_unlock_irqrestore(&csis->slock, flags);
> >>   }
> >>   
> >>   static int mipi_csis_dump_regs(struct mipi_csis_device *csis)

-- 
Regards,

Laurent Pinchart