diff mbox series

thermal: replace spin_lock_irqsave by spin_lock in hard IRQ

Message ID 1603249530-25218-1-git-send-email-tiantao6@hisilicon.com
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series thermal: replace spin_lock_irqsave by spin_lock in hard IRQ | expand

Commit Message

Tian Tao Oct. 21, 2020, 3:05 a.m. UTC
The code has been in a irq-disabled context since it is hard IRQ. There
is no necessity to do it again.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 drivers/thermal/rcar_thermal.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Niklas Söderlund Oct. 21, 2020, 7:42 p.m. UTC | #1
Hi Tian,

Thanks for your work.

On 2020-10-21 11:05:30 +0800, Tian Tao wrote:
> The code has been in a irq-disabled context since it is hard IRQ. There
> is no necessity to do it again.
> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/thermal/rcar_thermal.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index 5c2a13b..6ae757d 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -409,16 +409,15 @@ static irqreturn_t rcar_thermal_irq(int irq, void *data)
>  {
>  	struct rcar_thermal_common *common = data;
>  	struct rcar_thermal_priv *priv;
> -	unsigned long flags;
>  	u32 status, mask;
>  
> -	spin_lock_irqsave(&common->lock, flags);
> +	spin_lock(&common->lock);
>  
>  	mask	= rcar_thermal_common_read(common, INTMSK);
>  	status	= rcar_thermal_common_read(common, STR);
>  	rcar_thermal_common_write(common, STR, 0x000F0F0F & mask);
>  
> -	spin_unlock_irqrestore(&common->lock, flags);
> +	spin_unlock(&common->lock);
>  
>  	status = status & ~mask;
>  
> -- 
> 2.7.4
>
Geert Uytterhoeven Oct. 22, 2020, 12:51 p.m. UTC | #2
Hi Tian,

CC tglx

On Wed, Oct 21, 2020 at 2:15 PM Tian Tao <tiantao6@hisilicon.com> wrote:
> The code has been in a irq-disabled context since it is hard IRQ. There
> is no necessity to do it again.
>
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>

Thanks for your patch!

Is this also true if CONFIG_PREEMPT_RT=y, and all irq handlers execute
in the context of special tasks?

> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -409,16 +409,15 @@ static irqreturn_t rcar_thermal_irq(int irq, void *data)
>  {
>         struct rcar_thermal_common *common = data;
>         struct rcar_thermal_priv *priv;
> -       unsigned long flags;
>         u32 status, mask;
>
> -       spin_lock_irqsave(&common->lock, flags);
> +       spin_lock(&common->lock);
>
>         mask    = rcar_thermal_common_read(common, INTMSK);
>         status  = rcar_thermal_common_read(common, STR);
>         rcar_thermal_common_write(common, STR, 0x000F0F0F & mask);
>
> -       spin_unlock_irqrestore(&common->lock, flags);
> +       spin_unlock(&common->lock);
>
>         status = status & ~mask;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Thomas Gleixner Oct. 23, 2020, 9:45 p.m. UTC | #3
On Thu, Oct 22 2020 at 14:51, Geert Uytterhoeven wrote:
> On Wed, Oct 21, 2020 at 2:15 PM Tian Tao <tiantao6@hisilicon.com> wrote:
>> The code has been in a irq-disabled context since it is hard IRQ. There
>> is no necessity to do it again.
>>
> Is this also true if CONFIG_PREEMPT_RT=y, and all irq handlers execute
> in the context of special tasks?

On RT or even on mainline with 'threadirqs' on the command line all
interrupts which are not explicitly requested with IRQF_NO_THREAD run
their handlers in thread context. The same applies to soft interrupts.

That means they are subject to the normal scheduler rules and no other
code is going to acquire that lock from hard interrupt context either,
so the irqsave() here is pointless in all cases.

Famous last words...

  ... unless the driver does magic things like having a hrtimer armed
  which expires in hard interrupt context and touches the very same
  lock, but that's not the case in this particular driver.

So the change itself is correct, but the change log could do with some
polishing. :)

Thanks,

        tglx
Daniel Lezcano Oct. 26, 2020, 6:28 p.m. UTC | #4
On 21/10/2020 05:05, Tian Tao wrote:
> The code has been in a irq-disabled context since it is hard IRQ. There
> is no necessity to do it again.
> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> ---

Whe resending with the log improved, please fix the subject:

thermal: rcar: Replace ...

Thanks

  -- Daniel

>  drivers/thermal/rcar_thermal.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index 5c2a13b..6ae757d 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -409,16 +409,15 @@ static irqreturn_t rcar_thermal_irq(int irq, void *data)
>  {
>  	struct rcar_thermal_common *common = data;
>  	struct rcar_thermal_priv *priv;
> -	unsigned long flags;
>  	u32 status, mask;
>  
> -	spin_lock_irqsave(&common->lock, flags);
> +	spin_lock(&common->lock);
>  
>  	mask	= rcar_thermal_common_read(common, INTMSK);
>  	status	= rcar_thermal_common_read(common, STR);
>  	rcar_thermal_common_write(common, STR, 0x000F0F0F & mask);
>  
> -	spin_unlock_irqrestore(&common->lock, flags);
> +	spin_unlock(&common->lock);
>  
>  	status = status & ~mask;
>  
>
diff mbox series

Patch

diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index 5c2a13b..6ae757d 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -409,16 +409,15 @@  static irqreturn_t rcar_thermal_irq(int irq, void *data)
 {
 	struct rcar_thermal_common *common = data;
 	struct rcar_thermal_priv *priv;
-	unsigned long flags;
 	u32 status, mask;
 
-	spin_lock_irqsave(&common->lock, flags);
+	spin_lock(&common->lock);
 
 	mask	= rcar_thermal_common_read(common, INTMSK);
 	status	= rcar_thermal_common_read(common, STR);
 	rcar_thermal_common_write(common, STR, 0x000F0F0F & mask);
 
-	spin_unlock_irqrestore(&common->lock, flags);
+	spin_unlock(&common->lock);
 
 	status = status & ~mask;