linux-kernel - Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

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: <crx77prygiqsi4zjwjtzy2shy4agcdtvj6wcg5qjmpzzysh4j2@7fipw7rhrpmw>
Date: Mon, 30 Jun 2025 20:21:28 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>, 
	Rodrigo Vivi <rodrigo.vivi@...el.com>, Lucas De Marchi <lucas.demarchi@...el.com>, 
	Thomas Hellström <thomas.hellstrom@...ux.intel.com>, Jarkko Nikula <jarkko.nikula@...ux.intel.com>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Mika Westerberg <mika.westerberg@...ux.intel.com>, Jan Dabros <jsd@...ihalf.com>, Raag Jadav <raag.jadav@...el.com>, 
	"Tauro, Riana" <riana.tauro@...el.com>, "Adatrao, Srinivasa" <srinivasa.adatrao@...el.com>, 
	"Michael J. Ruhl" <michael.j.ruhl@...el.com>, intel-xe@...ts.freedesktop.org, linux-i2c@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/4] i2c: designware: Add quirk for Intel Xe

Hi Andy,

On Mon, Jun 30, 2025 at 04:16:56PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 30, 2025 at 02:59:21PM +0300, Heikki Krogerus wrote:
> > On Mon, Jun 30, 2025 at 01:02:56PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 30, 2025 at 11:10:00AM +0300, Heikki Krogerus wrote:
> > > > On Mon, Jun 30, 2025 at 10:30:19AM +0300, Andy Shevchenko wrote:
> > > > > On Fri, Jun 27, 2025 at 05:32:01PM -0400, Rodrigo Vivi wrote:
> > > > > > On Fri, Jun 27, 2025 at 05:13:36PM +0300, Andy Shevchenko wrote:
> > > > > > > On Fri, Jun 27, 2025 at 04:53:11PM +0300, Heikki Krogerus wrote:
> 
> ...
> 
> > > > > > > >  static int dw_i2c_plat_probe(struct platform_device *pdev)
> > > > > > > >  {
> > > > > > > > +	u32 flags = (uintptr_t)device_get_match_data(&pdev->dev);
> > > > > > > 
> > > > > > > > -	dev->flags = (uintptr_t)device_get_match_data(device);
> > > > > > > >  	if (device_property_present(device, "wx,i2c-snps-model"))
> > > > > > > > -		dev->flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > > +		flags = MODEL_WANGXUN_SP | ACCESS_POLLING;
> > > > > > > >  
> > > > > > > >  	dev->dev = device;
> > > > > > > >  	dev->irq = irq;
> > > > > > > > +	dev->flags = flags;
> > > > > > > 
> > > > > > > Maybe I'm missing something, but why do we need these (above) changes?
> > > > > > 
> > > > > > in between, it is introduced a new one:
> > > > > > flags |= ACCESS_POLLING;
> > > > > > 
> > > > > > So, the initialization moved up, before the ACCESS_POLLING, and
> > > > > > it let the assignment to the last, along with the group.
> > > > > 
> > > > > I still don't get. The cited code is complete equivalent.
> > > > 
> > > > This was requested by Jarkko.
> > > 
> > > Okay, but why? Sounds to me like unneeded churn. Can't we do this later when
> > > required?

I don't think it makes sense to add the extra flag later as it
would be a non relevant change, so that I'm fine with having it
here.

> > You need to ask why from Jarkko - I did not really question him on
> > this one. Unfortunately his on vacation at the moment.
> 
> Yeah :-(
> 
> > I can drop this, but then I'll have to drop also Jarkko's ACK.

Just for reference, Ack is not Reviewed. If Jarkko Acks, I assume
he is OK with the general idea, not specifically with the code. I
think that if you change a bit the code without altering the
result you can keep the Ack. Up to you.

> I can give mine if it helps. The code as far as I can see is 100% equivalent.
> 
> > I think we already agreed that this function, and probable the entire
> > file, need to be refactored a bit, so would you mind much if we just
> > went ahead with this as it is?
> > 
> > I'm asking that also because I don't have means or time to test this
> > anymore before I start my vacation.
> 
> I see, then we may ask Andi and Wolfram on this. I slightly prefer to have
> no additional churn added without a good reason.

To be honest I don't really mind. I think that if we add the
extra "flag" or we don't, the amount of code is the same.

If we decide not to add the extra "flag", then we need to move
the initialization of dev->flag above or shuffle something else
around.

In the meantime:

Reviewed-by: Andi Shyti <andi.shyti@...ux.intel.com>

Thanks,
Andi