linux-kernel - RE: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <981861a36edc4b759e20f9be45d243c7@siengine.com>
Date: Wed, 11 Sep 2024 01:37:43 +0000
From: Liu Kimriver/刘金河 <kimriver.liu@...ngine.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC: "jarkko.nikula@...ux.intel.com" <jarkko.nikula@...ux.intel.com>,
        "mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
        "jsd@...ihalf.com" <jsd@...ihalf.com>,
        "andi.shyti@...nel.org"
	<andi.shyti@...nel.org>,
        "linux-i2c@...r.kernel.org"
	<linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v8] i2c: designware: fix master is holding SCL low while
 ENABLE bit is disabled

Hi Andy


>-----Original Message-----
>From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com> 
>Sent: 2024年9月10日 19:59
>To: Liu Kimriver/刘金河 <kimriver.liu@...ngine.com>
>Cc: jarkko.nikula@...ux.intel.com; mika.westerberg@...ux.intel.com; jsd@...ihalf.com; andi.shyti@...nel.org; linux-i2c@...r.kernel.org; linux-kernel@...r.kernel.org
>Subject: Re: [PATCH v8] i2c: designware: fix master is holding SCL low while ENABLE bit is disabled

>On Tue, Sep 10, 2024 at 11:43:34AM +0000, Liu Kimriver/刘金河 wrote:
>> >-----Original Message-----
>> >From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>> >Sent: 2024年9月10日 18:45
>> >On Tue, Sep 10, 2024 at 09:38:53AM +0000, Liu Kimriver/刘金河 wrote:
>> >> >From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>> >> >Sent: 2024年9月10日 17:03
>> >> >at 02:13:09PM +0800, Kimriver Liu wrote:
>
>...
>
>> > >> +static bool i2c_dw_is_master_idling(struct dw_i2c_dev *dev)
>> >> 
>> >> >Sorry if I made a mistake, but again, looking at the usage you 
>> >> >have again negation here and there...
>> > 
>> >> >	i2c_dw_is_controller_active
>> >> 
>> >> > (note new terminology, dunno if it makes sense start using it in 
>> >> > function names, as we have more of them following old style)
>> >> 
>> >>  Last week , You suggested that I used this
>> >> i2c_dw_is_master_idling(dev)
>> 
>> >Yes, sorry about that. I did maybe not clearly get how it is going to look like.
>> 
>> >> >> +{
>> >> >> +	u32 status;
>> >> >> +
>> >> >> +	regmap_read(dev->map, DW_IC_STATUS, &status);
>> >> >> +	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
>> >> >> +		return true;
>> >> 
>> >> 		return false;
>> >> 
>> >> >> +	return !regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>> >> >> +			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
>> >> >> +			1100, 20000);
>> >> 
>> >> >...and drop !.
>> >> 
>> >>  We reproduce this issue in RTL simulation(About(~1:500) in our soc). 
>> >> It is necessary  to add waiting DW_IC_STATUS_MASTER_ACTIVITY idling 
>> >> before disabling I2C when  I2C transfer completed.  as described in 
>> >> the DesignWare  I2C databook(Flowchart for DW_apb_i2c Controller)
>> 
>> >Cool, but here I'm talking purely about inverting the logic (with renaming), nothing more.
>> 
>>  as described in the DesignWare I2C databook:
>>  DW_IC_STATUS[5].MST_ACTIVITY Description as follows:
>>  Controller FSM Activity Status. When the Controller Finite  State 
>> Machine (FSM) is not in the IDLE state, this bit is set.
>>  Note: IC_STATUS[0]-that is, ACTIVITY bit-is the OR of  SLV_ACTIVITY 
>> and MST_ACTIVITY bits.
>>  Values:
>>  ■ 0x1 (ACTIVE): Controller not idle
>>  ■ 0x0 (IDLE): Controller is idle
>> 
>> We need waiting DW_IC_STATUS.MST_ACTIVITY idling, If Controller not 
>> idle, Wait for a while.
>> Return value: 
>>   false(0): Controller is idle
>>   timeout(-110): Controller activity
>> 
>> Ok, change the function name i2c_dw_is_master_idling(dev) to 
>> i2c_dw_is_controller_active(dev) it seems more reasonable
>> 


 Change above text as a comment:

/*
 * This functions waits controller idling before disabling I2C
 * When the controller is not in the IDLE state, 
 * MST_ACTIVITY bit (IC_STATUS[5]) is set:
 * 0x1 (ACTIVE): Controller not idle
 * 0x0 (IDLE): Controller is idle
 * The function is called after returning the end of the current transfer
 * Returns:
 * Return 0 as controller IDLE,
 * Return a negative errno as controller ACTIVE
 */ 

>> static int i2c_dw_is_controller_active(struct dw_i2c_dev *dev) {
>> 	u32 status;
>> 
>> 	regmap_read(dev->map, DW_IC_STATUS, &status);
>> 	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
>> 		return 0;
>> 
>> 	return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>> 			!(status & DW_IC_STATUS_MASTER_ACTIVITY),
>> 			1100, 20000);
>> }

>Yes, thank you. This is pure readability wise, you may actually leave the above text as a comment on top of that helper. It will add a value of understanding what's behind the scenes.

> >> >> +}

...

>> I will be off work, If there are still emails that I have not been 
>> replied to, I will reply to your email immediately after going to work tomorrow.

>No problem. Just keep your time, proof-read and test the v9 before sending and I believe it will be the last iteration. Thank you for your patience and energy to push this change forward!

  After the testing and validation are completed, I will resend v9 version.
  Thank you!
>
>...
>
>> Thanks you for your suggestion!

>You are welcome!

------------------------------------------
Best Regards
Kimriver Liu