linux-kernel - RE: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode for scmi cpufreq

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:
 <PAXPR04MB8459B2CF515DC89DE98A1B7C88862@PAXPR04MB8459.eurprd04.prod.outlook.com>
Date: Tue, 13 Aug 2024 13:52:30 +0000
From: Peng Fan <peng.fan@....com>
To: Cristian Marussi <cristian.marussi@....com>
CC: Dhruva Gole <d-gole@...com>, "Peng Fan (OSS)" <peng.fan@....nxp.com>,
	"saravanak@...gle.com" <saravanak@...gle.com>, "sudeep.holla@....com"
	<sudeep.holla@....com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "arm-scmi@...r.kernel.org"
	<arm-scmi@...r.kernel.org>
Subject: RE: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode for scmi
 cpufreq

> Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode
> for scmi cpufreq
> 
> On Tue, Aug 13, 2024 at 10:25:31AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set
> fwnode
> > > for scmi cpufreq
> > >
> > > On Jul 29, 2024 at 15:03:25 +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
> > >
> > > The commit message itself seems very specific to some platform to
> me.
> > > What about platforms that don't atall have a GPU? Why would
> they
> > > care about this?
> >
> > It is a generic issue if a platform has performance domain to serve
> > scmi cpufreq and device performance level.
> >
> > >
> > > > device not ready.
> > > >
> > > > Because for cpufreq, no need use fw_devlink. So bypass setting
> > > fwnode
> > > > for scmi cpufreq device.
> > > >
> > > > Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the
> > > > scmi_device")
> > > > Signed-off-by: Peng Fan <peng.fan@....com>
> > > > ---
> > > >
> > > > V2:
> > > >  Use A!=B to replace !(A == B)
> > > >  Add fixes tag
> > > >  This might be a workaround, but since this is a fix, it is simple
> > > > for backporting.
> > >
> > > More than a workaround, it feels like a HACK to me.
> > >
> > > >
> > > > V1:
> > > >
> > > >
> > > >
> > > >  drivers/firmware/arm_scmi/bus.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > > b/drivers/firmware/arm_scmi/bus.c index
> > > 96b2e5f9a8ef..be91a82e0cda
> > > > 100644
> > > > --- a/drivers/firmware/arm_scmi/bus.c
> > > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > > @@ -395,7 +395,8 @@ __scmi_device_create(struct
> device_node
> > > *np, struct device *parent,
> > > >  	scmi_dev->id = id;
> > > >  	scmi_dev->protocol_id = protocol;
> > > >  	scmi_dev->dev.parent = parent;
> > > > -	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
> > > > +	if ((protocol != SCMI_PROTOCOL_PERF) || strcmp(name,
> > > "cpufreq"))
> > > > +		device_set_node(&scmi_dev->dev,
> > > of_fwnode_handle(np));
> > >
> > > I kind of disagree with the idea here to be specific about the
> > > PROTOCOL_PERF or cpufreq. This is a generic arm scmi bus driver
> right?
> > > Why bring in specific code into a bus driver? We will never fix the
> > > actual root cause of the issue this way.
> >
> > The root cause is fwnode devlink only supports one fwnode, one
> device.
> > 1:1 match. But current arm scmi driver use one fwnode for two
> devices.
> >
> > If your platform has scmi cpufreq and scmi device performance
> domain,
> > you might see that some devices are consumer of scmi cpufreq, but
> > actually they should be consumer of scmi device performance
> domain.
> >
> > I not have a good idea that this is fw devlink design that only allows
> > 1 fwnode has 1 device or not. If yes, that arm scmi should be fixed.
> > If not, fw devlink should be updated.
> >
> > The current patch is the simplest method for stable tree fixes as I
> > could work out.
> 
> So this is the same root cause at the end of the issues you had with
> IMX pinctrl coexistence...i.e. the SCMI stack creates scmi devices that
> embeds the protocol node, BUT since you can have multiple
> device/drivers doing different things on different resources within the
> same protocol you can end with 2 devices having the same embedded
> device_node, since we dont really have anything else to use as
> device_node, I rigth ?

I think, yes. And you remind me that with PINCTRL_SCMI and
CONFIG_PINCTRL_IMX_SCMI both enabled, the scmi pinctrl node
will only take one to set the fwnode device pointer depends on
the order to run __scmi_device_create.

So not only perf, pinctrl also has issue here, fwnode devlink will
not work properly for pinctrl/perf.

Regards,
Peng.

> 
> Thanks,
> Cristian