| Message ID | 20250421-i2c-imx-update-v1-1-1137f1f353d5@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: imx: adapting the mainline | expand |
Hi Troy,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 9d7a0577c9db35c4cc52db90bc415ea248446472]
url: https://github.com/intel-lab-lkp/linux/commits/Troy-Mitchell/i2c-imx-use-guard-to-take-spinlock/20250421-133753
base: 9d7a0577c9db35c4cc52db90bc415ea248446472
patch link: https://lore.kernel.org/r/20250421-i2c-imx-update-v1-1-1137f1f353d5%40gmail.com
patch subject: [PATCH 1/2] i2c: imx: use guard to take spinlock
config: hexagon-randconfig-001-20250421 (https://download.01.org/0day-ci/archive/20250421/202504211501.MNwGdl5F-lkp@intel.com/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250421/202504211501.MNwGdl5F-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504211501.MNwGdl5F-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-imx.c:1140:17: warning: unused variable 'ret' [-Wunused-variable]
irqreturn_t ret;
^
1 warning generated.
vim +/ret +1140 drivers/i2c/busses/i2c-imx.c
aa11e38ce6fe88 Darius Augulis 2009-01-30 1124
f7414cd6923fd7 Biwen Li 2020-11-11 1125 static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
f7414cd6923fd7 Biwen Li 2020-11-11 1126 {
f7414cd6923fd7 Biwen Li 2020-11-11 1127 struct imx_i2c_struct *i2c_imx = dev_id;
f7414cd6923fd7 Biwen Li 2020-11-11 1128 unsigned int ctl, status;
f7414cd6923fd7 Biwen Li 2020-11-11 1129
d3973a577b8e10 Troy Mitchell 2025-04-21 1130 guard(spinlock_irqsave)(&i2c_imx->slave_lock);
d3973a577b8e10 Troy Mitchell 2025-04-21 1131
f7414cd6923fd7 Biwen Li 2020-11-11 1132 status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
f7414cd6923fd7 Biwen Li 2020-11-11 1133 ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
05ae60bc24f765 Kevin Paul Herbert 2020-12-22 1134
f7414cd6923fd7 Biwen Li 2020-11-11 1135 if (status & I2SR_IIF) {
f7414cd6923fd7 Biwen Li 2020-11-11 1136 i2c_imx_clear_irq(i2c_imx, I2SR_IIF);
d3973a577b8e10 Troy Mitchell 2025-04-21 1137
05ae60bc24f765 Kevin Paul Herbert 2020-12-22 1138 if (i2c_imx->slave) {
05ae60bc24f765 Kevin Paul Herbert 2020-12-22 1139 if (!(ctl & I2CR_MSTA)) {
f89bf95632b416 Corey Minyard 2021-11-12 @1140 irqreturn_t ret;
f89bf95632b416 Corey Minyard 2021-11-12 1141
d3973a577b8e10 Troy Mitchell 2025-04-21 1142 return i2c_imx_slave_handle(i2c_imx,
f89bf95632b416 Corey Minyard 2021-11-12 1143 status, ctl);
05ae60bc24f765 Kevin Paul Herbert 2020-12-22 1144 }
f89bf95632b416 Corey Minyard 2021-11-12 1145 i2c_imx_slave_finish_op(i2c_imx);
05ae60bc24f765 Kevin Paul Herbert 2020-12-22 1146 }
d3973a577b8e10 Troy Mitchell 2025-04-21 1147
f7414cd6923fd7 Biwen Li 2020-11-11 1148 return i2c_imx_master_isr(i2c_imx, status);
f7414cd6923fd7 Biwen Li 2020-11-11 1149 }
f7414cd6923fd7 Biwen Li 2020-11-11 1150
aa11e38ce6fe88 Darius Augulis 2009-01-30 1151 return IRQ_NONE;
aa11e38ce6fe88 Darius Augulis 2009-01-30 1152 }
aa11e38ce6fe88 Darius Augulis 2009-01-30 1153
On Mon, Apr 21, 2025 at 01:36:38PM +0800, Troy Mitchell wrote: > Use guard to automatically release the lock after going out of scope > instead of calling it manually. > > Co-developed-by: Yongchao Jia <jyc0019@gmail.com> > Signed-off-by: Yongchao Jia <jyc0019@gmail.com> > Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> Reviewed-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/i2c/busses/i2c-imx.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 9e5d454d8318..cb96a57df4a0 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -23,6 +23,7 @@ > > #include <linux/acpi.h> > #include <linux/clk.h> > +#include <linux/cleanup.h> > #include <linux/completion.h> > #include <linux/delay.h> > #include <linux/dma-mapping.h> > @@ -891,13 +892,13 @@ static enum hrtimer_restart i2c_imx_slave_timeout(struct hrtimer *t) > struct imx_i2c_struct *i2c_imx = container_of(t, struct imx_i2c_struct, > slave_timer); > unsigned int ctl, status; > - unsigned long flags; > > - spin_lock_irqsave(&i2c_imx->slave_lock, flags); > + guard(spinlock_irqsave)(&i2c_imx->slave_lock); > + > status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR); > ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > i2c_imx_slave_handle(i2c_imx, status, ctl); > - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); > + > return HRTIMER_NORESTART; > } > > @@ -1125,30 +1126,27 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id) > { > struct imx_i2c_struct *i2c_imx = dev_id; > unsigned int ctl, status; > - unsigned long flags; > > - spin_lock_irqsave(&i2c_imx->slave_lock, flags); > + guard(spinlock_irqsave)(&i2c_imx->slave_lock); > + > status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR); > ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > > if (status & I2SR_IIF) { > i2c_imx_clear_irq(i2c_imx, I2SR_IIF); > + > if (i2c_imx->slave) { > if (!(ctl & I2CR_MSTA)) { > irqreturn_t ret; > > - ret = i2c_imx_slave_handle(i2c_imx, > - status, ctl); > - spin_unlock_irqrestore(&i2c_imx->slave_lock, > - flags); > - return ret; > + return i2c_imx_slave_handle(i2c_imx, > + status, ctl); > } > i2c_imx_slave_finish_op(i2c_imx); > } > - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); > + > return i2c_imx_master_isr(i2c_imx, status); > } > - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); > > return IRQ_NONE; > } > > -- > 2.34.1 >
On Mon, 21 Apr 2025 13:36:38 +0800 Troy Mitchell <troymitchell988@gmail.com> wrote: > Use guard to automatically release the lock after going out of scope > instead of calling it manually. Drive by review, but this changes behavior in a subtle way so we should have more commentary here... > > Co-developed-by: Yongchao Jia <jyc0019@gmail.com> > Signed-off-by: Yongchao Jia <jyc0019@gmail.com> > Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> > --- > drivers/i2c/busses/i2c-imx.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 9e5d454d8318..cb96a57df4a0 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -23,6 +23,7 @@ > > #include <linux/acpi.h> > #include <linux/clk.h> > +#include <linux/cleanup.h> > #include <linux/completion.h> > #include <linux/delay.h> > #include <linux/dma-mapping.h> > > @@ -1125,30 +1126,27 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id) > { > struct imx_i2c_struct *i2c_imx = dev_id; > unsigned int ctl, status; > - unsigned long flags; > > - spin_lock_irqsave(&i2c_imx->slave_lock, flags); > + guard(spinlock_irqsave)(&i2c_imx->slave_lock); > + > status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR); > ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > > if (status & I2SR_IIF) { > i2c_imx_clear_irq(i2c_imx, I2SR_IIF); > + > if (i2c_imx->slave) { > if (!(ctl & I2CR_MSTA)) { > irqreturn_t ret; > > - ret = i2c_imx_slave_handle(i2c_imx, > - status, ctl); > - spin_unlock_irqrestore(&i2c_imx->slave_lock, > - flags); > - return ret; > + return i2c_imx_slave_handle(i2c_imx, > + status, ctl); > } > i2c_imx_slave_finish_op(i2c_imx); > } > - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); In this path the patch changes the lock release to occur after i2c_imx_master_isr(i2c_imx, status); That may well be safe; I have no idea! You should talk about that in the patch description if it is. > + > return i2c_imx_master_isr(i2c_imx, status); > } > - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); > > return IRQ_NONE; > } >
On 2025/4/22 23:12, Jonathan Cameron wrote: > On Mon, 21 Apr 2025 13:36:38 +0800 > Troy Mitchell <troymitchell988@gmail.com> wrote: > >> Use guard to automatically release the lock after going out of scope >> instead of calling it manually. > Drive by review, but this changes behavior in a subtle way so we > should have more commentary here... Thanks for your review! > >> Co-developed-by: Yongchao Jia <jyc0019@gmail.com> >> Signed-off-by: Yongchao Jia <jyc0019@gmail.com> >> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com> >> --- >> drivers/i2c/busses/i2c-imx.c | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c >> index 9e5d454d8318..cb96a57df4a0 100644 >> --- a/drivers/i2c/busses/i2c-imx.c >> +++ b/drivers/i2c/busses/i2c-imx.c >> @@ -23,6 +23,7 @@ >> >> #include <linux/acpi.h> >> #include <linux/clk.h> >> +#include <linux/cleanup.h> >> #include <linux/completion.h> >> #include <linux/delay.h> >> #include <linux/dma-mapping.h> >> >> @@ -1125,30 +1126,27 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id) >> { >> struct imx_i2c_struct *i2c_imx = dev_id; >> unsigned int ctl, status; >> - unsigned long flags; >> >> - spin_lock_irqsave(&i2c_imx->slave_lock, flags); >> + guard(spinlock_irqsave)(&i2c_imx->slave_lock); >> + >> status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR); >> ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); >> >> if (status & I2SR_IIF) { >> i2c_imx_clear_irq(i2c_imx, I2SR_IIF); >> + >> if (i2c_imx->slave) { >> if (!(ctl & I2CR_MSTA)) { >> irqreturn_t ret; >> >> - ret = i2c_imx_slave_handle(i2c_imx, >> - status, ctl); >> - spin_unlock_irqrestore(&i2c_imx->slave_lock, >> - flags); >> - return ret; >> + return i2c_imx_slave_handle(i2c_imx, >> + status, ctl); >> } >> i2c_imx_slave_finish_op(i2c_imx); >> } >> - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); > In this path the patch changes the lock release to occur after > i2c_imx_master_isr(i2c_imx, status); > > That may well be safe; I have no idea! You should talk about that > in the patch description if it is. You're correct that this change slightly alters the lock release timing. However, both i2c_imx_slave_handle() and i2c_imx_master_isr() can safely be entered with the lock held — there's no requirement for the lock to be released before calling them. Using guard(spinlock_irqsave) simplifies the control flow by ensuring consistent and automatic unlock, which improves readability without affecting correctness. I'll update the commit message to clarify this. > >> + >> return i2c_imx_master_isr(i2c_imx, status); >> } >> - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); >> >> return IRQ_NONE; >> }
Hello Troy, On 4/22/25 17:26, Troy Mitchell wrote: > On 2025/4/22 23:12, Jonathan Cameron wrote: >> On Mon, 21 Apr 2025 13:36:38 +0800 [snip] >> In this path the patch changes the lock release to occur after >> i2c_imx_master_isr(i2c_imx, status); >> >> That may well be safe; I have no idea! You should talk about that >> in the patch description if it is. > You're correct that this change slightly alters the lock release timing. > > However, both i2c_imx_slave_handle() and i2c_imx_master_isr() can safely be > entered with the lock held — there's no requirement for the lock to be released > before calling them. Why would we want to hold a spinlock out of all things longer than absolutely necessary? > Using guard(spinlock_irqsave) simplifies the control flow by ensuring consistent > and automatic unlock, which improves readability without affecting correctness. > > I'll update the commit message to clarify this. Sorry, I don't think that this simplification is worth potentially increasing the time we spend with preemption disabled, i2c_imx_master_isr() even has a loop inside. Guards that don't increase critical section size however are fine by me. Thanks, Ahmad > >> >>> + >>> return i2c_imx_master_isr(i2c_imx, status); >>> } >>> - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); >>> >>> return IRQ_NONE; >>> } >
On 4/22/25 17:53, Ahmad Fatoum wrote: > Hello Troy, > > On 4/22/25 17:26, Troy Mitchell wrote: >> On 2025/4/22 23:12, Jonathan Cameron wrote: >>> On Mon, 21 Apr 2025 13:36:38 +0800 > > [snip] > >>> In this path the patch changes the lock release to occur after >>> i2c_imx_master_isr(i2c_imx, status); >>> >>> That may well be safe; I have no idea! You should talk about that >>> in the patch description if it is. >> You're correct that this change slightly alters the lock release timing. >> >> However, both i2c_imx_slave_handle() and i2c_imx_master_isr() can safely be >> entered with the lock held — there's no requirement for the lock to be released >> before calling them. > > Why would we want to hold a spinlock out of all things longer than > absolutely necessary? > >> Using guard(spinlock_irqsave) simplifies the control flow by ensuring consistent >> and automatic unlock, which improves readability without affecting correctness. >> >> I'll update the commit message to clarify this. > > Sorry, I don't think that this simplification is worth potentially > increasing the time we spend with preemption disabled, > i2c_imx_master_isr() even has a loop inside. Ah the loop is in lpi2c_imx_master_isr, sorry about the confusion. I still think that the simplification isn't worth increasing critical section size. > Guards that don't increase critical section size however are fine by me. Thanks, Ahmad > > Thanks, > Ahmad > >> >>> >>>> + >>>> return i2c_imx_master_isr(i2c_imx, status); >>>> } >>>> - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); >>>> >>>> return IRQ_NONE; >>>> } >> >
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 9e5d454d8318..cb96a57df4a0 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -23,6 +23,7 @@ #include <linux/acpi.h> #include <linux/clk.h> +#include <linux/cleanup.h> #include <linux/completion.h> #include <linux/delay.h> #include <linux/dma-mapping.h> @@ -891,13 +892,13 @@ static enum hrtimer_restart i2c_imx_slave_timeout(struct hrtimer *t) struct imx_i2c_struct *i2c_imx = container_of(t, struct imx_i2c_struct, slave_timer); unsigned int ctl, status; - unsigned long flags; - spin_lock_irqsave(&i2c_imx->slave_lock, flags); + guard(spinlock_irqsave)(&i2c_imx->slave_lock); + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR); ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); i2c_imx_slave_handle(i2c_imx, status, ctl); - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); + return HRTIMER_NORESTART; } @@ -1125,30 +1126,27 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id) { struct imx_i2c_struct *i2c_imx = dev_id; unsigned int ctl, status; - unsigned long flags; - spin_lock_irqsave(&i2c_imx->slave_lock, flags); + guard(spinlock_irqsave)(&i2c_imx->slave_lock); + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR); ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); if (status & I2SR_IIF) { i2c_imx_clear_irq(i2c_imx, I2SR_IIF); + if (i2c_imx->slave) { if (!(ctl & I2CR_MSTA)) { irqreturn_t ret; - ret = i2c_imx_slave_handle(i2c_imx, - status, ctl); - spin_unlock_irqrestore(&i2c_imx->slave_lock, - flags); - return ret; + return i2c_imx_slave_handle(i2c_imx, + status, ctl); } i2c_imx_slave_finish_op(i2c_imx); } - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); + return i2c_imx_master_isr(i2c_imx, status); } - spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); return IRQ_NONE; }