On Tue, Mar 10, 2026 at 5:07 AM Joshua Daley <[email protected]> wrote: > > On 3/8/2026 11:06 PM, Stefan Hajnoczi wrote: > > On Thu, Feb 26, 2026 at 09:43:45PM +0100, Joshua Daley wrote: > >> The last step of virtscsi_handle_event is to call virtscsi_kick_event, > >> which calls INIT_WORK on it's own work item. INIT_WORK resets the > >> work item's data bits to 0. > >> > >> If this occurs while the work item is being flushed by > >> cancel_work_sync, then kernel/workqueue.c/work_offqd_enable triggers a > >> kernel warning, as it expects the "disable" bit to be 1: > >> > >> [ 21.450115] workqueue: work disable count underflowed > >> [ 21.450117] WARNING: CPU: 1 PID: 56 at kernel/workqueue.c:4328 > >> enable_work+0x10a/0x120 > >> ... > >> [ 21.450171] Call Trace: > >> [ 21.450173] [<000003db2e5bdc3e>] enable_work+0x10e/0x120 > >> [ 21.450176] ([<000003db2e5bdc3a>] enable_work+0x10a/0x120) > >> [ 21.450178] [<000003db2e5bdd86>] cancel_work_sync+0x86/0xa0 > >> [ 21.450181] [<000003daae97d9e4>] virtscsi_remove+0xb4/0xd0 > >> [virtio_scsi] > >> [ 21.450184] [<000003db2ef3b5ca>] virtio_dev_remove+0x6a/0xd0 > >> [ 21.450186] [<000003db2ef9106c>] > >> device_release_driver_internal+0x1ac/0x260 > >> [ 21.450190] [<000003db2ef8edc8>] bus_remove_device+0xf8/0x190 > >> [ 21.450192] [<000003db2ef88d72>] device_del+0x142/0x340 > >> [ 21.450194] [<000003db2ef88fa0>] device_unregister+0x30/0xa0 > >> [ 21.450196] [<000003db2ef3b2fa>] unregister_virtio_device+0x2a/0x40 > >> > >> This warning may occur if a controller is detached immediately > >> following a disk detach. > >> > >> Move the INIT_WORK call to prevent this. Don't re-init event list > >> work items in virtscsi_kick_event, init them only once in > >> virtscsi_init instead. > >> > >> Signed-off-by: Joshua Daley <[email protected]> > >> --- > >> drivers/scsi/virtio_scsi.c | 6 +++++- > >> 1 file changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c > >> index 0ed8558dad72..173092931df6 100644 > >> --- a/drivers/scsi/virtio_scsi.c > >> +++ b/drivers/scsi/virtio_scsi.c > >> @@ -242,7 +242,6 @@ static int virtscsi_kick_event(struct virtio_scsi > >> *vscsi, > >> struct scatterlist sg; > >> unsigned long flags; > >> > >> - INIT_WORK(&event_node->work, virtscsi_handle_event); > >> sg_init_one(&sg, event_node->event, sizeof(struct virtio_scsi_event)); > >> > >> spin_lock_irqsave(&vscsi->event_vq.vq_lock, flags); > >> @@ -898,6 +897,11 @@ static int virtscsi_init(struct virtio_device *vdev, > >> virtscsi_config_set(vdev, cdb_size, VIRTIO_SCSI_CDB_SIZE); > >> virtscsi_config_set(vdev, sense_size, VIRTIO_SCSI_SENSE_SIZE); > >> > >> + if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { > >> + for (i = 0; i < VIRTIO_SCSI_EVENT_LEN; i++) > >> + INIT_WORK(&vscsi->event_list[i].work, > >> virtscsi_handle_event); > >> + } > > > > The eventq should be populated unconditionally so that non-hotplug > > events are processed even when F_HOTPLUG is not negotiated. For example, > > LUN capacity changes are reported via the VIRTIO_SCSI_T_PARAM_CHANGE > > event. LUN capacity changes depend on F_CHANGE, not F_HOTPLUG. > > > > There is a related bug here: the other if (virtio_has_feature(vdev, > > VIRTIO_SCSI_F_HOTPLUG)) conditionals in this file need to be revisited > > so that LUN capacity changes are reported even when F_HOTPLUG is not > > negotiated. You can test this bug with QEMU's -device > > virtio-scsi-pci,hotplug=off parameter and the 'block_resize' QEMU > > monitor command. > > > > Do you want to write a patch or do you want me to send a follow-up? > > > > Thanks, > > Stefan > > I can write a patch. Thanks for your review. > > There are 3 other if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) > conditionals in this file, for: > > 1. virtscsi_kick_event_all() called in virtscsi_probe() > 2. virtscsi_cancel_event_work() called in virtscsi_remove() > 3. virtscsi_kick_event_all() called in virtscsi_restore() > > Should the eventq be populated truly unconditionally? Then I would just > remove the conditions from > these calls. Or would it be better to just change the conditions to also > check the other feature: > if (... || virtio_has_feature(vdev, VIRTIO_SCSI_F_CHANGE))
Yes. The eventq is always present and the memory backing it is already allocated. I think dropping the conditionals simplifies the driver and avoids bugs like the F_CHANGE one without introducing downsides. Stefan
