Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Secumail-id: <9268.650af335.5c40b.0>
Message-ID: <ba6d4378-b646-4514-3a45-4b6c951fbb9c@kalrayinc.com>
Date:   Wed, 20 Sep 2023 15:27:10 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v2] i2c: designware: Fix corrupted memory seen in the
 ISR
To:     Wolfram Sang <wsa@kernel.org>,
        Serge Semin <fancer.lancer@gmail.com>,
        Jan Bottorff <janb@os.amperecomputing.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Yann Sionneau <yann@sionneau.net>,
        Will Deacon <will@kernel.org>,
        Jarkko Nikula <jarkko.nikula@linux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Jan Dabros <jsd@semihalf.com>,
        Andi Shyti <andi.shyti@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
References: <xxnggfauhkfum63p5bkgxsu3m5odyjda7pnwpb5ocwf4gez7fh@4lu6qyqy6dvh>
 <37e10c3d-b5ab-75ec-3c96-76e15eb9bef8@sionneau.net>
 <v4hdblxwhl6ncdfxre5gyrve7bgdsorfqpqj53ib6q4tr7aguy@4kfr6ergb3jn>
 <9de89e14-35bd-415d-97f1-4b6db1258997@os.amperecomputing.com>
 <ZQlwC9TCSwWJpuxy@arm.com> <ZQl1zwVkx9n2MPvr@shikoro>
 <da400d3e-a357-1ae8-cb92-728cc4974b67@kalrayinc.com>
 <ZQm1UyZ0g7KxRW3a@arm.com>
 <cde7e2fc-2e13-4b82-98b3-3d3a52c4c185@os.amperecomputing.com>
 <p7wl7fk4cdyhvw2mfsa6sfc7dhfls3foplmzwj6pzstargt2oh@33zuuznup2gq>
 <ZQq2cT+/QCenR5gx@shikoro>
Content-Language: en-us
From:   Yann Sionneau <ysionneau@kalrayinc.com>
In-Reply-To: <ZQq2cT+/QCenR5gx@shikoro>
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: GdVyjF7Ne+lUCTyLzNKZB21/KnEWw0+BETEUWZMAxxdIwXksdpOhw2SF5JRknk1aU7qjESCphJ9ZxGEDzFc/Ot4kEVmtHhp44I9mR6UCyvDJHBt7yp4bajkMqBMA7KSzKUBySNb8BoH2FwxBb6IVCMKOCjde7DVM7IP0MXzMbZ4tz8xs1SIzKMf0VDjFysf8JOlYUKMb/RpODG8EcXXA3wOlQjspFnZBRX283lSEn1jtSsUuLL6ln46VpXLcKuJioZfbVK54aHYCvrO5qm+kYMxUB4i4l2rODf//8zLaOw3SyqvWkgE43qIw+j/8h++VNW+9Oz9u/6wwKdJ4Y3hoHGEE9NrBkPe3gtBRDTFLchpIX8FQ4IMIyv0bzAQYPJTRipSSMvPidAdPRIc3/HHj9ZfWYnzdnwsGULl113JWCV5N+n6moA5laDsIprlnz0oG7IlGKD8OA0yB6eTzThbd7WWLHWSfn1bKmGLa6H4A1eMAa/2zFeg821SXrHrKjKt+IbWjKYInyeQ5t09IsJSt9nceTiKVvfy5ugwWpx5N5V/sM5iPkXk34NVTu0zTcIH7Ggg61gqKOWJVC3TLnok0bS2kM+w/FpNF6mAaO5VT4G/UN6PCiIz3FCkBWVMVp9aRYB4riQnCBG8Ul8s9COMq/ASSJoHQu4BcSLGEYjlcUKFqyvMjjLx2KGgNDkPaiz1DQha75TFhalzMD1bPX9nvBHZJIEIMNK9etIW8spn/eYPitEYOEDJnyvwmcJainkyVAFplmauJY41AxaM95ITYIP/6tve19ShpX0BOD5UwIAebU0zKKELXOb8l2rVP+OTTyZ6BO/DKu4DZMP68Zj13vFrGYJWndB4+fUhIFwoTUlAxf82XSGlpuExqIm8i0od/nWevFCPF1l1wgXn7mYNXzRfQzbpH0bn5U9/ZUSsHqqAryFsuceZLn7WOKzoIQ4ea
 YU86vKgzXmbMKSw7idfzJsY0gUVeo1vpAYZquhGxfuhmvJH14C9sTkOH66HoaKp12KpLrdmzSoZI4mPV6cvOXaokGVNQgJGmZ8NAHa16MdXzwge9n8hDoVkhKeGU/blXJz3vdpuY+8Vn82727nJjv+Al8+QUY/WaJcgqLXy+Bf8nj7MQRriR0/Lj79S2oBIfwCZYonqZg0tXRs4H5DUh+k7tZcNCWIgc5GGuStH1Wc7zId4QtzTBz6j1mIW+smLTr8XYwUmVuXpmi75aaz5npvn4iZ1DCHEPOcsGOlzf7zxtcieygH1SDWEsVdjyU1PLgv0aoNoJ0vKPpiTRaPvlrXc2isblxsjqB70Q5+GuXlzDq5uWb9sjE8uV1LTqJo1M281a2jPDapsoJ76vzgfALCCjg3YVYp8UsvbeVArcg9TVJz+LGrm82rsURsqcksdkXL1IYrd/NESnafNCoKvMrHLZ+OctC0YsTQX+1Yyt0XH1c8vtmjO/49XOcOy2G6S1Wr+aOENwjVvscgqGxxAn8P1f1LON80gZz9vbYSFCVXq53F4nFIrQstjL7kBXa4+QdpnRJ0g4Irz9ZY+wu7WtGcVTiQHKaKbif5yGWFY915KN3yv2wHy6ScIhuqwKhXvqpD06irPL3hLVbEtSw/CW7lipRuYLjs3Kdelkqfr7wf9wqg+r65K6wT+XfVjAA6NACo/8/e7MTBZCcOjAOHiFrQ==
X-OriginatorOrg: kalrayinc.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 95fab514-f22c-4066-7585-08dbb9dd4c89
X-MS-Exchange-CrossTenant-AuthSource: MR1P264MB1890.FRAP264.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Sep 2023 13:27:12.7361
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8931925d-7620-4a64-b7fe-20afd86363d3
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: S2nZxTdRzwQSq4ejnKDAqrMYfpj6oJVGQK62xw4UdVafJ3uZ3YFvFWEXya4ODmfkJpFYpNBzvPHd7+sB8J5hGw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MR1P264MB2339
X-ALTERMIMEV2_out: done
X-Spam-Status: No, score=-2.3 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Wed, 20 Sep 2023 06:27:36 -0700 (PDT)

Hi,

On 20/09/2023 11:08, Wolfram Sang wrote:
>> same thread." [1] Thus I'd suggest the next fix for the problem:
>>
>> --- a/drivers/i2c/busses/i2c-designware-common.c
>> +++ b/drivers/i2c/busses/i2c-designware-common.c
>> @@ -72,7 +72,10 @@ static int dw_reg_write(void *context, unsigned int reg, unsigned int val)
>>   {
>>   	struct dw_i2c_dev *dev = context;
>>   
>> -	writel_relaxed(val, dev->base + reg);
>> +	if (reg == DW_IC_INTR_MASK)
>> +		writel(val, dev->base + reg);
>> +	else
>> +		writel_relaxed(val, dev->base + reg);
>>   
>>   	return 0;
>>   }
>>
>> (and similar changes for dw_reg_write_swab() and dw_reg_write_word().)
>>
>> What do you think?
> To me, this looks reasonable and much more what I would have expected as
> a result (from a high level point of view). Let's hope it works. I am
> optimistic, though...
>
It works if we make sure all the other register accesses to the 
designware i2c IP can't generate IRQ.

Meaning that all register accesses that can trigger an IRQ are enclosed 
in between a call to i2c_dw_disable_int() and a call to 
regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_MASTER_MASK); or 
equivalent.

It seems to be the case, I'm not sure what's the best way to make sure 
it will stay that way.

Moreover, maybe writes to IC_ENABLE register should also use the 
non-relaxed writel() version?

Since one could do something like:

[ IP is currently disabled ]

1/ enable interrupts in DW_IC_INTR_MASK

2/ update some variable in dev-> structure in DDR

3/ enable the device by writing to IC_ENABLE, thus triggering for 
instance the TX_FIFO_EMPTY irq.

Regards,

-- 

Yann