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: <ZrtVrG-B5TqmTQUa@pluto>
Date: Tue, 13 Aug 2024 13:47:56 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Dhruva Gole <d-gole@...com>
Cc: "Peng Fan (OSS)" <peng.fan@....nxp.com>, sudeep.holla@....com,
	cristian.marussi@....com, saravanak@...gle.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	arm-scmi@...r.kernel.org, Peng Fan <peng.fan@....com>
Subject: Re: [PATCH V2] firmware: arm_scmi: bus: bypass set fwnode for scmi
 cpufreq

On Tue, Aug 13, 2024 at 02:27:03PM +0530, Dhruva Gole wrote:
> On Jul 29, 2024 at 15:03:25 +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@....com>
> > 

Hi Peng, Dhruva

> > 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?
> 

+1

If you faced a problem on specific platform and a specific config the aim
here should be to solve it generally...not to just hack something around
to fix your specific case while potentially breaking future users
(evenm if now cpufreq does not need it...)

> > 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.
> 

+1 ... exactly... I did not have time to dig into this but I would like
to understand better the implications of this, in order to avoid to
inadvertently fix one issue and create a few more :P

...as an example, for starters, killing devlink straigh away will most
probably kill also the auto-unbinding of the stack when a supplier is removed...

> > 
> > V1:
> >  https://lore.kernel.org/all/20240717093515.327647-1-peng.fan@oss.nxp.com/
> > 
> >  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.
> 

+1 again

Whatevier solution we will adopt ahs to be general...like enabling SCMI
drivers to flag a particular condition/need so the core can take
countermeasures...beside being in the bus code, we cannot really think
of just keep blacklisting here all the combination we dont like...

Thanks,
Cristian