linux-kernel - Re: [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio

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: <20250820010123.64603-1-junnan01.wu@samsung.com>
Date: Wed, 20 Aug 2025 09:01:23 +0800
From: Junnan Wu <junnan01.wu@...sung.com>
To: cristian.marussi@....com
Cc: arm-scmi@...r.kernel.org, florian.fainelli@...adcom.com,
	junnan01.wu@...sung.com, lei19.wang@...sung.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	q1.huang@...sung.com, sudeep.holla@....com
Subject: Re: [PATCH] firmware: arm_scmi: Set device ready before register
 scmi_virtio_driver

On Tue, 19 Aug 2025 13:33:10 +0100, Cristian Marussi wrote:
> On Tue, Aug 12, 2025 at 03:53:43PM +0800, Junnan Wu wrote:
> > Fix irregularities in scmi driver probe.
> > 
> 
> Good catch, thanks for this: indeed after the transports have been
> converted to modules the probe sequence changed a bit and we cannot
> leverage anymore the virtio_device_ready() called by the core in
> virtio_dev_probe() since the scmi_vio_probe() does not complete until
> the core SCMI stack has probed with scmi_probe() which in turn transmits
> the first BASE protocol exchanges.
> 
> I did not know about this behaviour of type_1 hyp..

It's our local hyp, which intercepts notification until driver is ready.
That helps us to find this irregularity. I think you can ignore this.

> 
> > After 'commit 20bda12a0ea0 ("firmware: arm_scmi: Make VirtIO transport a standalone driver")'
> > making SCMI VirtIO transport a standalone driver,
> > `scmi_virtio_probe` will follow `scmi_vio_probe`,
> > and during `scmi_virtio_probe`, it will do `scmi_probe` which will invoke
> > function `scmi_protocol_acquire(handle, SCMI_PROTOCOL_BASE)`,
> > and this function will send message via virtqueue and wait device's reply
> > 
> > Before this patch, only after `scmi_vio_probe`,
> > frontend will mark device DRIVER_OK by function `virtio_dev_probe`
> > in 'drivers/virtio/virtio.c' itself.
> > 
> > Accroding to chapter '3.1 Device Initialization' in virtio-spec,
> > ```
> > The driver MUST NOT send any buffer available notifications
> > to the device before setting DRIVER_OK.
> > ```
> > 
> > In some type1-hypervisor, available buffer notifications
> > will be intercepted until virtio driver is okay. In this case,
> > `scmi_vio_probe` will be blocked at `scmi_wait_for_reply`
> > and probe can never complete.
> > 
> > Therefore, the operation to set DRIVER_OK must follow
> > perform device-specific setup immediately,
> > then available buffer notifications can be sent in function `scmi_probe`.
> > 
> > Fixes: 20bda12a0ea0 ("firmware: arm_scmi: Make VirtIO transport a standalone driver")
> > Signed-off-by: Junnan Wu <junnan01.wu@...sung.com>
> > ---
> >  drivers/firmware/arm_scmi/transports/virtio.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/firmware/arm_scmi/transports/virtio.c b/drivers/firmware/arm_scmi/transports/virtio.c
> > index cb934db9b2b4..326c4a93e44b 100644
> > --- a/drivers/firmware/arm_scmi/transports/virtio.c
> > +++ b/drivers/firmware/arm_scmi/transports/virtio.c
> > @@ -871,6 +871,9 @@ static int scmi_vio_probe(struct virtio_device *vdev)
> >  	/* Ensure initialized scmi_vdev is visible */
> >  	smp_store_mb(scmi_vdev, vdev);
> >  
> > +	/* Set device ready */
> > +	virtio_device_ready(vdev);
> > +
> >  	ret = platform_driver_register(&scmi_virtio_driver);
> >  	if (ret) {
> >  		vdev->priv = NULL;
> 
> ...also, I suppose, that here nothing is to be done on the error path
> regarding the vdedv status, becasue is already taken care in the error
> path of virtio_dev_probe()...am I right ? (just confirming..)
> 

Yes you are right, I also checked it, error is handled by `virtio_dev_probe`

> Maybe you could simplify a bit the commit message but other than this
> 
> LGTM.
> 
> Reviewed-by: Cristian Marussi <cristian.marussi@....com>
> Tested-by: Cristian Marussi <cristian.marussi@....com>
> 
> Thanks,
> Cristian

Thanks for your kindly review!
BRs
Junnan Wu