linux-kernel - Re: [PATCH v12 2/3] HID: cp2112: Fwnode Support

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: <aSdvv3Qss5oz_o6P@smile.fi.intel.com>
Date: Wed, 26 Nov 2025 23:23:11 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Danny Kaehn <danny.kaehn@...xus.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Benjamin Tissoires <bentiss@...nel.org>,
	Andi Shyti <andi.shyti@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Jiri Kosina <jikos@...nel.org>,
	devicetree@...r.kernel.org, linux-input@...r.kernel.org,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
	Ethan Twardy <ethan.twardy@...xus.com>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org, Leo Huang <leohu@...dia.com>,
	Arun D Patil <arundp@...dia.com>, Willie Thai <wthai@...dia.com>,
	Ting-Kai Chen <tingkaic@...dia.com>
Subject: Re: [PATCH v12 2/3] HID: cp2112: Fwnode Support

On Wed, Nov 26, 2025 at 01:32:51PM -0600, Danny Kaehn wrote:
> On Wed, Nov 26, 2025 at 08:27:19PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 26, 2025 at 11:05:25AM -0600, Danny Kaehn wrote:

...

> > > For ACPI, the i2c_adapter will use the child with _ADR Zero and the
> > > gpio_chip will use the child with _ADR One. For DeviceTree, the
> > > i2c_adapter will use the child with name "i2c", but the gpio_chip
> > > will share a firmware node with the CP2112.
> > 
> > Hmm... Is there any explanation why DT decided to go that way?
> 
> I don't have an explanation, but Rob H. had directed that I make this
> change in [1].
> 
> In v11, I then removed that child node for both ACPI and DT, hoping to
> maintain unity, but you had directed that wouldn't be intuitive for ACPI
> in [2].
> 
> Thus, in this v12, I have just entirely split the two, as it seemed
> unlikely that any compromise to unify the schema between the two
> firmware languages would be possible for a change/driver this
> inconsquential to the overall kernel.

Even though, would be nice to try to get a rationale from Rob on this.
Then we can put it in the commit message to explain. Otherwise it will
confuse history diggers in the future.

> [1]:
> https://lore.kernel.org/all/20240213152825.GA1223720-robh@kernel.org/
> 
> [2]:
> https://lore.kernel.org/all/ZmISaEIGlxZVK_jf@smile.fi.intel.com/

...

> > > +			switch (addr) {
> > > +			case CP2112_I2C_ADR:
> > > +				device_set_node(&dev->adap.dev, child);
> > > +				break;
> > > +			case CP2112_GPIO_ADR:
> > > +				dev->gc.fwnode = child;
> > > +				break;
> > 
> > If by any chance we have malformed table and there are more devices with
> > the same address? Maybe we don't need to address this right now, just
> > asking... (I believe ACPI compiler won't allow that, but table can be
> > crafted directly in the binary format.)
> >
> 
> You're sugggesting perhaps that we explicitly keep track of which
> addresses have been encountered, and refuse to do any fwnode parsing
> if we detect the same address used twice? I believe the current behavior
> would be that the "last node wins"; not sure if it should be a "first node
> wins" or a full error scenario...

I'm suggesting to think about this, not acting right now. I don't believe in
such a case IRL.

> > > +			}

...

> > > +		device_set_node(&dev->adap.dev,
> > > +			device_get_named_child_node(&hdev->dev, "i2c"));
> > 
> > Here we bump the reference count, where is it going to be dropped?
> > 
> > Note, in the other branch (ACPI) the reference count is not bumped in
> > the current code.
> 
> Great point, forgot that I had dropped that handling in v9. The old
> behavior was that the CP2112 driver maintained a reference to each node
> during the lifetime of the device (and released during probe errors,
> etc..). I'm still a bit confused as to whether that is correct or not,
> or if the references should immediately be dropped once they're done
> being parsed during probe()... My understanding previously was that I
> should keep the reference count for the child fwnodes for the lifetime
> of the CP2112, since the pointers to those are stored in the child
> devices but would usually be managed by the parent bus-level code, does
> that seem correct?

While there is a (theoretical) possibility to have lifetime of fwnode shorter
than a device's, I don't think we have or ever will have such a practical
example. So, assumption is that, the fwnode that struct device holds has
the same or longer lifetime.

Note, I haven't investigated overlays (DT and ACPI) behaviour. IIRC you
experimented with ACPI SSDT on this device, perhaps you can try to see
what happens if there is a confirmed that the above is not only a theoretical
problem.

TL;DR: I would drop reference count just after we got a respective fwnode.

-- 
With Best Regards,
Andy Shevchenko