diff mbox series

media: imx-mipi-csis: avoid logging while holding spinlock

Message ID 20240723060909.534584-1-matthias.fend@emfend.at
State Accepted
Commit 3f6a2f8768e91cdb69947cb589d0b3a09114be83
Headers show
Series media: imx-mipi-csis: avoid logging while holding spinlock | expand

Commit Message

Matthias Fend July 23, 2024, 6:09 a.m. UTC
Refactor mipi_csis_log_counters() to prevent calling dev_info() while
IRQs are disabled. This reduces crucial IRQs off time to a bare minimum.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart July 26, 2024, 10:42 a.m. UTC | #1
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@vger.kernel.org

to get this backported to stable kernels ?

> Signed-off-by: Matthias Fend <matthias.fend@emfend.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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	}
> -	spin_unlock_irqrestore(&csis->slock, flags);
>  }
>  
>  static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
Matthias Fend July 26, 2024, 12:16 p.m. UTC | #2
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@vger.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.

> 
>> Signed-off-by: Matthias Fend <matthias.fend@emfend.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@ideasonboard.com>
> 
>>   	}
>> -	spin_unlock_irqrestore(&csis->slock, flags);
>>   }
>>   
>>   static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
>
Laurent Pinchart July 29, 2024, 9:32 a.m. UTC | #3
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@vger.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@emfend.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@ideasonboard.com>
> > 
> >>   	}
> >> -	spin_unlock_irqrestore(&csis->slock, flags);
> >>   }
> >>   
> >>   static int mipi_csis_dump_regs(struct mipi_csis_device *csis)
diff mbox series

Patch

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]);
 	}
-	spin_unlock_irqrestore(&csis->slock, flags);
 }
 
 static int mipi_csis_dump_regs(struct mipi_csis_device *csis)