Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <95d838d5-ea27-ef75-4ac6-575ebd75111a@amperemail.onmicrosoft.com>
Date:   Fri, 2 Jun 2023 11:30:12 +0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.2
Subject: Re: [PATCH v1] i2c: designware: Handle invalid SMBus block data
 response length
Content-Language: en-US
From:   Tam Chi Nguyen <tamnguyenchi@amperemail.onmicrosoft.com>
To:     Jarkko Nikula <jarkko.nikula@linux.intel.com>,
        Tam Nguyen <tamnguyenchi@os.amperecomputing.com>,
        linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org
Cc:     patches@amperecomputing.com, andriy.shevchenko@linux.intel.com,
        mika.westerberg@linux.intel.com, jsd@semihalf.com,
        chuong@os.amperecomputing.com, darren@os.amperecomputing.com,
        stable@vger.kernel.org
References: <20230523082118.10935-1-tamnguyenchi@os.amperecomputing.com>
 <bf392104-96b0-a85b-f68e-088bd3884d9a@linux.intel.com>
 <6def5691-2462-9707-f75a-09931d26d4af@amperemail.onmicrosoft.com>
In-Reply-To: <6def5691-2462-9707-f75a-09931d26d4af@amperemail.onmicrosoft.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bkRaTVI0YTVWaldPQzBKR3Y5YkNBeldHRUs3NzROcXhxb281ZnlVU1BVaXZJ?=
 =?utf-8?B?enQvd2E0WmRvU3hoNnlONm05T1RndGhQdjRCV1lsM0JLa3JZVzZudG9PZWtM?=
 =?utf-8?B?b1htMzdaL3ozcW9Xa1NYdFZUeE1SR2pCUUhudktBYm9YN3ZXeGNvdEd5a2hQ?=
 =?utf-8?B?OS9QWWE4YmwzQnZyZ1orQm1ZQ3dmTmdsRG9JajM3Yk81L2lFU2d3bElhcVh4?=
 =?utf-8?B?bmdsdEdkak01RGZlM2NmL3Z6dDdGRnYzWW1NN0x0U3pyZU96blBtOGQrd1h0?=
 =?utf-8?B?OTZZU0czY1c4eGtwdUxMdDZnUTNSL2ZzeGNISWhyWkFoOWxVaDVNSGdpWHRo?=
 =?utf-8?B?VmhwT2lIaGVKZStSZFlqQTZuSUdmQS95VzlrVTVmSnIrNFlPVVgzSUJLdkFz?=
 =?utf-8?B?bXVlSEN2TitVNFpBWWZEMnVUSHpPdG81aEprQm9QdnovODU1T0dnR051Kzcz?=
 =?utf-8?B?YTFRNHBtVVdmMFBtZXdXZ1BkVlZLNVY4MFFFYzhVNzF6aG1MNC84VURqejQ5?=
 =?utf-8?B?STAzQ2pDenlRcU9HaWgzOURUemxuclNkNUI0UXhkWTFCTTd1Sm1OTE5kbC81?=
 =?utf-8?B?d3NMM3JYdUxBNlZ3YmF3K0pxNmQ3QUtTbk5udm4rTmNlUFF3dUJIY2VzYjVp?=
 =?utf-8?B?UWNLTHJWcHNNY3dVYnpoanJMUU1DZG9tZThDdE1xY1BEczRrUi9QZnN3L0I2?=
 =?utf-8?B?RWlkNjlNQ1graWxYOUhwMkpFREVCelpqdnpSb3hDbS9TSkFmSjkzT2dad2tl?=
 =?utf-8?B?TnR5dWFiZEJWOFNOK0FwUlFWdld0bmtMVllDS0tUUnVlQWY2OHpnMmdQRWs0?=
 =?utf-8?B?Zk1zdmpKVnFSNmpUNVFiajRPaE50aGhXa1VzM3lyMzQ1L0pJcDI1Y3VZdFNI?=
 =?utf-8?B?YmY4eHphb25sSTI3Wk9IUC83S2NYUWUydlhaVys3MFUyZVdnLzdubldicy8y?=
 =?utf-8?B?cmh0RGtNZFp6RDA4Nk5NZ0VjL2VKOVc4QTNvYnZOVkllTlZqaFJYeTNGVmVW?=
 =?utf-8?B?Y3crV3l5RkFUVklFam92M1VmV0VNdmJHeURnNEVHejViRmhNVEFpZ3ZXeCtm?=
 =?utf-8?B?Y0dkamZaSmZ5d0p6RzNPdHYzeFEwTm5jZFEzVndOVCs0MjlyUDdIa1ZXV2F0?=
 =?utf-8?B?WVhmcXZzU2orcjBGQmRhdUxWNFgvY2hRRktWclNML2NLMlE4WEFOdXY5MVc2?=
 =?utf-8?B?UGpEZHlWS1FWZmlSeWxITWNEeG96OFY5MjZTdnpGTEdpaTZyQ0ZBT3pkOXpl?=
 =?utf-8?B?NnhvQTkrMVdzRWltRWpCNjg2L2JxZkNmbHZtTG1xc0xlZWFSK1kxMktVUity?=
 =?utf-8?B?Q0FTcUxjOTdLT0FxbWtZeEJjWUd5RW5abHptNUExRXVuT0QxK3hxWEtGSE9P?=
 =?utf-8?B?NEw5Z2hJU2MraTU4Y1o3SGRZUXF2cEJvQTYrZXk3dlVVbTRlMDFHUlVmL3Vv?=
 =?utf-8?B?ZUQ0anJPbzNJbGx5dzVQcGlzbENLT29waS9pLzQyZms4TGxKT2JYQzZMeXcw?=
 =?utf-8?B?czJnb2J1Z1dDQzB2ZUV2MW52OVRJYWVPZ1R0ekFXYk5Cb1ZCd3NJSU5SVk5D?=
 =?utf-8?B?bVYyS0JpbzAzMStFeUhzUU1mSGx5TUVVWmRqd1c5TDRFNnc4VjBjbWZkMjU1?=
 =?utf-8?B?RWg3cUswWW9oZnhFZ04zRjZjSGU5NEVjL21vMG9xMVlRdTY0Yjh4N1lrRlcv?=
 =?utf-8?B?RmxhSzhxZUdjcS94YzdkUjR4SFlpNUFvOEJ0Q0pVbWNSYkdwYVBFOWtrbmVO?=
 =?utf-8?B?ZDdTS3RuT1ZKajRtbFNURGJQenIwL3NwSFEzUnJmNWh5N3Z0Y0luSlVXR0RB?=
 =?utf-8?B?ZysrNU5KSi9sZUhTaDdXZGl3bUtkK2hZUjhpTGRYWCtkWHZiZlNaUE5EZ2F6?=
 =?utf-8?B?aFo2MGFtNzExT080cmNsbTczV2VNQ2UrTXVwT0ppZGl2U3VYeURRS21YT2lv?=
 =?utf-8?B?ajdNUUhBS1RGRWZ2b0o2bU5ubE5TMG1DSEtWZzQwbjczNHh2OG5YYVBnMnp5?=
 =?utf-8?B?bk5ZTThVU2xEUG9IUUI4amUwRDhtRXBEYmY4dmpuWlhMc2hZczNKaURaNCtr?=
 =?utf-8?B?aEFYZVI3bWg5aFFHYTFRNC9zcjdKSk1ONi9pdURYbUcwZmljSGJRQW43Tytx?=
 =?utf-8?B?bFRudkhTeTdPV0krWE9aZW8vc3p6RFhRRjZqSi9JcjBkQi9OcTZRakY5d0ZS?=
 =?utf-8?Q?PHQRpmT6DToy3CKeF4VUhf0kK/dgOGwcr+Q/c4L3Mv0T?=
X-OriginatorOrg: amperemail.onmicrosoft.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 70b3af44-e0ef-4be0-4a4a-08db63221375
X-MS-Exchange-CrossTenant-AuthSource: CY4PR01MB2840.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Jun 2023 04:30:21.4498
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: gat9zokjjMVWQrDCAvC9e6FG5P2rTj12hqA9Gqdk4vuZkFCkGfidTEMQOU3BYoTVeLy9H7DZOz6IMG46KuMmy2HM40v4d1HlC5F1d9SJ7UjyNGufsgTD018Mb0KuZh17
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR01MB8564
X-Spam-Status: No, score=-1.3 required=5.0 tests=BAYES_00,DKIM_INVALID,
        DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,
        HEADER_FROM_DIFFERENT_DOMAINS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Jarkko,

Before pushing the v2 patch, I have one more question to ask.

The commit f53f15ba5a85 ("i2c: designware: Get right data length"), you 
mentioned,
does not handle bit 11 set (on a HW where it's supported) correctly.
"tmp" was not marked with DW_IC_DATA_CMD_DAT when passing to 
i2c_dw_recv_len function.

So I plan to update it in the v2 patch by adding this
     regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
     tmp &= DW_IC_DATA_CMD_DAT;

My question is: does it need a separate patch for this change?

Best regards,
Tam Nguyen


On 5/25/2023 16:30, Tam Chi Nguyen wrote:
> Hi Jarkko
>
> On 5/24/2023 19:33, Jarkko Nikula wrote:
>> Hi
>>
>> On 5/23/23 11:21, Tam Nguyen wrote:
>>> In I2C_FUNC_SMBUS_BLOCK_DATA case, the I2C Designware driver does not
>>> handle correctly when it receives the length of SMBus block data
>>> response from SMBus slave device, which is outside the range 1-32 
>>> bytes.
>>> Consequently, the I2C Designware bus is stuck and cannot recover.
>>> Because if IC_EMPTYFIFO_HOLD_MASTER_EN is set, which cannot be detected
>>> from the registers, the controller can be disabled if the STOP bit 
>>> is set.
>>> But it is only set after receiving block data response length.
>>>
>>> Hence, to prevent the bus from stuck condition, after receiving the
>>> invalid block data response length, the driver will read another byte
>>> with STOP bit set.
>>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Tam Nguyen <tamnguyenchi@os.amperecomputing.com>
>>> ---
>>>   drivers/i2c/busses/i2c-designware-master.c | 15 +++++++++++++--
>>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-master.c 
>>> b/drivers/i2c/busses/i2c-designware-master.c
>>> index 55ea91a63382..94dadd785ed0 100644
>>> --- a/drivers/i2c/busses/i2c-designware-master.c
>>> +++ b/drivers/i2c/busses/i2c-designware-master.c
>>> @@ -527,8 +527,19 @@ i2c_dw_read(struct dw_i2c_dev *dev)
>>>                 regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
>>>               /* Ensure length byte is a valid value */
>>> -            if (flags & I2C_M_RECV_LEN &&
>>> -                (tmp & DW_IC_DATA_CMD_DAT) <= I2C_SMBUS_BLOCK_MAX 
>>> && tmp > 0) {
>>> +            if (flags & I2C_M_RECV_LEN) {
>>> +                /*
>>> +                 * if IC_EMPTYFIFO_HOLD_MASTER_EN is set, which 
>>> cannot be
>>> +                 * detected from the registers, the controller can be
>>> +                 * disabled if the STOP bit is set. But it is only set
>>> +                 * after receiving block data response length in
>>> +                 * I2C_FUNC_SMBUS_BLOCK_DATA case. That needs to read
>>> +                 * another byte with STOP bit set when the block data
>>> +                 * response length is invalid to complete the 
>>> transaction.
>>> +                 */
>>> +                if ((tmp & DW_IC_DATA_CMD_DAT) > 
>>> I2C_SMBUS_BLOCK_MAX || tmp == 0)
>>> +                    tmp = 1;
>>> +
>>>                   len = i2c_dw_recv_len(dev, tmp);
>>>               }
>>>               *buf++ = tmp;
>>
>> Looks otherwise good to me but I'm wondering the "tmp == 0" test can 
>> it have the bit 11 set (on a HW where it's supported) and should it 
>> be covered with DW_IC_DATA_CMD_DAT mask too? Please see commit 
>> f53f15ba5a85 ("i2c: designware: Get right data length").
>
> You're right. We should use the DW_IC_DATA_CMD_DAT mask to get the 
> data part if HW supports reading bit 11 from IC_DATA_CMD. I will 
> update it in v2.
>
> Best regards,
> Tam Nguyen
>