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 <[email protected]> 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/[email protected]/ > 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/[email protected]?part=11
