Re: [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio_driver

[Cristian Marussi <cristian.marussi@arm.com>]

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

> 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@samsung.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..)

Maybe you could simplify a bit the commit message but other than this

LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>
Tested-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian