linux-kernel - Re: [PATCH v4 1/4] i2c: imx: only poll for bus busy in multi master mode

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] [day] [month] [year] [list]

Message-ID: <bcdd6cae28edd9dd05a71118f9979e7460688775.camel@pengutronix.de>
Date: Wed, 02 Oct 2024 17:16:35 +0200
From: Lucas Stach <l.stach@...gutronix.de>
To: Stefan Eichenberger <eichest@...il.com>
Cc: o.rempel@...gutronix.de, kernel@...gutronix.de, andi.shyti@...nel.org, 
 shawnguo@...nel.org, s.hauer@...gutronix.de, festevam@...il.com,
 Frank.Li@....com,  imx@...ts.linux.dev, linux-kernel@...r.kernel.org,
 Stefan Eichenberger <stefan.eichenberger@...adex.com>,
 linux-i2c@...r.kernel.org,  francesco.dolcini@...adex.com,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 1/4] i2c: imx: only poll for bus busy in multi master
 mode

Am Mittwoch, dem 02.10.2024 um 16:56 +0200 schrieb Stefan Eichenberger:
> On Wed, Oct 02, 2024 at 04:40:32PM +0200, Lucas Stach wrote:
> > Am Mittwoch, dem 02.10.2024 um 13:19 +0200 schrieb Stefan Eichenberger:
> > > From: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> > > 
> > > According to the i.MX8M Mini reference manual chapter "16.1.4.2
> > > Generation of Start" it is only necessary to poll for bus busy and
> > > arbitration lost in multi master mode. This helps to avoid rescheduling
> > > while the i2c bus is busy and avoids SMBus devices to timeout.
> > > 
> > This is a backward incompatible change, as far as I can see. Until now
> > the driver would properly handle a multi-mastered bus, without any
> > specific configuration. Now it requires the new multi-master DT
> > property to be set, which isn't even documented in the binding to be
> > understood by this driver.
> > 
> > Are you sure that every single instance of a i.MX i2c bus is only
> > single mastered?
> > 
> > If this is a worthwhile performance improvement I guess you need to
> > flip the logic around by adding a new single-master DT property (or
> > something along those lines), which should go through proper DT binding
> > review. You can then use this property for boards/busses to opt into
> > skipping the arbitration lost check.
> 
> According to the discussion here the property documentation should not
> be added:
> https://lore.kernel.org/linux-i2c/2bbddaxyjkxfmlgmq3yqcbzo7dsb2pq5bvdatk2y4ig4iintkt@35btqkdv7sy3/
> 
Ah, I wasn't aware of that.

> However, the point regarding single-master and multi-master is correct.
> We also discussed this internally and assumed the single-master use case
> is more likely to be the default and that this patch series would fix
> issues for other devices out there.
> 
I agree that the vast majority of busses is single master only and I
can see why you did it this way.

However, I still think it's a risky change, as some boards/DTs may rely
on the fact that the driver implicitly handled multi-master until now
and we can't retroactively change DTs in the wild.

I just looked up the dtschema for i2c-controller and there is a
"single-master" property defined already. I think it would be good if
you used this instead to have boards opt in to this optimization.

Regards,
Lucas

>  However, your point is valid and if
> preferred I can change it to single-master with the next version.
> 
> Francesco Dolcini <francesco.dolcini@...adex.com> I think you once had a
> discussion regarding multi master mode for i2c on i.MX devices? Maybe
> you can remember the details?
> 
> Regards,
> Stefan