Re: [PATCH] scsi: core: Improve type safety of scsi_rescan_device()

[John Garry <john.g.garry@oracle.com>]

On 21/08/2023 21:40, Bart Van Assche wrote:
> Most callers of scsi_rescan_device() have the scsi_device pointer
> available. 

Indirect response to Damien, I think that you mean "... have the 
scsi_device pointer readily available."

> Pass a struct scsi_device pointer to scsi_rescan_device()
> instead of a struct device pointer. This change prevents that a
> pointer to another struct device pointer would be passed accidentally to
> scsi_rescan_device().
> 

Looking back through history, I assume that scsi_rescan_device() was 
originally written to accept a device * because the only caller only had 
a device * available; and the function only required the device *. Then 
sdev->handler and vpd support was then added there, which was when the 
sdev pointer was required.

So ok, I suppose:

Reviewed-by: John Garry <john.g.garry@oracle.com>

> Remove the scsi_rescan_device() declaration from the scsi_priv.h header
> file since it duplicates the declaration in <scsi/scsi_host.h>.
> 
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: John Garry <john.g.garry@oracle.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ata/libata-scsi.c             | 2 +-
>   drivers/scsi/aacraid/commsup.c        | 2 +-
>   drivers/scsi/mvumi.c                  | 2 +-
>   drivers/scsi/scsi_lib.c               | 2 +-
>   drivers/scsi/scsi_priv.h              | 1 -
>   drivers/scsi/scsi_scan.c              | 4 ++--
>   drivers/scsi/scsi_sysfs.c             | 4 ++--
>   drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
>   drivers/scsi/storvsc_drv.c            | 2 +-
>   drivers/scsi/virtio_scsi.c            | 2 +-
>   include/scsi/scsi_host.h              | 2 +-
>   11 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 370d18aca71e..f5c36c8c243a 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4884,7 +4884,7 @@ void ata_scsi_dev_rescan(struct work_struct *work)
>   			}
>   
>   			spin_unlock_irqrestore(ap->lock, flags);
> -			scsi_rescan_device(&(sdev->sdev_gendev));
> +			scsi_rescan_device(sdev);
>   			scsi_device_put(sdev);
>   			spin_lock_irqsave(ap->lock, flags);
>   		}
> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index 3f062e4013ab..013a9a334972 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -1451,7 +1451,7 @@ static void aac_handle_aif(struct aac_dev * dev, struct fib * fibptr)
>   #endif
>   				break;
>   			}
> -			scsi_rescan_device(&device->sdev_gendev);
> +			scsi_rescan_device(device);


nit: how about change this code to have the variable named as sdev, not 
device? That name would be more consistent. That would not be a small 
change, so maybe not worth it.

>   			break;
>   
>   		default:
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 73aa7059b556..6cfbac518085 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c