| Message ID | 1603249530-25218-1-git-send-email-tiantao6@hisilicon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | thermal: replace spin_lock_irqsave by spin_lock in hard IRQ | expand |
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 >
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
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
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 --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;
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(-)