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

Message-ID: <20251126193251.GA269764@LNDCL34533.neenah.na.plexus.com>
Date: Wed, 26 Nov 2025 13:32:51 -0600
From: Danny Kaehn <danny.kaehn@...xus.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.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

Hi Andy,

Thanks for the review!

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:
> > Support describing the CP2112's I2C and GPIO interfaces in firmware.
> > 
> > Bindings between the firmware nodes and the functions of the device
> > are distinct between ACPI and DeviceTree.
> > 
> > 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.

[1]:
https://lore.kernel.org/all/20240213152825.GA1223720-robh@kernel.org/

[2]:
https://lore.kernel.org/all/ZmISaEIGlxZVK_jf@smile.fi.intel.com/

> ...
> 
> > +	if (is_acpi_device_node(hdev->dev.fwnode)) {
> 
> Please, do not dereference fwnode, use dev_fwnode() or other APIs for that
> (actually the same applies to OF node, but people too much neglect that).
>

Thanks, will do.

> > +		device_for_each_child_node(&hdev->dev, child) {
> > +			ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > +			if (ret)
> > +				continue;
> > +
> > +			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...

> > +			}
> > +		}
> > +	} else {
> > +		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?

> > +	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Thanks,

Danny Kaehn