Re: [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio_driver
From: Cristian Marussi <cristian.marussi@arm.com>
Date: 2025-08-19 12:33:21
Also in:
arm-scmi, lkml
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..
quoted hunk ↗ jump to hunk
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 <redacted> --- 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