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

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

[Bart Van Assche]

Most callers of scsi_rescan_device() have the scsi_device pointer
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().

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);
 			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
@@ -1500,7 +1500,7 @@ static void mvumi_rescan_devices(struct mvumi_hba *mhba, int id)
 
 	sdev = scsi_device_lookup(mhba->shost, 0, id, 0);
 	if (sdev) {
-		scsi_rescan_device(&sdev->sdev_gendev);
+		scsi_rescan_device(sdev);
 		scsi_device_put(sdev);
 	}
 }
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d84209a96b48..b0169ffaefd2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2445,7 +2445,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 		envp[idx++] = "SDEV_MEDIA_CHANGE=1";
 		break;
 	case SDEV_EVT_INQUIRY_CHANGE_REPORTED:
-		scsi_rescan_device(&sdev->sdev_gendev);
+		scsi_rescan_device(sdev);
 		envp[idx++] = "SDEV_UA=INQUIRY_DATA_HAS_CHANGED";
 		break;
 	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index f42388ecb024..65c993c97909 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -138,7 +138,6 @@ extern int scsi_complete_async_scans(void);
 extern int scsi_scan_host_selected(struct Scsi_Host *, unsigned int,
 				   unsigned int, u64, enum scsi_scan_mode);
 extern void scsi_forget_host(struct Scsi_Host *);
-extern void scsi_rescan_device(struct device *);
 
 /* scsi_sysctl.c */
 #ifdef CONFIG_SYSCTL
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e046dd51d326..712eebceda35 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1622,9 +1622,9 @@ int scsi_add_device(struct Scsi_Host *host, uint channel,
 }
 EXPORT_SYMBOL(scsi_add_device);
 
-void scsi_rescan_device(struct device *dev)
+void scsi_rescan_device(struct scsi_device *sdev)
 {
-	struct scsi_device *sdev = to_scsi_device(dev);
+	struct device *dev = &sdev->sdev_gendev;
 
 	device_lock(dev);
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 60317676e45f..24f6eefb6803 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -747,7 +747,7 @@ static ssize_t
 store_rescan_field (struct device *dev, struct device_attribute *attr,
 		    const char *buf, size_t count)
 {
-	scsi_rescan_device(dev);
+	scsi_rescan_device(to_scsi_device(dev));
 	return count;
 }
 static DEVICE_ATTR(rescan, S_IWUSR, NULL, store_rescan_field);
@@ -840,7 +840,7 @@ store_state_field(struct device *dev, struct device_attribute *attr,
 		 * waiting for pending I/O to finish.
 		 */
 		blk_mq_run_hw_queues(sdev->request_queue, true);
-		scsi_rescan_device(dev);
+		scsi_rescan_device(sdev);
 	}
 
 	return ret == 0 ? count : -EINVAL;
diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index 6aaaa7ebca37..ed694d939964 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -2257,7 +2257,7 @@ static void pqi_update_device_list(struct pqi_ctrl_info *ctrl_info,
 			device->advertised_queue_depth = device->queue_depth;
 			scsi_change_queue_depth(device->sdev, device->advertised_queue_depth);
 			if (device->rescan) {
-				scsi_rescan_device(&device->sdev->sdev_gendev);
+				scsi_rescan_device(device->sdev);
 				device->rescan = false;
 			}
 		}
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 659196a2f63a..6bfd88495edb 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -470,7 +470,7 @@ static void storvsc_device_scan(struct work_struct *work)
 	sdev = scsi_device_lookup(wrk->host, 0, wrk->tgt_id, wrk->lun);
 	if (!sdev)
 		goto done;
-	scsi_rescan_device(&sdev->sdev_gendev);
+	scsi_rescan_device(sdev);
 	scsi_device_put(sdev);
 
 done:
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index bd5633667d01..9d1bdcdc1331 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -325,7 +325,7 @@ static void virtscsi_handle_param_change(struct virtio_scsi *vscsi,
 	/* Handle "Parameters changed", "Mode parameters changed", and
 	   "Capacity data has changed".  */
 	if (asc == 0x2a && (ascq == 0x00 || ascq == 0x01 || ascq == 0x09))
-		scsi_rescan_device(&sdev->sdev_gendev);
+		scsi_rescan_device(sdev);
 
 	scsi_device_put(sdev);
 }
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 70b7475dcf56..b07db65906dd 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -764,7 +764,7 @@ scsi_template_proc_dir(const struct scsi_host_template *sht);
 #define scsi_template_proc_dir(sht) NULL
 #endif
 extern void scsi_scan_host(struct Scsi_Host *);
-extern void scsi_rescan_device(struct device *);
+extern void scsi_rescan_device(struct scsi_device *);
 extern void scsi_remove_host(struct Scsi_Host *);
 extern struct Scsi_Host *scsi_host_get(struct Scsi_Host *);
 extern int scsi_host_busy(struct Scsi_Host *shost);

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

[Damien Le Moal]

On 8/22/23 05:40, Bart Van Assche wrote:
> Most callers of scsi_rescan_device() have the scsi_device pointer

s/Most/All ? If it is most, then you have an issue somewhere :)

> 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

...pointer to another struct device to be passed accidentally to...

> scsi_rescan_device().
> 
> Remove the scsi_rescan_device() declaration from the scsi_priv.h header
> file since it duplicates the declaration in <scsi/scsi_host.h>.

With the above nits fixed, feel free to add:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

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

[Hannes Reinecke]

On 8/21/23 22:40, Bart Van Assche wrote:
> Most callers of scsi_rescan_device() have the scsi_device pointer
> 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().
> 
> 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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[John Garry]

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