linux-kernel - Re: [PATCH] firmware: arm_scmi: Give SMC transport precedence over mailbox

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] [day] [month] [year] [list]

Message-ID: <ZwZ5EinIhUsXT83d@bogus>
Date: Wed, 9 Oct 2024 13:37:38 +0100
From: Sudeep Holla <sudeep.holla@....com>
To: Florian Fainelli <florian.fainelli@...adcom.com>
Cc: Cristian Marussi <cristian.marussi@....com>,
	linux-arm-kernel@...ts.infread.org, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Sudeep Holla <sudeep.holla@....com>,
	Conor Dooley <conor+dt@...nel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE" <arm-scmi@...r.kernel.org>,
	"moderated list:SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE" <linux-arm-kernel@...ts.infradead.org>,
	justin.chen@...adcom.com, opendmb@...il.com,
	kapil.hali@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
	Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] firmware: arm_scmi: Give SMC transport precedence over
 mailbox

On Tue, Oct 08, 2024 at 10:49:01AM -0700, Florian Fainelli wrote:
> On 10/8/24 06:06, Sudeep Holla wrote:

[...]

> > IIUC, you need to support old kernel with SMC mailbox driver and new SMC
> > transport within the SCMI. Is that right understanding ?
> 
> That is correct.
> 

Thanks.

> > IIUC, the DTB has mailbox nodes that are available but fail only in the setup
> > stage ? Or is it marked unavailable and we are missing some checks either
> > in SCMI or mailbox ?
> 
> We fail at scmi_chan_setup -> idr_find() returning -EINVAL.

IIRC, the original intention of code under if(!info->desc->ops->chan_available()
in scmi_chan_setup(), is to just handle Rx case and valid Tx missing case for
non BASE protocols. I wonder if we can add additional check at the start of
this if block:
	if (tx && prot_id == SCMI_PROTOCOL_BASE)
		return -ENODEV or -ENOENT;

just to better handle your scenario. But IIUC it may not fix your issue as
we still return success from the platform_device probing with the new
restructuring. It is only the probe of the device we create from this one
can be made to fail. I think I know understand the issue much better than
before.

> I did check that
> returning -ENODEV, which arguably might be a somewhat more accurate return
> code (-ENOENT being one, too) does not help us here. Cristian suggested
> device_release_driver() which sounded like a good idea, but will deadlock.
>

Cristian mentioned in private he will explore other options just in case
we need solid alternative to address your issue.

> The reason why we fail there is because mailbox_chan_available() returns
> false. With fw_devlink=on Linux will parse the Device Tree, find the
> 'mboxes' property pointing to the brcm_scmi_mailbox Device Tree node and
> puts it on a list of providers that it is waiting for.
> 
> Because we are using the ARM SMC transport however, the brcm_scmi_mailbox
> node is never backed by any driver in Linux and this causes the system to
> fail booting since we do not have any SCMI provider. At the time, because we
> were under pressure to get a GKI kernel we decided to "break" our older
> downstream kernels by doing this property rename and put in a patch in those
> kernel to treat "brcm,mboxes" the same as "mboxes" where relevant, which was
> mostly in SCMI.
> 
> Now, assuming that we revert that DT property rename, that still does not
> really solve anything anyway, the channel is not available regardless of how
> we shake it.
>

Understood. Thanks for detailed explanation and time.

> > 
> > IOW, have you already explored that this -EINVAL is correct return value
> > here and can't be changed to -ENODEV ? I might be not following the failure
> > path correctly here, but I assume it is
> > 	scmi_chan_setup()
> > 	info->desc->ops->chan_setup()
> > 	mailbox_chan_setup()
> > 	mbox_request_channel()

[...]

> > OK this sounds like you have already explored returning -ENODEV is not
> > an option ? It is fair enough, but just want to understand correctly.
> > I still think I am missing something.
> 
> Yes, that was my first start.
>

Now I know why that won't work or its not so trivial as we have some kind
of redirection to address various transport dependencies and the mechanism we
have implemented to avoid probe deferral with additional platform devices
and associated probing make it difficult to propagate the error of DD model.
We need that to handle the dependency better than relying on probe deferral
which comes with its problems(like initcall level adjustments when using
some transports like OPTEE/FFA/virtio/...). Your issue is not an issue
in normal case 😄. Anyways, I will queue this as it is not affecting anyone
else. Hopefully no one has any objections.

-- 
Regards,
Sudeep