| 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
| ||
|
Message-ID: <20250106043715.GA14389@localhost.localdomain>
Date: Mon, 6 Jan 2025 12:37:15 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Cristian Marussi <cristian.marussi@....com>
Cc: Peng Fan <peng.fan@....com>, Sudeep Holla <sudeep.holla@....com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Saravana Kannan <saravanak@...gle.com>,
Linus Walleij <linus.walleij@...aro.org>,
Aisheng Dong <aisheng.dong@....com>,
Fabio Estevam <festevam@...il.com>, Shawn Guo <shawnguo@...nel.org>,
Jacky Bai <ping.bai@....com>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Sascha Hauer <s.hauer@...gutronix.de>,
"arm-scmi@...r.kernel.org" <arm-scmi@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
"imx@...ts.linux.dev" <imx@...ts.linux.dev>
Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting fwnode for
scmi cpufreq
On Thu, Jan 02, 2025 at 05:06:57PM +0000, Cristian Marussi wrote:
>On Thu, Jan 02, 2025 at 07:38:06AM +0000, Peng Fan wrote:
>> > Subject: Re: [PATCH 1/4] firmware: arm_scmi: bus: Bypass setting
>> > fwnode for scmi cpufreq
>> >
>> > On Fri, Dec 27, 2024 at 03:13:06PM +0000, Sudeep Holla wrote:
>> > > On Wed, Dec 25, 2024 at 04:20:44PM +0800, Peng Fan (OSS) wrote:
>> > > > From: Peng Fan <peng.fan@....com>
>> > > >
>> > > > Two drivers scmi_cpufreq.c and scmi_perf_domain.c both use
>> > > > SCMI_PROTCOL_PERF protocol, but with different name, so two
>> > scmi
>> > > > devices will be created. But the fwnode->dev could only point to
>> > one device.
>> > > >
>> > > > If scmi cpufreq device created earlier, the fwnode->dev will point
>> > > > to the scmi cpufreq device. Then the fw_devlink will link
>> > > > performance domain user device(consumer) to the scmi cpufreq
>> > device(supplier).
>> > > > But actually the performance domain user device, such as GPU,
>> > should
>> > > > use the scmi perf device as supplier. Also if 'cpufreq.off=1' in
>> > > > bootargs, the GPU driver will defer probe always, because of the
>> > > > scmi cpufreq device not ready.
>> > > >
>> > > > Because for cpufreq, no need use fw_devlink. So bypass setting
>> > > > fwnode for scmi cpufreq device.
>> > > >
>> >
>> > Hi,
>> >
>> > > > Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the
>> > > > scmi_device")
>> > > > Signed-off-by: Peng Fan <peng.fan@....com>
>> > > > ---
>> > > > drivers/firmware/arm_scmi/bus.c | 15 ++++++++++++++-
>> > > > 1 file changed, 14 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/drivers/firmware/arm_scmi/bus.c
>> > > > b/drivers/firmware/arm_scmi/bus.c index
>> > > >
>> > 157172a5f2b577ce4f04425f967f548230c1ebed..12190d4dabb654845
>> > 43044b442
>> > > > 4fbe3b67245466 100644
>> > > > --- a/drivers/firmware/arm_scmi/bus.c
>> > > > +++ b/drivers/firmware/arm_scmi/bus.c
>> > > > @@ -345,6 +345,19 @@ static void __scmi_device_destroy(struct
>> > scmi_device *scmi_dev)
>> > > > device_unregister(&scmi_dev->dev);
>> > > > }
>> > > >
>> > > > +static int
>> > > > +__scmi_device_set_node(struct scmi_device *scmi_dev, struct
>> > device_node *np,
>> > > > + int protocol, const char *name) {
>> > > > + /* cpufreq device does not need to be supplier from devlink
>> > perspective */
>> > > > + if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name,
>> > "cpufreq"))
>> > > > + return 0;
>> > > >
>> > >
>> > > This is just a assumption based on current implementation. What
>> > > happens if this is needed. Infact, it is used in the current
>> > > implementation to create a dummy clock provider, so for sure with
>> > this
>> > > change that will break IMO.
>> >
>> > I agree with Sudeep on this: if you want to exclude some SCMI device
>> > from the fw_devlink handling to address the issues with multiple SCMI
>> > devices created on the same protocol nodes, cant we just flag this
>> > requirement here and avoid to call device_link_add in
>> > driver:scmi_set_handle(), instead of killing completely any possibility of
>> > referencing phandles (and having device_link_add failing as a
>> > consequence of having a NULL supplier)
>> >
>> > i.e. something like:
>> >
>> > @bus.c
>> > ------
>> > static int
>> > __scmi_device_set_node(struct scmi_device *scmi_dev, struct
>> > device_node *np,
>> > int protocol, const char *name) {
>> > if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name,
>> > "cpufreq"))
>> > scmi_dev->avoid_devlink = true;
>> >
>> > device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
>> > ....
>> >
>> >
>> > and @driver.c
>> > -------------
>> >
>> > static void scmi_set_handle(struct scmi_device *scmi_dev) {
>> > scmi_dev->handle = scmi_handle_get(&scmi_dev->dev);
>> > if (scmi_dev->handle && !scmi_dev->avoid_devlink)
>> > scmi_device_link_add(&scmi_dev->dev, scmi_dev-
>> > >handle->dev); }
>> >
>> > .... so that you can avoid fw_devlink BUT keep the device_node NON-
>> > null for the device.
>> >
>> > This would mean also restoring the pre-existing explicit blacklisting in
>> > pinctrl-imx to avoid issues when pinctrl subsystem searches by
>> > device_node...
>> >
>> > ..or I am missing something ?
>>
>> link_ret = device_links_check_suppliers(dev); to check fw_devlink
>> is before "ret = driver_sysfs_add(dev);" which
>> issue bus notify.
>>
>> The link is fw_devlink, the devlink is created in 'device_add'
>> if (dev->fwnode && !dev->fwnode->dev) {
>> dev->fwnode->dev = dev;
>> fw_devlink_link_device(dev);
>> }
>> The check condition is fwnode.
>>
>> I think scmi_dev->avoid_devlink not help here.
>>
>
>Ah right...my bad, the issue comes from the device_links created by
>fw_devlink indirectly while walking the phandles backrefs...still...
>...cant we keep the device_node reference while keep on dropping the
>fw_node as you did:
>
> if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
> scmi_dev->dev.of_node = np;
> return 0;
> }
>
> device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
> ....
>
>...so that the fw_devlink machinery is disabled but still we create a
>device with an underlying related device_node that can be referred in a
>phandle.
ok, I will add "scmi_dev->dev.of_node = np" for cpufreq device.
>
>I wonder also if it was not even more clean to DO initialize fw_devlink
>instead, BUT add some of the existent fw_devlink/devlink flags to inhibit
>all the checks...but I am not familiar with fw_devlink so much and I
>have not experimented in these regards...so I may have just said
>something unfeasible.
fw_devlink is based on device tree node, so there is no way, unless
add subnodes for a protocol node, but this is not welcomed.
Thanks,
Peng
>
>Thanks,
>Cristian
>
Powered by blists - more mailing lists