Re: [PATCH v7 11/12] nvdimm: virtio_pmem: converge broken virtqueue to -EIO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The use of `dev_err_once()` in `virtio_pmem_mark_broken()` causes a false positive error during normal device removal and permanently masks actual virtqueue failures for all subsequent devices.
--

commit eef59350d160a3f85b9b2a847d436de7706e56bd
Author: Li Chen <me@linux.beauty>

nvdimm: virtio_pmem: converge broken virtqueue to -EIO

This commit tracks a device-level broken state for the virtio pmem
request virtqueue to fail fast and wake up waiters when the queue is
broken. It unifies the handling of broken queues and synchronously fails
parent bios.

Link: https://lore.kernel.org/r/202512250116.ewtzlD0g-lkp@intel.com/

> diff --git a/drivers/nvdimm/nd_virtio.c b/drivers/nvdimm/nd_virtio.c
> --- a/drivers/nvdimm/nd_virtio.c
> +++ b/drivers/nvdimm/nd_virtio.c
[ ... ]
> +void virtio_pmem_mark_broken(struct virtio_pmem *vpmem)
> +{
> +	if (!READ_ONCE(vpmem->broken)) {
> +		WRITE_ONCE(vpmem->broken, true);
> +		dev_err_once(&vpmem->vdev->dev, "virtqueue is broken\n");

[Severity: Medium]
Will this dev_err_once() globally mask true virtqueue failures and produce
false positive errors during normal operation?

Because dev_err_once() uses a globally static boolean, it will only print
this message once for the entire driver, rather than once per device.

> +	}
> +
> +	virtio_pmem_wake_inflight(vpmem);
> +	virtio_pmem_wake_all_waiters(vpmem);
> +}
> +EXPORT_SYMBOL_GPL(virtio_pmem_mark_broken);
[ ... ]
> diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
> --- a/drivers/nvdimm/virtio_pmem.c
> +++ b/drivers/nvdimm/virtio_pmem.c
[ ... ]
> @@ -148,11 +150,21 @@ static void virtio_pmem_remove(struct virtio_device *vdev)
>  {
>  	struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
>  	struct virtio_pmem *vpmem = vdev->priv;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vpmem->pmem_lock, flags);
> +	virtio_pmem_mark_broken(vpmem);

[Severity: Medium]
Does this unconditional call during teardown mean the first normal device
removal will trigger the one-time error log?

If a user unbinds the driver or removes a device, virtio_pmem_remove()
will call virtio_pmem_mark_broken(). This appears to log a false positive
error during expected teardown, and because it was logged via
dev_err_once(), it will silently mask any actual hardware or virtqueue
failures on other virtio-pmem devices afterwards.

> +	spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
>  
> -	nvdimm_bus_unregister(nvdimm_bus);
>  	drain_workqueue(vpmem->flush_wq);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630092338.2094628-1-me@linux.beauty?part=11