| 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: <Z3bHsUMvajaOOhgO@pluto>
Date: Thu, 2 Jan 2025 17:06:57 +0000
From: Cristian Marussi <cristian.marussi@....com>
To: Peng Fan <peng.fan@....com>
Cc: Cristian Marussi <cristian.marussi@....com>,
Sudeep Holla <sudeep.holla@....com>,
"Peng Fan (OSS)" <peng.fan@....nxp.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 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.
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.
Thanks,
Cristian
Powered by blists - more mailing lists