[PATCH v4 0/4] Prepare for constifying SCSI host templates

[PATCH v4 0/4] Prepare for constifying SCSI host templates

[Bart Van Assche]

Hi Martin,

This patch series prepares for constifying SCSI host templates by moving the
members that are not constant out of the SCSI host template. Please consider
this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v3:
- Changed the 'present' counter from 8 to 32 bits.
- Fixed a bug in an error path (reported by John Garry).
- Changed EXPORT_SYMBOL() into EXPORT_SYMBOL_GPL().
- Split patch 1/3 into two patches.

Changes compared to v2:
- Optimized the show_info == NULL case.
- Added a patch that removes the code that clears the module pointer in the host
  template.

Changes compared to v1:
- Fix the CONFIG_SCSI_PROC_FS=n build.

Bart Van Assche (4):
  scsi: esas2r: Initialize two host template members implicitly
  scsi: esas2r: Introduce scsi_template_proc_dir()
  scsi: core: Introduce a new list for SCSI proc directory entries
  scsi: core: Rework the code for dropping the LLD module reference

 drivers/scsi/esas2r/esas2r_main.c |  19 +++--
 drivers/scsi/scsi_priv.h          |   4 +-
 drivers/scsi/scsi_proc.c          | 120 ++++++++++++++++++++++++++----
 drivers/scsi/scsi_sysfs.c         |   7 +-
 include/scsi/scsi_device.h        |   1 +
 include/scsi/scsi_host.h          |  18 ++---
 6 files changed, 127 insertions(+), 42 deletions(-)

[PATCH v4 1/4] scsi: esas2r: Initialize two host template members implicitly

[Bart Van Assche]

Prepare for removing the 'proc_dir' and 'present' members from the SCSI
host template by implicitly initializing 'present' and 'emulated' in
'driver_template'.

Cc: Bradley Grove <linuxdrivers@attotech.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/esas2r/esas2r_main.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/esas2r/esas2r_main.c b/drivers/scsi/esas2r/esas2r_main.c
index 7a4eadad23d7..27f6e7ccded8 100644
--- a/drivers/scsi/esas2r/esas2r_main.c
+++ b/drivers/scsi/esas2r/esas2r_main.c
@@ -248,8 +248,6 @@ static struct scsi_host_template driver_template = {
 	.sg_tablesize			= SG_CHUNK_SIZE,
 	.cmd_per_lun			=
 		ESAS2R_DEFAULT_CMD_PER_LUN,
-	.present			= 0,
-	.emulated			= 0,
 	.proc_name			= ESAS2R_DRVR_NAME,
 	.change_queue_depth		= scsi_change_queue_depth,
 	.max_sectors			= 0xFFFF,

[PATCH v4 2/4] scsi: esas2r: Introduce scsi_template_proc_dir()

[Bart Van Assche]

Prepare for removing the 'proc_dir' and 'present' members from the SCSI
host template. This patch does not change any functionality.

Cc: Bradley Grove <linuxdrivers@attotech.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/esas2r/esas2r_main.c | 17 +++++++++++------
 drivers/scsi/scsi_proc.c          | 11 +++++++++++
 include/scsi/scsi_host.h          |  6 ++++++
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/esas2r/esas2r_main.c b/drivers/scsi/esas2r/esas2r_main.c
index 27f6e7ccded8..d7a2c49ff5ee 100644
--- a/drivers/scsi/esas2r/esas2r_main.c
+++ b/drivers/scsi/esas2r/esas2r_main.c
@@ -635,10 +635,13 @@ static void __exit esas2r_exit(void)
 	esas2r_log(ESAS2R_LOG_INFO, "%s called", __func__);
 
 	if (esas2r_proc_major > 0) {
+		struct proc_dir_entry *proc_dir;
+
 		esas2r_log(ESAS2R_LOG_INFO, "unregister proc");
 
-		remove_proc_entry(ATTONODE_NAME,
-				  esas2r_proc_host->hostt->proc_dir);
+		proc_dir = scsi_template_proc_dir(esas2r_proc_host->hostt);
+		if (proc_dir)
+			remove_proc_entry(ATTONODE_NAME, proc_dir);
 		unregister_chrdev(esas2r_proc_major, ESAS2R_DRVR_NAME);
 
 		esas2r_proc_major = 0;
@@ -728,11 +731,13 @@ const char *esas2r_info(struct Scsi_Host *sh)
 			       esas2r_proc_major);
 
 		if (esas2r_proc_major > 0) {
-			struct proc_dir_entry *pde;
+			struct proc_dir_entry *proc_dir;
+			struct proc_dir_entry *pde = NULL;
 
-			pde = proc_create(ATTONODE_NAME, 0,
-					  sh->hostt->proc_dir,
-					  &esas2r_proc_ops);
+			proc_dir = scsi_template_proc_dir(sh->hostt);
+			if (proc_dir)
+				pde = proc_create(ATTONODE_NAME, 0, proc_dir,
+						  &esas2r_proc_ops);
 
 			if (!pde) {
 				esas2r_log_dev(ESAS2R_LOG_WARN,
diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 95aee1ad1383..456b43097288 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -83,6 +83,17 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
 				4 * PAGE_SIZE);
 }
 
+/**
+ * scsi_template_proc_dir() - returns the procfs dir for a SCSI host template
+ * @sht: SCSI host template pointer.
+ */
+struct proc_dir_entry *
+scsi_template_proc_dir(const struct scsi_host_template *sht)
+{
+	return sht->proc_dir;
+}
+EXPORT_SYMBOL_GPL(scsi_template_proc_dir);
+
 static const struct proc_ops proc_scsi_ops = {
 	.proc_open	= proc_scsi_host_open,
 	.proc_release	= single_release,
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 9b0a028bf053..030faca947d2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -751,6 +751,12 @@ extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
 extern int __must_check scsi_add_host_with_dma(struct Scsi_Host *,
 					       struct device *,
 					       struct device *);
+#if defined(CONFIG_SCSI_PROC_FS)
+struct proc_dir_entry *
+scsi_template_proc_dir(const struct scsi_host_template *sht);
+#else
+#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_remove_host(struct Scsi_Host *);

[PATCH v4 3/4] scsi: core: Introduce a new list for SCSI proc directory entries

[Bart Van Assche]

Instead of using scsi_host_template members to track the SCSI proc
directory entries, track these entries in a list. This patch changes the
time needed for looking up the proc dir pointer from O(1) into O(n). I
think this is acceptable since the number of SCSI host adapter types per
host is usually small (less than ten).

This patch has been tested by attaching two USB storage devices to a
qemu host:

$ grep -aH . /proc/scsi/usb-storage/*
/proc/scsi/usb-storage/7:   Host scsi7: usb-storage
/proc/scsi/usb-storage/7:       Vendor: QEMU
/proc/scsi/usb-storage/7:      Product: QEMU USB HARDDRIVE
/proc/scsi/usb-storage/7:Serial Number: 1-0000:00:02.1:00.0-6
/proc/scsi/usb-storage/7:     Protocol: Transparent SCSI
/proc/scsi/usb-storage/7:    Transport: Bulk
/proc/scsi/usb-storage/7:       Quirks: SANE_SENSE
/proc/scsi/usb-storage/8:   Host scsi8: usb-storage
/proc/scsi/usb-storage/8:       Vendor: QEMU
/proc/scsi/usb-storage/8:      Product: QEMU USB HARDDRIVE
/proc/scsi/usb-storage/8:Serial Number: 1-0000:00:02.1:00.0-7
/proc/scsi/usb-storage/8:     Protocol: Transparent SCSI
/proc/scsi/usb-storage/8:    Transport: Bulk
/proc/scsi/usb-storage/8:       Quirks: SANE_SENSE

This patch prepares for constifying most SCSI host templates.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_priv.h |   4 +-
 drivers/scsi/scsi_proc.c | 111 +++++++++++++++++++++++++++++++++------
 include/scsi/scsi_host.h |  12 -----
 3 files changed, 96 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index f385b3f04d6e..be5d7c9b7f39 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -100,8 +100,8 @@ extern void scsi_evt_thread(struct work_struct *work);
 
 /* scsi_proc.c */
 #ifdef CONFIG_SCSI_PROC_FS
-extern void scsi_proc_hostdir_add(struct scsi_host_template *);
-extern void scsi_proc_hostdir_rm(struct scsi_host_template *);
+extern void scsi_proc_hostdir_add(const struct scsi_host_template *);
+extern void scsi_proc_hostdir_rm(const struct scsi_host_template *);
 extern void scsi_proc_host_add(struct Scsi_Host *);
 extern void scsi_proc_host_rm(struct Scsi_Host *);
 extern int scsi_init_procfs(void);
diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 456b43097288..7dffa73d6f92 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -43,8 +43,23 @@
 
 static struct proc_dir_entry *proc_scsi;
 
-/* Protect sht->present and sht->proc_dir */
+/* Protects scsi_proc_list */
 static DEFINE_MUTEX(global_host_template_mutex);
+static LIST_HEAD(scsi_proc_list);
+
+/**
+ * struct scsi_proc_entry - (host template, SCSI proc dir) association
+ * @entry: entry in scsi_proc_list.
+ * @sht: SCSI host template associated with the procfs directory.
+ * @proc_dir: procfs directory associated with the SCSI host template.
+ * @present: Number of SCSI hosts instantiated for @sht.
+ */
+struct scsi_proc_entry {
+	struct list_head	entry;
+	const struct scsi_host_template *sht;
+	struct proc_dir_entry	*proc_dir;
+	unsigned int		present;
+};
 
 static ssize_t proc_scsi_host_write(struct file *file, const char __user *buf,
                            size_t count, loff_t *ppos)
@@ -83,6 +98,32 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
 				4 * PAGE_SIZE);
 }
 
+static struct scsi_proc_entry *
+__scsi_lookup_proc_entry(const struct scsi_host_template *sht)
+{
+	struct scsi_proc_entry *e;
+
+	lockdep_assert_held(&global_host_template_mutex);
+
+	list_for_each_entry(e, &scsi_proc_list, entry)
+		if (e->sht == sht)
+			return e;
+
+	return NULL;
+}
+
+static struct scsi_proc_entry *
+scsi_lookup_proc_entry(const struct scsi_host_template *sht)
+{
+	struct scsi_proc_entry *e;
+
+	mutex_lock(&global_host_template_mutex);
+	e = __scsi_lookup_proc_entry(sht);
+	mutex_unlock(&global_host_template_mutex);
+
+	return e;
+}
+
 /**
  * scsi_template_proc_dir() - returns the procfs dir for a SCSI host template
  * @sht: SCSI host template pointer.
@@ -90,7 +131,9 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
 struct proc_dir_entry *
 scsi_template_proc_dir(const struct scsi_host_template *sht)
 {
-	return sht->proc_dir;
+	struct scsi_proc_entry *e = scsi_lookup_proc_entry(sht);
+
+	return e ? e->proc_dir : NULL;
 }
 EXPORT_SYMBOL_GPL(scsi_template_proc_dir);
 
@@ -109,34 +152,57 @@ static const struct proc_ops proc_scsi_ops = {
  * Sets sht->proc_dir to the new directory.
  */
 
-void scsi_proc_hostdir_add(struct scsi_host_template *sht)
+void scsi_proc_hostdir_add(const struct scsi_host_template *sht)
 {
+	struct scsi_proc_entry *e;
+
 	if (!sht->show_info)
 		return;
 
 	mutex_lock(&global_host_template_mutex);
-	if (!sht->present++) {
-		sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
-        	if (!sht->proc_dir)
-			printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
-			       __func__, sht->proc_name);
+	e = __scsi_lookup_proc_entry(sht);
+	if (!e) {
+		e = kzalloc(sizeof(*e), GFP_KERNEL);
+		if (!e)
+			goto unlock;
 	}
+	if (e->present++) {
+		e = NULL;
+		goto unlock;
+	}
+	e->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
+	if (!e->proc_dir) {
+		printk(KERN_ERR "%s: proc_mkdir failed for %s\n", __func__,
+		       sht->proc_name);
+		e->present--;
+		goto unlock;
+	}
+	e->sht = sht;
+	list_add_tail(&e->entry, &scsi_proc_list);
+	e = NULL;
+unlock:
 	mutex_unlock(&global_host_template_mutex);
+
+	kfree(e);
 }
 
 /**
  * scsi_proc_hostdir_rm - remove directory in /proc for a scsi host
  * @sht: owner of directory
  */
-void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
+void scsi_proc_hostdir_rm(const struct scsi_host_template *sht)
 {
+	struct scsi_proc_entry *e;
+
 	if (!sht->show_info)
 		return;
 
 	mutex_lock(&global_host_template_mutex);
-	if (!--sht->present && sht->proc_dir) {
+	e = __scsi_lookup_proc_entry(sht);
+	if (e && !--e->present) {
 		remove_proc_entry(sht->proc_name, proc_scsi);
-		sht->proc_dir = NULL;
+		list_del(&e->entry);
+		kfree(e);
 	}
 	mutex_unlock(&global_host_template_mutex);
 }
@@ -148,16 +214,21 @@ void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
  */
 void scsi_proc_host_add(struct Scsi_Host *shost)
 {
-	struct scsi_host_template *sht = shost->hostt;
+	const struct scsi_host_template *sht = shost->hostt;
+	struct scsi_proc_entry *e;
 	struct proc_dir_entry *p;
 	char name[10];
 
-	if (!sht->proc_dir)
+	if (!sht->show_info)
+		return;
+
+	e = scsi_lookup_proc_entry(sht);
+	if (!e)
 		return;
 
 	sprintf(name,"%d", shost->host_no);
-	p = proc_create_data(name, S_IRUGO | S_IWUSR,
-		sht->proc_dir, &proc_scsi_ops, shost);
+	p = proc_create_data(name, S_IRUGO | S_IWUSR, e->proc_dir,
+			     &proc_scsi_ops, shost);
 	if (!p)
 		printk(KERN_ERR "%s: Failed to register host %d in"
 		       "%s\n", __func__, shost->host_no,
@@ -170,13 +241,19 @@ void scsi_proc_host_add(struct Scsi_Host *shost)
  */
 void scsi_proc_host_rm(struct Scsi_Host *shost)
 {
+	const struct scsi_host_template *sht = shost->hostt;
+	struct scsi_proc_entry *e;
 	char name[10];
 
-	if (!shost->hostt->proc_dir)
+	if (!sht->show_info)
+		return;
+
+	e = scsi_lookup_proc_entry(sht);
+	if (!e)
 		return;
 
 	sprintf(name,"%d", shost->host_no);
-	remove_proc_entry(name, shost->hostt->proc_dir);
+	remove_proc_entry(name, e->proc_dir);
 }
 /**
  * proc_print_scsidevice - return data about this host
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 030faca947d2..fb8184d87384 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -357,12 +357,6 @@ struct scsi_host_template {
 	 */
 	const char *proc_name;
 
-	/*
-	 * Used to store the procfs directory if a driver implements the
-	 * show_info method.
-	 */
-	struct proc_dir_entry *proc_dir;
-
 	/*
 	 * This determines if we will use a non-interrupt driven
 	 * or an interrupt driven scheme.  It is set to the maximum number
@@ -423,12 +417,6 @@ struct scsi_host_template {
 	 */
 	short cmd_per_lun;
 
-	/*
-	 * present contains counter indicating how many boards of this
-	 * type were found when we did the scan.
-	 */
-	unsigned char present;
-
 	/* If use block layer to manage tags, this is tag allocation policy */
 	int tag_alloc_policy;
 

[PATCH v4 4/4] scsi: core: Rework the code for dropping the LLD module reference

[Bart Van Assche]

Instead of clearing the host template module pointer if the LLD kernel
module is being unloaded, set the 'drop_module_ref' SCSI device member.
This patch prepares for constifying the SCSI host template.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.garry@huawei.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_sysfs.c  | 7 +++----
 include/scsi/scsi_device.h | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 5d61f58399dc..822ae60a64b9 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -454,7 +454,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 
 	sdev = container_of(work, struct scsi_device, ew.work);
 
-	mod = sdev->host->hostt->module;
+	mod = sdev->drop_module_ref ? sdev->host->hostt->module : NULL;
 
 	scsi_dh_release_device(sdev);
 
@@ -525,9 +525,8 @@ static void scsi_device_dev_release(struct device *dev)
 {
 	struct scsi_device *sdp = to_scsi_device(dev);
 
-	/* Set module pointer as NULL in case of module unloading */
-	if (!try_module_get(sdp->host->hostt->module))
-		sdp->host->hostt->module = NULL;
+	if (try_module_get(sdp->host->hostt->module))
+		sdp->drop_module_ref = true;
 
 	execute_in_process_context(scsi_device_dev_release_usercontext,
 				   &sdp->ew);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 2493bd65351a..b03176b69056 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -214,6 +214,7 @@ struct scsi_device {
 					 * creation time */
 	unsigned ignore_media_change:1; /* Ignore MEDIA CHANGE on resume */
 	unsigned silence_suspend:1;	/* Do not print runtime PM related messages */
+	unsigned drop_module_ref:1;
 
 	unsigned int queue_stopped;	/* request queue is quiesced */
 	bool offline_already;		/* Device offline message logged */

Re: [PATCH v4 1/4] scsi: esas2r: Initialize two host template members implicitly

[John Garry]

On 13/09/2022 20:57, Bart Van Assche wrote:
> Prepare for removing the 'proc_dir' and 'present' members from the SCSI
> host template by implicitly initializing 'present' and 'emulated' in
> 'driver_template'.
> 
> Cc: Bradley Grove <linuxdrivers@attotech.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---



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

Re: [PATCH v4 2/4] scsi: esas2r: Introduce scsi_template_proc_dir()

[John Garry]

On 13/09/2022 20:57, Bart Van Assche wrote:
> Prepare for removing the 'proc_dir' and 'present' members from the SCSI
> host template.


>This patch does not change any functionality.

... intentionally :)

> 
> Cc: Bradley Grove<linuxdrivers@attotech.com>
> Cc: Christoph Hellwig<hch@lst.de>
> Cc: Ming Lei<ming.lei@redhat.com>
> Cc: Hannes Reinecke<hare@suse.de>
> Cc: John Garry<john.garry@huawei.com>
> Cc: Mike Christie<michael.christie@oracle.com>
> Cc: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
> Signed-off-by: Bart Van Assche<bvanassche@acm.org>

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

Re: [PATCH v4 0/4] Prepare for constifying SCSI host templates

[John Garry]

On 13/09/2022 20:57, Bart Van Assche wrote:
> Hi Martin,
> 
> This patch series prepares for constifying SCSI host templates by moving the
> members that are not constant out of the SCSI host template. Please consider
> this patch series for the next merge window.
> 
> Thanks,
> 

note: I find that this series does not apply cleanly to mkp-scsi 6.1 
staging (which I guess it should), but ok for v6.0-c5

Thanks,
John

> Bart.
> 
> Changes compared to v3:
> - Changed the 'present' counter from 8 to 32 bits.
> - Fixed a bug in an error path (reported by John Garry).
> - Changed EXPORT_SYMBOL() into EXPORT_SYMBOL_GPL().
> - Split patch 1/3 into two patches.
> 
> Changes compared to v2:
> - Optimized the show_info == NULL case.
> - Added a patch that removes the code that clears the module pointer in the host
>    template.
> 
> Changes compared to v1:
> - Fix the CONFIG_SCSI_PROC_FS=n build.
> 
> Bart Van Assche (4):
>    scsi: esas2r: Initialize two host template members implicitly
>    scsi: esas2r: Introduce scsi_template_proc_dir()
>    scsi: core: Introduce a new list for SCSI proc directory entries
>    scsi: core: Rework the code for dropping the LLD module reference
> 
>   drivers/scsi/esas2r/esas2r_main.c |  19 +++--
>   drivers/scsi/scsi_priv.h          |   4 +-
>   drivers/scsi/scsi_proc.c          | 120 ++++++++++++++++++++++++++----
>   drivers/scsi/scsi_sysfs.c         |   7 +-
>   include/scsi/scsi_device.h        |   1 +
>   include/scsi/scsi_host.h          |  18 ++---
>   6 files changed, 127 insertions(+), 42 deletions(-)
> 
> 
> .

Re: [PATCH v4 3/4] scsi: core: Introduce a new list for SCSI proc directory entries

[John Garry]

On 13/09/2022 20:57, Bart Van Assche wrote:
> Instead of using scsi_host_template members to track the SCSI proc
> directory entries, track these entries in a list. This patch changes the
> time needed for looking up the proc dir pointer from O(1) into O(n). I
> think this is acceptable since the number of SCSI host adapter types per
> host is usually small (less than ten).
> 
> This patch has been tested by attaching two USB storage devices to a
> qemu host:
> 
> $ grep -aH . /proc/scsi/usb-storage/*
> /proc/scsi/usb-storage/7:   Host scsi7: usb-storage
> /proc/scsi/usb-storage/7:       Vendor: QEMU
> /proc/scsi/usb-storage/7:      Product: QEMU USB HARDDRIVE
> /proc/scsi/usb-storage/7:Serial Number: 1-0000:00:02.1:00.0-6
> /proc/scsi/usb-storage/7:     Protocol: Transparent SCSI
> /proc/scsi/usb-storage/7:    Transport: Bulk
> /proc/scsi/usb-storage/7:       Quirks: SANE_SENSE
> /proc/scsi/usb-storage/8:   Host scsi8: usb-storage
> /proc/scsi/usb-storage/8:       Vendor: QEMU
> /proc/scsi/usb-storage/8:      Product: QEMU USB HARDDRIVE
> /proc/scsi/usb-storage/8:Serial Number: 1-0000:00:02.1:00.0-7
> /proc/scsi/usb-storage/8:     Protocol: Transparent SCSI
> /proc/scsi/usb-storage/8:    Transport: Bulk
> /proc/scsi/usb-storage/8:       Quirks: SANE_SENSE
> 
> This patch prepares for constifying most SCSI host templates.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/scsi_priv.h |   4 +-
>   drivers/scsi/scsi_proc.c | 111 +++++++++++++++++++++++++++++++++------
>   include/scsi/scsi_host.h |  12 -----
>   3 files changed, 96 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index f385b3f04d6e..be5d7c9b7f39 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -100,8 +100,8 @@ extern void scsi_evt_thread(struct work_struct *work);
>   
>   /* scsi_proc.c */
>   #ifdef CONFIG_SCSI_PROC_FS
> -extern void scsi_proc_hostdir_add(struct scsi_host_template *);
> -extern void scsi_proc_hostdir_rm(struct scsi_host_template *);
> +extern void scsi_proc_hostdir_add(const struct scsi_host_template *);
> +extern void scsi_proc_hostdir_rm(const struct scsi_host_template *);
>   extern void scsi_proc_host_add(struct Scsi_Host *);
>   extern void scsi_proc_host_rm(struct Scsi_Host *);
>   extern int scsi_init_procfs(void);
> diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
> index 456b43097288..7dffa73d6f92 100644
> --- a/drivers/scsi/scsi_proc.c
> +++ b/drivers/scsi/scsi_proc.c
> @@ -43,8 +43,23 @@
>   
>   static struct proc_dir_entry *proc_scsi;
>   
> -/* Protect sht->present and sht->proc_dir */
> +/* Protects scsi_proc_list */
>   static DEFINE_MUTEX(global_host_template_mutex);
> +static LIST_HEAD(scsi_proc_list);
> +
> +/**
> + * struct scsi_proc_entry - (host template, SCSI proc dir) association
> + * @entry: entry in scsi_proc_list.
> + * @sht: SCSI host template associated with the procfs directory.
> + * @proc_dir: procfs directory associated with the SCSI host template.
> + * @present: Number of SCSI hosts instantiated for @sht.
> + */
> +struct scsi_proc_entry {
> +	struct list_head	entry;
> +	const struct scsi_host_template *sht;
> +	struct proc_dir_entry	*proc_dir;
> +	unsigned int		present;
> +};
>   
>   static ssize_t proc_scsi_host_write(struct file *file, const char __user *buf,
>                              size_t count, loff_t *ppos)
> @@ -83,6 +98,32 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
>   				4 * PAGE_SIZE);
>   }
>   
> +static struct scsi_proc_entry *
> +__scsi_lookup_proc_entry(const struct scsi_host_template *sht)
> +{
> +	struct scsi_proc_entry *e;
> +
> +	lockdep_assert_held(&global_host_template_mutex);
> +
> +	list_for_each_entry(e, &scsi_proc_list, entry)
> +		if (e->sht == sht)
> +			return e;
> +
> +	return NULL;
> +}
> +
> +static struct scsi_proc_entry *
> +scsi_lookup_proc_entry(const struct scsi_host_template *sht)
> +{
> +	struct scsi_proc_entry *e;
> +
> +	mutex_lock(&global_host_template_mutex);
> +	e = __scsi_lookup_proc_entry(sht);
> +	mutex_unlock(&global_host_template_mutex);
> +
> +	return e;
> +}
> +
>   /**
>    * scsi_template_proc_dir() - returns the procfs dir for a SCSI host template
>    * @sht: SCSI host template pointer.
> @@ -90,7 +131,9 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
>   struct proc_dir_entry *
>   scsi_template_proc_dir(const struct scsi_host_template *sht)
>   {
> -	return sht->proc_dir;
> +	struct scsi_proc_entry *e = scsi_lookup_proc_entry(sht);
> +
> +	return e ? e->proc_dir : NULL;
>   }
>   EXPORT_SYMBOL_GPL(scsi_template_proc_dir);
>   
> @@ -109,34 +152,57 @@ static const struct proc_ops proc_scsi_ops = {
>    * Sets sht->proc_dir to the new directory.
>    */
>   
> -void scsi_proc_hostdir_add(struct scsi_host_template *sht)
> +void scsi_proc_hostdir_add(const struct scsi_host_template *sht)
>   {
> +	struct scsi_proc_entry *e;
> +
>   	if (!sht->show_info)
>   		return;
>   
>   	mutex_lock(&global_host_template_mutex);
> -	if (!sht->present++) {
> -		sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
> -        	if (!sht->proc_dir)
> -			printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
> -			       __func__, sht->proc_name);
> +	e = __scsi_lookup_proc_entry(sht);
> +	if (!e) {
> +		e = kzalloc(sizeof(*e), GFP_KERNEL);


So this following scenario is safe, right?

1. host0 calls scsi_proc_hostdir_add() but alloc here fails
2. host1 calls scsi_proc_hostdir_add() but alloc passes, so e->present = 1
3. host0 calls scsi_proc_hostdir_rm(), so e->present goes to zero and we 
dealloc the scsi_proc_entry
4. host1 calls scsi_proc_hostdir_rm(), but does not find the 
scsi_proc_entry and returns

I suppose the only problem is that we try to free the procfs dir but 
host1 still has a procfs entry after 3

I just wonder why we can't make scsi_alloc_alloc() -> 
scsi_proc_hostdir_add() error, i.e. return an error code in this 
function - it would seem to make things simpler

> +		if (!e)
> +			goto unlock;
>   	}
> +	if (e->present++) {
> +		e = NULL;
> +		goto unlock;
> +	}
> +	e->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
> +	if (!e->proc_dir) {
> +		printk(KERN_ERR "%s: proc_mkdir failed for %s\n", __func__,
> +		       sht->proc_name);
> +		e->present--;

I don't see why we decrement e->present and then free e

> +		goto unlock;
> +	}
> +	e->sht = sht;
> +	list_add_tail(&e->entry, &scsi_proc_list);
> +	e = NULL;
> +unlock:
>   	mutex_unlock(&global_host_template_mutex);
> +
> +	kfree(e);
>   }
>   
>   /**
>    * scsi_proc_hostdir_rm - remove directory in /proc for a scsi host
>    * @sht: owner of directory
>    */
> -void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
> +void scsi_proc_hostdir_rm(const struct scsi_host_template *sht)
>   {
> +	struct scsi_proc_entry *e;
> +
>   	if (!sht->show_info)
>   		return;
>   
>   	mutex_lock(&global_host_template_mutex);
> -	if (!--sht->present && sht->proc_dir) {
> +	e = __scsi_lookup_proc_entry(sht);
> +	if (e && !--e->present) {
>   		remove_proc_entry(sht->proc_name, proc_scsi);
> -		sht->proc_dir = NULL;
> +		list_del(&e->entry);
> +		kfree(e);
>   	}
>   	mutex_unlock(&global_host_template_mutex);
>   }
> @@ -148,16 +214,21 @@ void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
>    */
>   void scsi_proc_host_add(struct Scsi_Host *shost)
>   {
> -	struct scsi_host_template *sht = shost->hostt;
> +	const struct scsi_host_template *sht = shost->hostt;
> +	struct scsi_proc_entry *e;
>   	struct proc_dir_entry *p;
>   	char name[10];
>   
> -	if (!sht->proc_dir)
> +	if (!sht->show_info)
> +		return;
> +
> +	e = scsi_lookup_proc_entry(sht);
> +	if (!e)
>   		return;
>   
>   	sprintf(name,"%d", shost->host_no);
> -	p = proc_create_data(name, S_IRUGO | S_IWUSR,
> -		sht->proc_dir, &proc_scsi_ops, shost);
> +	p = proc_create_data(name, S_IRUGO | S_IWUSR, e->proc_dir,
> +			     &proc_scsi_ops, shost);
>   	if (!p)
>   		printk(KERN_ERR "%s: Failed to register host %d in"
>   		       "%s\n", __func__, shost->host_no,
> @@ -170,13 +241,19 @@ void scsi_proc_host_add(struct Scsi_Host *shost)
>    */
>   void scsi_proc_host_rm(struct Scsi_Host *shost)
>   {
> +	const struct scsi_host_template *sht = shost->hostt;
> +	struct scsi_proc_entry *e;
>   	char name[10];
>   
> -	if (!shost->hostt->proc_dir)
> +	if (!sht->show_info)
> +		return;
> +
> +	e = scsi_lookup_proc_entry(sht);
> +	if (!e)
>   		return;
>   
>   	sprintf(name,"%d", shost->host_no);
> -	remove_proc_entry(name, shost->hostt->proc_dir);
> +	remove_proc_entry(name, e->proc_dir);
>   }
>   /**
>    * proc_print_scsidevice - return data about this host
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 030faca947d2..fb8184d87384 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -357,12 +357,6 @@ struct scsi_host_template {
>   	 */
>   	const char *proc_name;
>   
> -	/*
> -	 * Used to store the procfs directory if a driver implements the
> -	 * show_info method.
> -	 */
> -	struct proc_dir_entry *proc_dir;
> -
>   	/*
>   	 * This determines if we will use a non-interrupt driven
>   	 * or an interrupt driven scheme.  It is set to the maximum number
> @@ -423,12 +417,6 @@ struct scsi_host_template {
>   	 */
>   	short cmd_per_lun;
>   
> -	/*
> -	 * present contains counter indicating how many boards of this
> -	 * type were found when we did the scan.
> -	 */
> -	unsigned char present;
> -
>   	/* If use block layer to manage tags, this is tag allocation policy */
>   	int tag_alloc_policy;
>   
> 
> .

Re: [PATCH v4 4/4] scsi: core: Rework the code for dropping the LLD module reference

[John Garry]

On 13/09/2022 20:57, Bart Van Assche wrote:
> Instead of clearing the host template module pointer if the LLD kernel
> module is being unloaded, set the 'drop_module_ref' SCSI device member.
> This patch prepares for constifying the SCSI host template.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/scsi/scsi_sysfs.c  | 7 +++----
>   include/scsi/scsi_device.h | 1 +
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 5d61f58399dc..822ae60a64b9 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -454,7 +454,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
>   
>   	sdev = container_of(work, struct scsi_device, ew.work);
>   
> -	mod = sdev->host->hostt->module;
> +	mod = sdev->drop_module_ref ? sdev->host->hostt->module : NULL;

I suppose that this works.

My reservation is that there were some concerns of current module 
referencing solution, so may not be better to build directly on it:

https://lore.kernel.org/linux-scsi/Ynt0aFMX+z%2FUhGJ2@infradead.org/

Thanks,
John

>   
>   	scsi_dh_release_device(sdev);
>   
> @@ -525,9 +525,8 @@ static void scsi_device_dev_release(struct device *dev)
>   {
>   	struct scsi_device *sdp = to_scsi_device(dev);
>   
> -	/* Set module pointer as NULL in case of module unloading */
> -	if (!try_module_get(sdp->host->hostt->module))
> -		sdp->host->hostt->module = NULL;
> +	if (try_module_get(sdp->host->hostt->module))
> +		sdp->drop_module_ref = true;
>   
>   	execute_in_process_context(scsi_device_dev_release_usercontext,
>   				   &sdp->ew);
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 2493bd65351a..b03176b69056 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -214,6 +214,7 @@ struct scsi_device {
>   					 * creation time */
>   	unsigned ignore_media_change:1; /* Ignore MEDIA CHANGE on resume */
>   	unsigned silence_suspend:1;	/* Do not print runtime PM related messages */
> +	unsigned drop_module_ref:1;
>   
>   	unsigned int queue_stopped;	/* request queue is quiesced */
>   	bool offline_already;		/* Device offline message logged */
> 
> .

Re: [PATCH v4 2/4] scsi: esas2r: Introduce scsi_template_proc_dir()

[Bart Van Assche]

On 9/14/22 02:06, John Garry wrote:
> On 13/09/2022 20:57, Bart Van Assche wrote:
>> This patch does not change any functionality.
> 
> ... intentionally :)

The above comment is not clear to me, nor whether any action is expected 
from my side?

Thanks,

Bart.

Re: [PATCH v4 3/4] scsi: core: Introduce a new list for SCSI proc directory entries

[Bart Van Assche]

On 9/14/22 02:43, John Garry wrote:
> On 13/09/2022 20:57, Bart Van Assche wrote:
>> +void scsi_proc_hostdir_add(const struct scsi_host_template *sht)
>>   {
>> +    struct scsi_proc_entry *e;
>> +
>>       if (!sht->show_info)
>>           return;
>>       mutex_lock(&global_host_template_mutex);
>> -    if (!sht->present++) {
>> -        sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
>> -            if (!sht->proc_dir)
>> -            printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
>> -                   __func__, sht->proc_name);
>> +    e = __scsi_lookup_proc_entry(sht);
>> +    if (!e) {
>> +        e = kzalloc(sizeof(*e), GFP_KERNEL);
> 
> So this following scenario is safe, right?
> 
> 1. host0 calls scsi_proc_hostdir_add() but alloc here fails
> 2. host1 calls scsi_proc_hostdir_add() but alloc passes, so e->present = 1
> 3. host0 calls scsi_proc_hostdir_rm(), so e->present goes to zero and we 
> dealloc the scsi_proc_entry
> 4. host1 calls scsi_proc_hostdir_rm(), but does not find the 
> scsi_proc_entry and returns
> 
> I suppose the only problem is that we try to free the procfs dir but 
> host1 still has a procfs entry after 3
> 
> I just wonder why we can't make scsi_alloc_alloc() -> 
> scsi_proc_hostdir_add() error, i.e. return an error code in this 
> function - it would seem to make things simpler

That sounds like a good idea to me. I will look into this.

>> +    e->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
>> +    if (!e->proc_dir) {
>> +        printk(KERN_ERR "%s: proc_mkdir failed for %s\n", __func__,
>> +               sht->proc_name);
>> +        e->present--;
> 
> I don't see why we decrement e->present and then free e

I will remove the code that decrements e->present again.

Thanks,

Bart.

Re: [PATCH v4 2/4] scsi: esas2r: Introduce scsi_template_proc_dir()

[John Garry]

On 14/09/2022 17:43, Bart Van Assche wrote:
> On 9/14/22 02:06, John Garry wrote:
>> On 13/09/2022 20:57, Bart Van Assche wrote:
>>> This patch does not change any functionality.
>>
>> ... intentionally :)

I was just half-joking, so please ignore me.

It's common to write that there is no intentional change in 
functionality, as maybe there is some unknown change in functionality.

Thanks,
John

Re: [PATCH v4 0/4] Prepare for constifying SCSI host templates

[Bart Van Assche]

On 9/14/22 02:21, John Garry wrote:
> note: I find that this series does not apply cleanly to mkp-scsi 6.1 
> staging (which I guess it should), but ok for v6.0-c5

Hi John,

A few reverts have been sent to Linus and are present in his tree but 
not in Martin's for-next branch. Hence the choice to base this patch 
series on Linus' master branch.

Thanks,

Bart.

Re: [PATCH v4 4/4] scsi: core: Rework the code for dropping the LLD module reference

[Bart Van Assche]

On 9/14/22 02:52, John Garry wrote:
> On 13/09/2022 20:57, Bart Van Assche wrote:
>> -    mod = sdev->host->hostt->module;
>> +    mod = sdev->drop_module_ref ? sdev->host->hostt->module : NULL;
> 
> I suppose that this works.
> 
> My reservation is that there were some concerns of current module 
> referencing solution, so may not be better to build directly on it:
> 
> https://lore.kernel.org/linux-scsi/Ynt0aFMX+z%2FUhGJ2@infradead.org/

Hi John,

The above link points at a reply from Christoph Hellwig with the 
suggestion to use __module_get(). Implementing that suggestion would 
require to modify the kernel module implementation. My conclusion from 
reading the code in kernel/module/main.c is that increasing the module 
reference count after unloading has started currently has no effect. I 
have not found any code in the delete_module() system call 
implementation that waits for the reference count to drop to zero after 
try_stop_module() succeeded. I will look into this.

Thanks,

Bart.