linux-kernel - RE: [PATCH v11] i2c: designware: fix controller 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: <cf1272474d3342e88798edc77365b420@siengine.com>
Date: Fri, 13 Sep 2024 09:32:11 +0000
From: Liu Kimriver/刘金河 <kimriver.liu@...ngine.com>
To: Andi Shyti <andi.shyti@...nel.org>
CC: Andy Shevchenko <andy.shevchenko@...il.com>,
        "jarkko.nikula@...ux.intel.com" <jarkko.nikula@...ux.intel.com>,
        "andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>,
        "mika.westerberg@...ux.intel.com" <mika.westerberg@...ux.intel.com>,
        "jsd@...ihalf.com" <jsd@...ihalf.com>,
        "linux-i2c@...r.kernel.org"
	<linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        Andy Shevchenko <andy@...nel.org>
Subject: RE: [PATCH v11] i2c: designware: fix controller is holding SCL low
 while ENABLE bit is disabled

HI,
 Thank you for taking the time to review the patch and considering it.
 Thanks a bunch!

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

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

> Hi Kimriver,

> please, don't send v12 anymore, I will take care of these little notes from Andy.

> You did a great job at following up on all the reviews, thanks!

> Andi

> On Fri, Sep 13, 2024 at 08:31:18AM GMT, Liu Kimriver/刘金河 wrote:
> > Hi Andy
>>   
>>    Subject:  [PATCH v11] i2c: designware: fix controller is holding SCL low while ENABLE bit is disabled
>>    I will change the subject to：
>>    Subject:  [PATCH v11] i2c: designware: fix controller is holding SCL 
>>  low while the ENABLE bit is disabled
>>  
> > >-----Original Message-----
>>  >From: Andy Shevchenko <andy.shevchenko@...il.com>
> > >Sent: 2024年9月13日 15:41
> > >To: Liu Kimriver/刘金河 <kimriver.liu@...ngine.com>
> > >Cc: jarkko.nikula@...ux.intel.com; andriy.shevchenko@...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; Andy >Shevchenko <andy@...nel.org>
>> >Subject: Re: [PATCH v11] i2c: designware: fix controller is holding 
>> >SCL low while ENABLE bit is disabled
>> 
>> >On Fri, Sep 13, 2024 at 6:35 AM Kimriver Liu <kimriver.liu@...ngine.com> wrote:
> >>>
> >>> It was observed that issuing the ABORT bit (IC_ENABLE[1]) will not 
> >>>work when IC_ENABLE is already disabled.
> >>>
> >>> Check if the ENABLE bit (IC_ENABLE[0]) is disabled when the 
> >>> controller is holding SCL low. If the ENABLE bit is disabled, the 
> >>> software needs to enable it before trying to issue the ABORT bit. 
> >>> otherwise, the controller ignores any write to ABORT bit.
> >>>
> >>>These kernel logs show up whenever an I2C transaction is attempted 
> >>> after this failure.
> >>> i2c_designware e95e0000.i2c: timeout waiting for bus ready 
> >>> i2c_designware e95e0000.i2c: timeout in disabling adapter
> >>>
> >>> The patch fixes the issue where the controller cannot be disabled 
> >>> while SCL is held low if the ENABLE bit is already disabled.
> >>
> >>...
> >>
> >>> +                       /*Set ENABLE bit before setting ABORT*/
> >
> >       /* Set ENABLE bit before setting ABORT */
> >
> >>Missing spaces
> >>
> >>...
>> 
>> >> +/*
>> >> + * This function waits controller idling before disabling I2C
>> 
>> >waits for controller
>> 
>>  + * This function waits for controller idling before disabling I2C
>> 
>> >> + * When the controller is not in the IDLE state,
> >>> + * MST_ACTIVITY bit (IC_STATUS[5]) is set.
> >>> + * Values:
> >>> + * 0x1 (ACTIVE): Controller not idle
>> >> + * 0x0 (IDLE): Controller is idle
>> >> + * The function is called after returning the end of the current 
>> >> + transfer
>> >> + * Returns:
>> 
>> >> + * False when controller is in IDLE state.
>> >> + * True when controller is in ACTIVE state.
>> 
> >>Yeah, I know that this is a copy of what I suggested, but if we going 
> >>to amend, these should be with definite article
>> 
>> > * False when the controller is in the IDLE state.
>> > * True when the controller is in the ACTIVE state.
>> 
>>  
>> 
>> >> + */
>> 
>> >...
>> 
>> >> +       return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
>> >> +                                      !(status & DW_IC_STATUS_MASTER_ACTIVITY),
>> >> +                                      1100, 20000) != 0;
>> 
>> >You broke the indentation again.
>> 
>> it has been indented and aligned from the web：
>> https://lore.kernel.org/all/4ebc4e8882a52620cbca30f1bf25650cbc3723fb.1
>> 726197817.git.kimriver.liu@...ngine.com/
>> 
>> Thanks！
>> 
>> >--
 ------------------------------------------
 Best Regards
 Kimriver Liu