linux-kernel - Re: [PATCH v4 2/4] drm/xe: Support for I2C attached MCUs

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: <aF59pnqrLEzSiKVL@smile.fi.intel.com>
Date: Fri, 27 Jun 2025 14:16:54 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@...el.com>,
	Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
	Rodrigo Vivi <rodrigo.vivi@...el.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>, Andi Shyti <andi.shyti@...nel.org>,
	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 v4 2/4] drm/xe: Support for I2C attached MCUs

On Fri, Jun 27, 2025 at 11:56:51AM +0300, Heikki Krogerus wrote:
> On Thu, Jun 26, 2025 at 05:21:02PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 26, 2025 at 04:56:07PM +0300, Heikki Krogerus wrote:

...

> > > +	regmap = devm_regmap_init(i2c->drm_dev, NULL, i2c, &i2c_regmap_config);
> > 
> > Use of i2c->drm_dev makes harder to maintain and understand the code.
> > Managed resources should be carefully attached to the correct device,
> > otherwise it's inevitable object lifetime related issues.
> > 
> > With
> > 
> > 	struct device *dev = xe->drm.dev;
> > 
> > and using local dev, it becomes easier to get and avoid such subtle mistakes.
> 
> I have to disagree with you on this one. Local dev pointers create
> problems because of the assumption that there is only a single device
> in the function to deal with (especially if they are named "dev"),
> which is almost never the case - this function is no exception.

Hmm... In my experience more than 70% of the drivers are okay with this
approach as they do *not* use multiple device pointers. Usually, if we
take IIO for the example, they have a physical device, which is depicted
by the local variable named 'dev' and a Linux IIO device, which is container
that has struct device inside, but it's not used explicitly, IIO APIs use
the pointer to the container (and of course its name differs).

That said, it seems we have different experience (you are most likely talking
about USB cases, where that is indeed quite complicated already).

> But I'll add the local variable as you requested - I'll just name it
> carefully.

Sure.

...

> This kinda related but off topic. IMO in cases like this the regmap
> should be assigned to the child device that is being created instead
> of the parent device. That is currently prevented by the current
> regmap API - the device has to be fully registered before the regmap
> can be assigned (and I'm not referring to the resource managed devm_*
> API), but I'm not convinced that it has to be like that. The problem
> is that the parent device may have multiple child devices that each
> need a dedicated regmag. So just as a note to self: check if we can
> improve the regmap API.

I'm not sure I follow. The (some of) MFD drivers, for instance, work
with many children and one regmapi that is split over the devices.
I don't see any issue with that. The problem is when one tries to mix
regmap and non-regmap approaches in the same (big) driver. That's
a road to mine field.

-- 
With Best Regards,
Andy Shevchenko