[PATCH 0/1] scsi: virtio_scsi: move INIT_WORK calls to virtscsi_init

[PATCH 0/1] scsi: virtio_scsi: move INIT_WORK calls to virtscsi_init

[Joshua Daley]

This patch avoids a kernel warning that may occur if a virtio_scsi
controller is detached immediately following a disk detach. See the
commit message for details. The following are instructions to
produce the warning (without the proposed patch).

Timing matters--if all event work items call INIT_WORK before they are
flushed by cancel_work_sync, then the warning will not occur.

The warning will occur consistently if a sleep is added in
virtscsi_kick_event before the INIT_WORK call, like so:

#include <linux/delay.h>

static int virtscsi_kick_event(struct virtio_scsi *vscsi,
			       struct virtio_scsi_event_node *event_node)
{
    int err;
    struct scatterlist sg;
    unsigned long flags;

 -> msleep(1000);
    INIT_WORK(&event_node->work, virtscsi_handle_event);
	
    ...
}

Then, just detach a disk and its controller in quick succession:

virsh detach-device --domain <domain> disk.xml; \
virsh detach-device --domain <domain> controller.xml

where disk.xml and controller.xml are text files containing the XML
of the disk and controller.

Or, with the libvirt python module:

domain.detachDevice(str(disk_xml))
domain.detachDevice(str(controller_xml))

Joshua Daley (1):
  scsi: virtio_scsi: move INIT_WORK calls to virtscsi_init

 drivers/scsi/virtio_scsi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/1] scsi: virtio_scsi: move INIT_WORK calls to virtscsi_init

[Joshua Daley]

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 <jdaley@linux.ibm.com>
---
 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);
+	}
+
 	err = 0;
 
 out:
-- 
2.34.1

Re: [PATCH 1/1] scsi: virtio_scsi: move INIT_WORK calls to virtscsi_init

[Eric Farman]

On Thu, 2026-02-26 at 21:43 +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 <jdaley@linux.ibm.com>

The fact that the INIT_WORK points to virtscsi_handle_event(), which itself calls
virtscsi_kick_event() and re-inits the workqueue struct today, does seem odd. Moving this to _init,
as part of the _probe() process, seems correct to me. One nit below, but FWIW:

Reviewed-by: Eric Farman <farman@linux.ibm.com>
Tested-by: Eric Farman <farman@linux.ibm.com>

> ---
>  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,

Just before this hunk is a prototype for virtscsi_handle_event(), since it was previously used in
this function but defined afterwards. I suspect it can be removed now?

>  	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);
> +	}
> +
>  	err = 0;
>  
>  out:

Re: [PATCH 1/1] scsi: virtio_scsi: move INIT_WORK calls to virtscsi_init

[Joshua Daley]

On 3/3/2026 4:45 PM, Eric Farman wrote:
> On Thu, 2026-02-26 at 21:43 +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 <jdaley@linux.ibm.com>
> 
> The fact that the INIT_WORK points to virtscsi_handle_event(), which itself calls
> virtscsi_kick_event() and re-inits the workqueue struct today, does seem odd. Moving this to _init,
> as part of the _probe() process, seems correct to me. One nit below, but FWIW:
> 
> Reviewed-by: Eric Farman <farman@linux.ibm.com>
> Tested-by: Eric Farman <farman@linux.ibm.com>
> 
>> ---
>>   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,
> 
> Just before this hunk is a prototype for virtscsi_handle_event(), since it was previously used in
> this function but defined afterwards. I suspect it can be removed now?
> 

Yes, looks like it can be removed safely. That should probably be done in a separate patch.

>>   	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);
>> +	}
>> +
>>   	err = 0;
>>   
>>   out:

Re: [PATCH 1/1] scsi: virtio_scsi: move INIT_WORK calls to virtscsi_init

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3393 bytes --]

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 <jdaley@linux.ibm.com>
> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/1] scsi: virtio_scsi: move INIT_WORK calls to virtscsi_init

[Joshua Daley]

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 <jdaley@linux.ibm.com>
>> ---
>>   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))

Re: [PATCH 1/1] scsi: virtio_scsi: move INIT_WORK calls to virtscsi_init

[Stefan Hajnoczi]

On Tue, Mar 10, 2026 at 5:07 AM Joshua Daley <jdaley@linux.ibm.com> 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 <jdaley@linux.ibm.com>
> >> ---
> >>   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