* 回复: [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio_driver
2025-08-12 7:53 ` [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio_driver Junnan Wu
@ 2025-08-12 8:06 ` yizhijiao2025
2025-08-19 12:33 ` Cristian Marussi
2025-08-26 10:39 ` Sudeep Holla
2 siblings, 0 replies; 5+ messages in thread
From: yizhijiao2025 @ 2025-08-12 8:06 UTC (permalink / raw)
To: 'Junnan Wu', sudeep.holla, cristian.marussi
Cc: florian.fainelli, arm-scmi, linux-arm-kernel, linux-kernel,
lei19.wang, q1.huang
-----邮件原件-----
发件人: arm-scmi+bounces-2017-yizhijiao2025=163.com@vger.kernel.org <arm-scmi+bounces-2017-yizhijiao2025=163.com@vger.kernel.org> 代表 Junnan Wu
发送时间: 2025年8月12日 15:54
收件人: sudeep.holla@arm.com; cristian.marussi@arm.com
抄送: florian.fainelli@broadcom.com; arm-scmi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; lei19.wang@samsung.com; q1.huang@samsung.com; Junnan Wu <junnan01.wu@samsung.com>
主题: [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio_driver
Fix irregularities in scmi driver probe.
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;
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio_driver
2025-08-12 7:53 ` [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio_driver Junnan Wu
2025-08-12 8:06 ` 回复: " yizhijiao2025
@ 2025-08-19 12:33 ` Cristian Marussi
2025-08-20 1:01 ` Junnan Wu
2025-08-26 10:39 ` Sudeep Holla
2 siblings, 1 reply; 5+ messages in thread
From: Cristian Marussi @ 2025-08-19 12:33 UTC (permalink / raw)
To: Junnan Wu
Cc: sudeep.holla, cristian.marussi, florian.fainelli, arm-scmi,
linux-arm-kernel, linux-kernel, lei19.wang, q1.huang
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio_driver
2025-08-19 12:33 ` Cristian Marussi
@ 2025-08-20 1:01 ` Junnan Wu
0 siblings, 0 replies; 5+ messages in thread
From: Junnan Wu @ 2025-08-20 1:01 UTC (permalink / raw)
To: cristian.marussi
Cc: arm-scmi, florian.fainelli, junnan01.wu, lei19.wang,
linux-arm-kernel, linux-kernel, q1.huang, sudeep.holla
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@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..)
>
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@arm.com>
> Tested-by: Cristian Marussi <cristian.marussi@arm.com>
>
> Thanks,
> Cristian
Thanks for your kindly review!
BRs
Junnan Wu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio_driver
2025-08-12 7:53 ` [PATCH] firmware: arm_scmi: Set device ready before register scmi_virtio_driver Junnan Wu
2025-08-12 8:06 ` 回复: " yizhijiao2025
2025-08-19 12:33 ` Cristian Marussi
@ 2025-08-26 10:39 ` Sudeep Holla
2 siblings, 0 replies; 5+ messages in thread
From: Sudeep Holla @ 2025-08-26 10:39 UTC (permalink / raw)
To: cristian.marussi, Junnan Wu
Cc: Sudeep Holla, florian.fainelli, arm-scmi, linux-arm-kernel,
linux-kernel, lei19.wang, q1.huang
On Tue, 12 Aug 2025 15:53:43 +0800, Junnan Wu wrote:
> Fix irregularities in scmi driver probe.
>
> 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
>
> [...]
Applied to sudeep.holla/linux (for-next/scmi/updates), thanks!
[1/1] firmware: arm_scmi: Set device ready before register scmi_virtio_driver
https://git.kernel.org/sudeep.holla/c/e8faa8a466f6
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 5+ messages in thread