Re: [PATCH 2/2] scsi: core: put LLD module refcnt after SCSI device is released

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 01:20:28PM +0800, Ming Lei wrote:
> SCSI host release is triggered when SCSI device is released, and we have to
> make sure that LLD module won't be unloaded before SCSI host instance is
> released.
> 
> So put LLD module refcnt after SCSI device is released.
> 
> SCSI device release may be moved into workqueue context if scsi_device_put
> is called in interrupt context, and handle this case by piggybacking
> putting LLD module refcnt into SCSI device release handler.
> 
> Reported-by: Changhui Zhong <czhong@xxxxxxxxxx>
> Reported-by: Yi Zhang <yi.zhang@xxxxxxxxxx>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> ---
>  drivers/scsi/scsi.c        | 14 ++++++++++++--
>  drivers/scsi/scsi_sysfs.c  |  8 ++++++++
>  include/scsi/scsi_device.h |  1 +
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index b241f9e3885c..7cad256ba895 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -553,8 +553,18 @@ EXPORT_SYMBOL(scsi_device_get);
>   */
>  void scsi_device_put(struct scsi_device *sdev)
>  {
> -	module_put(sdev->host->hostt->module);
> -	put_device(&sdev->sdev_gendev);
> +	struct module *mod = sdev->host->hostt->module;
> +	/*
> +	 * sdev->sdev_gendev's real release handler will be scheduled into
> +	 * user context if we are in interrupt context, and we have to put
> +	 * LLD module refcnt after the device is really released.
> +	 */
> +	preempt_disable();
> +	if (put_device(&sdev->sdev_gendev) && in_interrupt())

Why does in_interrupt() matter here?  And is this even set if you have
threaded interrupts?

This feels very wrong as you are doing something different if this is
called depending on the context and you really do not have control over
the context of when this is called at all.

What problem is this solving?  How is a host controller driver being
unloaded before the children it controls are removed?  Who is holding a
reference on them and why is this happening only now?

And who cares about unloading the kernel module in this fashion?

thanks,

greg k-h