mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Peng Fan <peng.fan@oss.nxp.com>
To: Sudeep Holla <sudeep.holla@kernel.org>,
	Cristian Marussi <cristian.marussi@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Danilo Krummrich <dakr@kernel.org>,
	Saravana Kannan <saravanak@kernel.org>,
	Hans de Goede <johannes.goede@oss.qualcomm.com>,
	driver-core@lists.linux.dev, linux-kernel@vger.kernel.org,
	imx@lists.linux.dev, Peng Fan <peng.fan@nxp.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH] driver core: hand off fwnode ownership when shared fwnode owner is rejected
Date: Tue, 29 Sep 2026 09:47:42 +0800	[thread overview]
Message-ID: <arsYvrYaQIr1lwbT@shlinux89> (raw)
In-Reply-To: <20260928-conscious-spectral-manul-19bc10@sudeepholla>

Hi Sudeep,

On Mon, Sep 28, 2026 at 01:09:08PM +0100, Sudeep Holla wrote:
>On Mon, Sep 28, 2026 at 07:49:01PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>> 
>> When multiple devices share the same fwnode (e.g. the SCMI bus creates
>> both "pinctrl" and "pinctrl-imx" devices for SCMI_PROTOCOL_PINCTRL), only
>> the first device registered becomes the fwnode owner (fwnode->dev, set in
>
>This has been rejected in the past. Apart from the trigger in -next,
>anything else has changed ?

When both pinctrl-scmi.c and pinctrl-imx-scmi.c are built into the
kernel Image, both drivers call module_scmi_driver() at init time, which
goes through:
  scmi_driver_register()
    → scmi_protocol_table_register(id_table)
      → scmi_protocol_device_request()
        → scmi_device_request_notifier()
          → scmi_create_protocol_devices(fwnode, ..., "pinctrl-imx")
So the on-demand path already creates both "pinctrl" and "pinctrl-imx"
devices for protocol@19, sharing the same fwnode, regardless of
scmi_std_id_table. aac4e67d6eb9 just added a second path that does
the same thing earlier - the fundamental problem existed before it.

This issue has been here for 2 years.
I proposed a patch in scmi side [1][2], but never made into mainline. 
[1] https://lore.kernel.org/all/CAGETcx87Stfkru9gJrc1sf=PtFGLY7=jrfFaCzK5Z4hq+2TCzg@mail.gmail.com/
[2] https://lore.kernel.org/arm-scmi/ZryUgTOVr_haiHuh@pluto/

The issue is not specific to pinctrl w/o imx.

The problem is that two devices share one fwnode, the first one registered
claims fwnode->dev, and if it never binds, driver_bound() of the second
device skips the dangling consumer pickup because fwnode->dev != dev.

>
>> device_add()). If that owner never binds -- for example its driver returns
>> -ENODEV because it is blocklisted on this SoC, or because its driver is
>> not compiled in at all -- then driver_bound() is never called for it, so
>> fwnode_links_purge_suppliers() and fw_devlink_pickup_dangling_consumers()
>> are never run for the fwnode. The child fwnode supplier links (pin group
>> nodes such as lpi2c3grp, uart5grp, ...) stay unsatisfied and every
>> consumer of those child nodes defers probe forever.
>> 
>> On i.MX95 this manifests as a complete boot failure: the generic "pinctrl"
>> SCMI device claims fwnode ownership but its driver returns -ENODEV, while
>> the vendor "pinctrl-imx" device binds successfully. Because
>> dev->fwnode->dev still points at the rejected "pinctrl" device,
>> driver_bound() of "pinctrl-imx" skips the supplier purge and dangling
>> consumer pickup, so all I2C buses, SPI, UART, MMC, USB and PCIe
>> controllers wait forever for their pinctrl suppliers.
>> 
>> Fix this in two places:
>> 
>> 1. In really_probe() failure path: when the driver definitively rejects
>>    a device (-ENODEV / -ENXIO), fw_devlink_release_shared_fwnode() is
>>    called. If the rejected device is the fwnode owner, it either
>>    transfers ownership to an already-bound sibling (and runs the
>>    purge/pickup on its behalf) or clears ownership so the next sibling
>>    to bind can re-acquire it.
>> 
>> 2. In device_links_driver_bound(): re-acquire the fwnode when it is
>>    unowned (!fwnode->dev) or when the current owner has no driver at
>>    all (!fwnode->dev->driver, meaning the driver was never compiled in
>>    or loaded as a module). This covers the case where probe rejection
>>    never happens because no driver ever matches.
>> 
>> fwnode->dev is not serialized by a lock; instead every writer only ever
>> touches a fwnode->dev it already owns (== dev, as device_del() does when
>> it clears ownership) or one that is currently unowned (== NULL, as
>> device_add() does when it claims ownership). This patch follows the same
>> discipline: fw_devlink_release_shared_fwnode() only writes fwnode->dev
>> when this device is the current owner; device_links_driver_bound() only
>> claims fwnode->dev when it is NULL or when the current owner has no
>> driver (and therefore cannot be in the process of binding).
>> 
>> Fixes: f9aa460672c9 ("driver core: Refactor fw_devlink feature")
>> Cc: stable@vger.kernel.org
>> Assisted-by: LLM
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>> This issue is triggered by 
>> aac4e67d6eb9 ("firmware: arm_scmi: Always create devices for standard protocols")
>> in linux-next next-20260925.
>> 
>> But I think this is a fix to
>> f9aa460672c9 ("driver core: Refactor fw_devlink feature")
>> 
>
>Does dropping i.MX specials from list of devices solves the problem ?

No - dropping "pinctrl-imx" from scmi_std_id_table would not fix it
when both drivers are built-in, because scmi_protocol_device_request()
from the driver registration path still creates both devices.

>I am more than happy to drop i.MX special in the code and let you
>sort the pinmux mess you guys have created.

I understand the concern about platform-specific code in the standard
table. But the issue is not specific to pinctrl-imx - it is a generic
fw_devlink gap that affects any bus creating multiple devices per fwnode.
The same structural pattern exists for SCMI_PROTOCOL_PERF ("perf" +
"cpufreq") and SCMI_PROTOCOL_SENSOR ("hwmon" + "iiodev").

Cristian also shared his insights before, in [3].

"
....while other drivers exists that share the usage of the same protocol
(HWMON/IIO GENPD/CPUFREQ), they use the same protocol to achieve different
things in different subsytems...and they are anyway impacted (even to a less
degree) by this fw_devlink issue AFAIU so the problem indeed exist also
out of pinctrl-imx
"

[3] https://lore.kernel.org/all/Z65U2SMwSiOFYC0v@pluto/

>
>And also I remember you creating situation disabling cpufreq in the cmdline.
>Will that be ever used on those i.MX platforms ?

For the PERF pair, both drivers bind successfully today so there is no
issue in practice. But if "perf" (the fwnode owner) fails probe while
"cpufreq" binds, the same fwnode ownership deadlock occurs. This is not
about disabling cpufreq - it is about the owner device failing to bind
for any reason.

>
>I am not against the patch if others are OK.

Thanks. The patch fixes a generic fw_devlink gap in driver_bound()
where a device binding successfully on a shared fwnode cannot resolve
dangling consumers of child fwnodes because it is not the fwnode owner.
The fix follows the existing fwnode->dev ownership discipline (only
writing fwnode->dev when it is NULL, owned by self, or owned by a
driverless device) and handles all three scenarios:
1. Owner probe rejected (-ENODEV/-ENXIO) - owner hands off in
really_probe() failure path
2. Owner rejected earlier, fwnode now unowned - sibling re-acquires
in driver_bound()
3. Owner's driver never compiled/loaded - sibling takes over from
driverless owner in driver_bound()


If driver core maintainers have better solution for the case that
multiple devices share one fwnode, I would appreciate.

Thanks
Peng

>
>-- 
>Regards,
>Sudeep
>
>

  reply	other threads:[~2026-09-29  1:43 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-28 11:49 Peng Fan (OSS)
2026-09-28 12:09 ` Sudeep Holla
2026-09-29  1:47   ` Peng Fan [this message]
2026-09-29  8:35     ` Sudeep Holla
2026-09-29 15:56       ` Sudeep Holla

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=arsYvrYaQIr1lwbT@shlinux89 \
    --to=peng.fan@oss.nxp.com \
    --cc=cristian.marussi@arm.com \
    --cc=dakr@kernel.org \
    --cc=driver-core@lists.linux.dev \
    --cc=gregkh@linuxfoundation.org \
    --cc=imx@lists.linux.dev \
    --cc=johannes.goede@oss.qualcomm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peng.fan@nxp.com \
    --cc=rafael@kernel.org \
    --cc=saravanak@kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=sudeep.holla@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®