Re: [PATCH 1/2] i2c: imx: use guard to take spinlock

[Troy Mitchell <troymitchell988@gmail.com>]

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;
>>  }

-- 
Troy Mitchell