diff mbox series

[1/4] scsi: core: constify pointer to scsi_host_template

Message ID 20220408103027.311624-1-krzysztof.kozlowski@linaro.org
State New
Headers show
Series [1/4] scsi: core: constify pointer to scsi_host_template | expand

Commit Message

Krzysztof Kozlowski April 8, 2022, 10:30 a.m. UTC
Several pointers to 'struct scsi_host_template' do not modify it, so
made them const for safety.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/scsi/hosts.c      |  2 +-
 drivers/scsi/scsi_error.c | 17 +++++++++--------
 drivers/scsi/scsi_proc.c  |  2 +-
 drivers/scsi/scsi_sysfs.c |  6 +++---
 4 files changed, 14 insertions(+), 13 deletions(-)

Comments

John Garry April 12, 2022, 7:57 a.m. UTC | #1
On 08/04/2022 20:31, Ewan D. Milne wrote:
> On Fri, 2022-04-08 at 13:57 +0100, John Garry wrote:
>> On 08/04/2022 13:32, Krzysztof Kozlowski wrote:
>>> On 08/04/2022 14:14, John Garry wrote:
>>>> On 08/04/2022 11:30, Krzysztof Kozlowski wrote:
>>>>> Several pointers to 'struct scsi_host_template' do not modify it, so
>>>>> made them const for safety.
>>>>>
>>>> Is this standard practice? What is so special here?
>>> This is standard practice and there is nothing special here. Pointers to
>>> const are preferred because:
>>> 1. They add safety if data is actually const. This is not yet the case,
>>> but scsi_host_template allocation could be made const with some effort.
> 
> This seems unlikely, because some drivers, e.g. vmw_pvscsi and scsi_debug,
> modify the scsi_host_template based on things like module parameters.
>

The standard flow is:

shost = scsi_host_alloc(sht, )

// modify shost, like
shost->cmd_per_lun = 5;

scsi_add_host(shost)

Is there some reason for which those two drivers can't follow that?

>>
>> To me this seems better, but I think that some drivers might modify
>> their scsi_host_template (so not possible)
> 
> Several drivers modify scsi_host_template, e.g. .can_queue, .cmd_per_lun
> 
> There is also code in lpfc_create_port() that initializes a scsi_host_template
> that is embedded in the lpfc_hba struct.  I don't think it gets modified after
> scsi_add_host() but it seems like driver maintainers might expect to be able
> to do so, in general.
> 

Even so, I don't see why other drivers cannot declare their 
scsi_host_template as const. C would have no problem with sht not be 
being const for this:

struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, )

thanks,
John
Christoph Hellwig April 20, 2022, 7:03 a.m. UTC | #2
On Tue, Apr 12, 2022 at 08:57:39AM +0100, John Garry wrote:
> > 
> 
> The standard flow is:
> 
> shost = scsi_host_alloc(sht, )
> 
> // modify shost, like
> shost->cmd_per_lun = 5;
> 
> scsi_add_host(shost)
> 
> Is there some reason for which those two drivers can't follow that?

I think they should.  Method tables should not be mutable data.
John Garry April 25, 2022, 8:58 a.m. UTC | #3
On 20/04/2022 08:03, Christoph Hellwig wrote:
>> The standard flow is:
>>
>> shost = scsi_host_alloc(sht, )
>>
>> // modify shost, like
>> shost->cmd_per_lun = 5;
>>
>> scsi_add_host(shost)
>>
>> Is there some reason for which those two drivers can't follow that?
> I think they should.  Method tables should not be mutable data.
> .

Hi Krzysztof,

Do you have any interest in going further with your work and trying to 
change all SCSI driver instances of scsi_host_template to be const? I am 
not sure if it has been attempted before...

Thanks,
John
Krzysztof Kozlowski April 25, 2022, 9:22 a.m. UTC | #4
On 25/04/2022 10:58, John Garry wrote:
> On 20/04/2022 08:03, Christoph Hellwig wrote:
>>> The standard flow is:
>>>
>>> shost = scsi_host_alloc(sht, )
>>>
>>> // modify shost, like
>>> shost->cmd_per_lun = 5;
>>>
>>> scsi_add_host(shost)
>>>
>>> Is there some reason for which those two drivers can't follow that?
>> I think they should.  Method tables should not be mutable data.
>> .
> 
> Hi Krzysztof,
> 
> Do you have any interest in going further with your work and trying to 
> change all SCSI driver instances of scsi_host_template to be const? I am 
> not sure if it has been attempted before...

I can work on this, but what about the SCSI core modifying the template?
For example scsi_proc_hostdir_rm(): 'present' and 'proc_dir' members.
Where should they be stored? Should they be moved to the Scsi_Host?


Best regards,
Krzysztof
John Garry April 25, 2022, 1:04 p.m. UTC | #5
On 25/04/2022 10:22, Krzysztof Kozlowski wrote:
> On 25/04/2022 10:58, John Garry wrote:
>> On 20/04/2022 08:03, Christoph Hellwig wrote:
>>>> The standard flow is:
>>>>
>>>> shost = scsi_host_alloc(sht, )
>>>>
>>>> // modify shost, like
>>>> shost->cmd_per_lun = 5;
>>>>
>>>> scsi_add_host(shost)
>>>>
>>>> Is there some reason for which those two drivers can't follow that?
>>> I think they should.  Method tables should not be mutable data.
>>> .
>>
>> Hi Krzysztof,
>>
>> Do you have any interest in going further with your work and trying to
>> change all SCSI driver instances of scsi_host_template to be const? I am
>> not sure if it has been attempted before...
> 
> I can work on this, but what about the SCSI core modifying the template?

I hope that this isn't a can of worms...

> For example scsi_proc_hostdir_rm(): 'present' and 'proc_dir' members.
> Where should they be stored? Should they be moved to the Scsi_Host?
> 

I don't think scsi_Host is appropriate as this is per-scsi host 
template, unless you see a way to do it that way. Alternatively we could 
keep a separate list of registered sht, like this:

struct sht_proc_dir {
	int cnt;
	struct list_head list;
	struct proc_dir_entry *proc_dir;
	struct scsi_host_template *sht;
};
static LIST_HEAD(sht_proc_dir_list);

void scsi_proc_hostdir_add(struct scsi_host_template *sht)
{
	struct sht_proc_dir *dir;

	if (!sht->show_info)
		return;

	mutex_lock(&global_host_template_mutex);
	list_for_each_entry(dir, &sht_proc_dir_list, list) {
		if (dir->sht == sht) {
			dir->cnt++;
			goto out;
		}
	}
	dir = kzalloc(sizeof(*dir), GFP_KERNEL);
	if (!dir)
		goto out;

	dir->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
	if (!dir->proc_dir) {
		printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
			       __func__, sht->proc_name);
		kfree(dir);
		goto out;
	}

	dir->cnt++;
	list_add_tail(&dir->list, &sht_proc_dir_list);
out:
	mutex_unlock(&global_host_template_mutex);
}

and so on..

--->8---

Thanks,
John
Bart Van Assche April 26, 2022, 1:16 a.m. UTC | #6
On 4/25/22 06:04, John Garry wrote:
> On 25/04/2022 10:22, Krzysztof Kozlowski wrote:
>> For example scsi_proc_hostdir_rm(): 'present' and 'proc_dir' members.
>> Where should they be stored? Should they be moved to the Scsi_Host?
>>
> 
> I don't think scsi_Host is appropriate as this is per-scsi host 
> template, unless you see a way to do it that way. Alternatively we could 
> keep a separate list of registered sht, like this:
> 
> struct sht_proc_dir {
>      int cnt;
>      struct list_head list;
>      struct proc_dir_entry *proc_dir;
>      struct scsi_host_template *sht;
> };
> static LIST_HEAD(sht_proc_dir_list);
> 
> void scsi_proc_hostdir_add(struct scsi_host_template *sht)
> {
>      struct sht_proc_dir *dir;
> 
>      if (!sht->show_info)
>          return;
> 
>      mutex_lock(&global_host_template_mutex);
>      list_for_each_entry(dir, &sht_proc_dir_list, list) {
>          if (dir->sht == sht) {
>              dir->cnt++;
>              goto out;
>          }
>      }
>      dir = kzalloc(sizeof(*dir), GFP_KERNEL);
>      if (!dir)
>          goto out;
> 
>      dir->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
>      if (!dir->proc_dir) {
>          printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
>                     __func__, sht->proc_name);
>          kfree(dir);
>          goto out;
>      }
> 
>      dir->cnt++;
>      list_add_tail(&dir->list, &sht_proc_dir_list);
> out:
>      mutex_unlock(&global_host_template_mutex);
> }

How about removing scsi_proc_hostdir_add(), scsi_proc_hostdir_rm() and 
all other code that creates files or directories under /proc/scsi? There 
should be corresponding entries in sysfs for all /proc/scsi entries. 
Some tools in sg3_utils use that directory so sg3_utils will have to be 
updated.

Thanks,

Bart.
Douglas Gilbert April 26, 2022, 1:54 a.m. UTC | #7
On 2022-04-25 21:16, Bart Van Assche wrote:
> On 4/25/22 06:04, John Garry wrote:
>> On 25/04/2022 10:22, Krzysztof Kozlowski wrote:
>>> For example scsi_proc_hostdir_rm(): 'present' and 'proc_dir' members.
>>> Where should they be stored? Should they be moved to the Scsi_Host?
>>>
>>
>> I don't think scsi_Host is appropriate as this is per-scsi host template, 
>> unless you see a way to do it that way. Alternatively we could keep a separate 
>> list of registered sht, like this:
>>
>> struct sht_proc_dir {
>>      int cnt;
>>      struct list_head list;
>>      struct proc_dir_entry *proc_dir;
>>      struct scsi_host_template *sht;
>> };
>> static LIST_HEAD(sht_proc_dir_list);
>>
>> void scsi_proc_hostdir_add(struct scsi_host_template *sht)
>> {
>>      struct sht_proc_dir *dir;
>>
>>      if (!sht->show_info)
>>          return;
>>
>>      mutex_lock(&global_host_template_mutex);
>>      list_for_each_entry(dir, &sht_proc_dir_list, list) {
>>          if (dir->sht == sht) {
>>              dir->cnt++;
>>              goto out;
>>          }
>>      }
>>      dir = kzalloc(sizeof(*dir), GFP_KERNEL);
>>      if (!dir)
>>          goto out;
>>
>>      dir->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
>>      if (!dir->proc_dir) {
>>          printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
>>                     __func__, sht->proc_name);
>>          kfree(dir);
>>          goto out;
>>      }
>>
>>      dir->cnt++;
>>      list_add_tail(&dir->list, &sht_proc_dir_list);
>> out:
>>      mutex_unlock(&global_host_template_mutex);
>> }
> 
> How about removing scsi_proc_hostdir_add(), scsi_proc_hostdir_rm() and all other 
> code that creates files or directories under /proc/scsi? There should be 
> corresponding entries in sysfs for all /proc/scsi entries. Some tools in 
> sg3_utils use that directory so sg3_utils will have to be updated.

... breaking this:

~$ cat /proc/scsi/scsi

Attached devices:

Host: scsi3 Channel: 00 Id: 00 Lun: 00

   Vendor: IBM-207x Model: HUSMM8020ASS20   Rev: J4B6

   Type:   Direct-Access                    ANSI  SCSI revision: 06

Host: scsi3 Channel: 00 Id: 01 Lun: 00

   Vendor: IBM-207x Model: HUSMM8020ASS20   Rev: J4B6

   Type:   Direct-Access                    ANSI  SCSI revision: 06

Host: scsi3 Channel: 00 Id: 02 Lun: 00

   Vendor: SEAGATE  Model: ST200FM0073      Rev: 0007

   Type:   Direct-Access                    ANSI  SCSI revision: 06
...

A deprecation notice would be helpful, then removal after a few kernel
cycles. Yes, lsscsi can give that output:

$ lsscsi -c

Attached devices:

Host: scsi2 Channel: 00 Target: 00 Lun: 00

   Vendor: SEAGATE  Model: ST200FM0073      Rev: 0007

   Type:   Direct-Access                    ANSI SCSI revision: 06

Host: scsi2 Channel: 00 Target: 01 Lun: 00

   Vendor: WDC      Model: WSH722020AL5204  Rev: C421

   Type:   Zoned Block                      ANSI SCSI revision: 07

Host: scsi2 Channel: 00 Target: 02 Lun: 00

   Vendor: Areca Te Model: ARC-802801.37.69 Rev: 0137

   Type:   Enclosure                        ANSI SCSI revision: 05
...

[Hmmm, in a different order.]

However no distribution that I'm aware of includes lsscsi in its installation.
[Most recent example: Ubuntu 22.04]
Linux is not alone ... in FreeBSD how do you list SCSI devices in your
system? Answer: as root you invoke 'camcontrol devlist', it's so obvious.

Perhaps the Linux kernel could have a deprecation process which uses inotify
or similar to notice accesses to /proc/scsi/scsi (say) and print out
a helpful response along the lines" "this is no longer supported, try using
the lsscsi utility".

Doug Gilbert
Bart Van Assche April 26, 2022, 4:13 a.m. UTC | #8
On 4/25/22 18:54, Douglas Gilbert wrote:
> On 2022-04-25 21:16, Bart Van Assche wrote:
>> How about removing scsi_proc_hostdir_add(), scsi_proc_hostdir_rm() and 
>> all other code that creates files or directories under /proc/scsi? 
>> There should be corresponding entries in sysfs for all /proc/scsi 
>> entries. Some tools in sg3_utils use that directory so sg3_utils will 
>> have to be updated.
> 
> ... breaking this:
> 
> ~$ cat /proc/scsi/scsi
> 
> Attached devices:
> 
> Host: scsi3 Channel: 00 Id: 00 Lun: 00
> 
>    Vendor: IBM-207x Model: HUSMM8020ASS20   Rev: J4B6
> 
>    Type:   Direct-Access                    ANSI  SCSI revision: 06
> 
> Host: scsi3 Channel: 00 Id: 01 Lun: 00
> 
>    Vendor: IBM-207x Model: HUSMM8020ASS20   Rev: J4B6
> 
>    Type:   Direct-Access                    ANSI  SCSI revision: 06
> 
> Host: scsi3 Channel: 00 Id: 02 Lun: 00
> 
>    Vendor: SEAGATE  Model: ST200FM0073      Rev: 0007
> 
>    Type:   Direct-Access                    ANSI  SCSI revision: 06
> ...
> 
> A deprecation notice would be helpful, then removal after a few kernel
> cycles.

Agreed with the deprecation notice + delayed removal, but is anyone 
using cat /proc/scsi/scsi?

> Yes, lsscsi can give that output:
> 
> $ lsscsi -c
> 
> Attached devices:
> 
> Host: scsi2 Channel: 00 Target: 00 Lun: 00
> 
>    Vendor: SEAGATE  Model: ST200FM0073      Rev: 0007
> 
>    Type:   Direct-Access                    ANSI SCSI revision: 06
> 
> Host: scsi2 Channel: 00 Target: 01 Lun: 00
> 
>    Vendor: WDC      Model: WSH722020AL5204  Rev: C421
> 
>    Type:   Zoned Block                      ANSI SCSI revision: 07
> 
> Host: scsi2 Channel: 00 Target: 02 Lun: 00
> 
>    Vendor: Areca Te Model: ARC-802801.37.69 Rev: 0137
> 
>    Type:   Enclosure                        ANSI SCSI revision: 05
> ...
> 
> [Hmmm, in a different order.]
> 
> However no distribution that I'm aware of includes lsscsi in its 
> installation.
> [Most recent example: Ubuntu 22.04]

Hmm ... are you sure? Last time I looked into this an lsscsi package was 
available for every distro I tried (RHEL, SLES, Debian and openSUSE). 
See also 
https://packages.debian.org/search?searchon=contents&keywords=lsscsi&mode=path&suite=stable&arch=any.

Are there other utilities in sg3_utils that would break if the 
/proc/scsi directory would be removed?

$ cd sg3_utils && git grep /proc/scsi | wc -l
51

Thanks,

Bart.
Douglas Gilbert April 27, 2022, 1:47 a.m. UTC | #9
On 2022-04-26 00:13, Bart Van Assche wrote:
> On 4/25/22 18:54, Douglas Gilbert wrote:
>> On 2022-04-25 21:16, Bart Van Assche wrote:
>>> How about removing scsi_proc_hostdir_add(), scsi_proc_hostdir_rm() and all 
>>> other code that creates files or directories under /proc/scsi? There should 
>>> be corresponding entries in sysfs for all /proc/scsi entries. Some tools in 
>>> sg3_utils use that directory so sg3_utils will have to be updated.
>>
>> ... breaking this:
>>
>> ~$ cat /proc/scsi/scsi
>>
>> Attached devices:
>>
>> Host: scsi3 Channel: 00 Id: 00 Lun: 00
>>    Vendor: IBM-207x Model: HUSMM8020ASS20   Rev: J4B6
>>    Type:   Direct-Access                    ANSI  SCSI revision: 06
>> Host: scsi3 Channel: 00 Id: 01 Lun: 00
>>    Vendor: IBM-207x Model: HUSMM8020ASS20   Rev: J4B6
>>    Type:   Direct-Access                    ANSI  SCSI revision: 06
>> Host: scsi3 Channel: 00 Id: 02 Lun: 00
>>    Vendor: SEAGATE  Model: ST200FM0073      Rev: 0007
>>    Type:   Direct-Access                    ANSI  SCSI revision: 06
>> ...
>>
>> A deprecation notice would be helpful, then removal after a few kernel
>> cycles.
> 
> Agreed with the deprecation notice + delayed removal, but is anyone using cat 
> /proc/scsi/scsi?
> 
>> Yes, lsscsi can give that output:
>>
>> $ lsscsi -c
>> Attached devices:
>> Host: scsi2 Channel: 00 Target: 00 Lun: 00
>>    Vendor: SEAGATE  Model: ST200FM0073      Rev: 0007
>>    Type:   Direct-Access                    ANSI SCSI revision: 06
>> Host: scsi2 Channel: 00 Target: 01 Lun: 00
>>    Vendor: WDC      Model: WSH722020AL5204  Rev: C421
>>    Type:   Zoned Block                      ANSI SCSI revision: 07
>> Host: scsi2 Channel: 00 Target: 02 Lun: 00
>>    Vendor: Areca Te Model: ARC-802801.37.69 Rev: 0137
>>    Type:   Enclosure                        ANSI SCSI revision: 05
>> ...
>>
>> [Hmmm, in a different order.]
>>
>> However no distribution that I'm aware of includes lsscsi in its installation.
>> [Most recent example: Ubuntu 22.04]
> 
> Hmm ... are you sure? Last time I looked into this an lsscsi package was 
> available for every distro I tried (RHEL, SLES, Debian and openSUSE). See also 
> https://packages.debian.org/search?searchon=contents&keywords=lsscsi&mode=path&suite=stable&arch=any.

I was talking about the _initial_ installation. When I install new versions
of Fedora or Ubuntu, or play with a "live" CD (usually a USB stick) one
of the first things I do is get a terminal and then invoke 'lsscsi'.
Invariably that second step fails. And on a "live" USB stick you can install
lsscsi but the next time you use it, lsscsi is gone because those "live"
USB sticks hardly ever have persistent storage set up. [Why not? ..
typically the rest of the storage on such a USB stick is un-utilized.]

> Are there other utilities in sg3_utils that would break if the /proc/scsi 
> directory would be removed?
> 
> $ cd sg3_utils && git grep /proc/scsi | wc -l
> 51

Most of those are in the scripts/rescan-scsi-bus.sh which, judging from the
number of patches and additions it gets, has quite a bit of use out there.
The rest are in my dd variants that are mainly setting /proc/scsi/sg/allow_dio
which has no effect in my sg driver rewrite.

Doug Gilbert
Krzysztof Kozlowski May 6, 2022, 4:42 p.m. UTC | #10
On 25/04/2022 15:04, John Garry wrote:
> 
>> For example scsi_proc_hostdir_rm(): 'present' and 'proc_dir' members.
>> Where should they be stored? Should they be moved to the Scsi_Host?
>>
> 
> I don't think scsi_Host is appropriate as this is per-scsi host 
> template, unless you see a way to do it that way. Alternatively we could 
> keep a separate list of registered sht, like this:
> 
> struct sht_proc_dir {
> 	int cnt;
> 	struct list_head list;
> 	struct proc_dir_entry *proc_dir;
> 	struct scsi_host_template *sht;
> };
> static LIST_HEAD(sht_proc_dir_list);

Hi everyone,

It took me some time to get back to this topic. I moved the proc_dir out
of SHT, how John proposed. Patches do not look that bad:
The commit:
https://github.com/krzk/linux/commit/157eb2ee8867afbae9dac3836e4c0bedb542e5c1

Branch:
https://github.com/krzk/linux/commits/n/qcom-ufs-opp-cleanups-v2

However this does not solve the problem. The SHT has "module" which gets
incremented/decremented. Exactly like in case of other drivers
(driver->owner).

I started moving the SHT->module to a new field scsi_host->owner and
trying to use the parent's driver (so PCI, USB) owner.
I am not sure if it is correct approach, so before implementing such big
change affecting multiple subsystems (USB, ATA, SCSI) - can you share
ideas/opinion?

The Work-in-Progress looks like this (last commit):
https://github.com/krzk/linux/commit/17609caecd53df20f631703ea084a70e7735b5d7


Best regards,
Krzysztof
John Garry May 9, 2022, 11:28 a.m. UTC | #11
On 06/05/2022 17:42, Krzysztof Kozlowski wrote:
>> I don't think scsi_Host is appropriate as this is per-scsi host
>> template, unless you see a way to do it that way. Alternatively we could
>> keep a separate list of registered sht, like this:
>>
>> struct sht_proc_dir {
>> 	int cnt;
>> 	struct list_head list;
>> 	struct proc_dir_entry *proc_dir;
>> 	struct scsi_host_template *sht;
>> };
>> static LIST_HEAD(sht_proc_dir_list);
> Hi everyone,
> 

Hi Krzysztof,

> It took me some time to get back to this topic. I moved the proc_dir out
> of SHT, how John proposed. Patches do not look that bad:
> The commit:
> https://github.com/krzk/linux/commit/157eb2ee8867afbae9dac3836e4c0bedb542e5c1
> 

For some reason I cannot fetch your git due to "error: RPC failed ..." 
which I think is a timeout. I seem to have this problem recently 
whenever a linux.git clone has branches based on linux-next.git . Maybe 
a git config issue for me...

> Branch:
> https://github.com/krzk/linux/commits/n/qcom-ufs-opp-cleanups-v2
> 
> However this does not solve the problem. The SHT has "module" which gets
> incremented/decremented. Exactly like in case of other drivers
> (driver->owner).

Ah, I missed that this could be a problem. So we have this member to 
stop the SCSI host driver being removed when we have disks mounted, etc.

But isn't scsi_host_template.module just a pointer to the local driver 
module data (and that data gets incremented/decremented)? I am looking 
at the THIS_MODULE definition in export.h:

extern stuct module __this_module;
#define THIS_MODULE(&__this_module)

However I do see scsi_device_dev_release(), which does:

sdp->host->hostt->module = NULL

I am not sure how necessary that really is. I would need to check further.

Did you see if there other places which set hostt->module dynamically?

> 
> I started moving the SHT->module to a new field scsi_host->owner and
> trying to use the parent's driver (so PCI, USB) owner.
> I am not sure if it is correct approach, so before implementing such big
> change affecting multiple subsystems (USB, ATA, SCSI) - can you share
> ideas/opinion?
> 
> The Work-in-Progress looks like this (last commit):
> https://github.com/krzk/linux/commit/17609caecd53df20f631703ea084a70e7735b5d7

Thanks,
John
Krzysztof Kozlowski May 9, 2022, 1:20 p.m. UTC | #12
On 09/05/2022 13:28, John Garry wrote:
> 
> For some reason I cannot fetch your git due to "error: RPC failed ..." 
> which I think is a timeout. I seem to have this problem recently 
> whenever a linux.git clone has branches based on linux-next.git . Maybe 
> a git config issue for me...

Just to be sure - the link was not a git remote, but direct link for a
web browser. The repo is:
https://github.com/krzk/linux.git
branch: n/qcom-ufs-opp-cleanups-v2

>> However this does not solve the problem. The SHT has "module" which gets
>> incremented/decremented. Exactly like in case of other drivers
>> (driver->owner).
> 
> Ah, I missed that this could be a problem. So we have this member to 
> stop the SCSI host driver being removed when we have disks mounted, etc.
> 
> But isn't scsi_host_template.module just a pointer to the local driver 
> module data (and that data gets incremented/decremented)? I am looking 
> at the THIS_MODULE definition in export.h:

Yes, it is just a pointer, just like 'struct device_driver.owner' is.

> 
> extern stuct module __this_module;
> #define THIS_MODULE(&__this_module)
> 
> However I do see scsi_device_dev_release(), which does:
> 
> sdp->host->hostt->module = NULL
> 
> I am not sure how necessary that really is. I would need to check further.

> 
> Did you see if there other places which set hostt->module dynamically?

I think all SHT set it statically. Then it is being dynamically
incremented when needed and in scsi_device_dev_release() is being
nullified (as you mentioned above).

I guess this SHT->module is actually duplicating what most of drivers
(e.g. PCI, platform, USB) are doing. If I understand correctly, the
Scsi_Host is instantiated by the probe of a driver (pci_driver,
virtio_driver etc), therefore the SHT->module could be simply stored in
Scsi_Host.


>>
>> I started moving the SHT->module to a new field scsi_host->owner and
>> trying to use the parent's driver (so PCI, USB) owner.
>> I am not sure if it is correct approach, so before implementing such big
>> change affecting multiple subsystems (USB, ATA, SCSI) - can you share
>> ideas/opinion?
>>
>> The Work-in-Progress looks like this (last commit):
>> https://github.com/krzk/linux/commit/17609caecd53df20f631703ea084a70e7735b5d7



Best regards,
Krzysztof
John Garry May 9, 2022, 2:50 p.m. UTC | #13
On 09/05/2022 14:20, Krzysztof Kozlowski wrote:
> On 09/05/2022 13:28, John Garry wrote:
>>
>> For some reason I cannot fetch your git due to "error: RPC failed ..."
>> which I think is a timeout. I seem to have this problem recently
>> whenever a linux.git clone has branches based on linux-next.git . Maybe
>> a git config issue for me...
> 
> Just to be sure - the link was not a git remote, but direct link for a
> web browser. The repo is:
> https://github.com/krzk/linux.git
> branch: n/qcom-ufs-opp-cleanups-v2
> 

OK, I'll double check. Regardless I'll sort my own IT issues :)

>>> However this does not solve the problem. The SHT has "module" which gets
>>> incremented/decremented. Exactly like in case of other drivers
>>> (driver->owner).
>>
>> Ah, I missed that this could be a problem. So we have this member to
>> stop the SCSI host driver being removed when we have disks mounted, etc.
>>
>> But isn't scsi_host_template.module just a pointer to the local driver
>> module data (and that data gets incremented/decremented)? I am looking
>> at the THIS_MODULE definition in export.h:
> 
> Yes, it is just a pointer, just like 'struct device_driver.owner' is.
> 
>>
>> extern stuct module __this_module;
>> #define THIS_MODULE(&__this_module)
>>
>> However I do see scsi_device_dev_release(), which does:
>>
>> sdp->host->hostt->module = NULL
>>
>> I am not sure how necessary that really is. I would need to check further.
> 
>>
>> Did you see if there other places which set hostt->module dynamically?
> 
> I think all SHT set it statically. 

Incidentally I did notice that the ata ahci driver does not set sht->module.

> Then it is being dynamically
> incremented when needed and in scsi_device_dev_release() is being
> nullified (as you mentioned above).
> 
> I guess this SHT->module is actually duplicating what most of drivers
> (e.g. PCI, platform, USB) are doing. If I understand correctly, the
> Scsi_Host is instantiated by the probe of a driver (pci_driver,
> virtio_driver etc), therefore the SHT->module could be simply stored in
> Scsi_Host.

If you check scsi_device_dev_release(), we try to do a 'get' - if it 
fails, then we nullify hostt->module. I think that is important as then 
we call execute_in_process_context(), whose worker does the 'put'. 
However, the 'put' will get upset if the refcnt was 0, which it would be 
if the earlier 'get' fails - hence the nullify is to avoid that 
possibility. So whatever you do needs to handle that. Details are in 
f2b85040

Thanks,
john
Christoph Hellwig May 11, 2022, 8:31 a.m. UTC | #14
On Mon, May 09, 2022 at 03:50:33PM +0100, John Garry wrote:
> If you check scsi_device_dev_release(), we try to do a 'get' - if it fails,
> then we nullify hostt->module. I think that is important as then we call
> execute_in_process_context(), whose worker does the 'put'. However, the
> 'put' will get upset if the refcnt was 0, which it would be if the earlier
> 'get' fails - hence the nullify is to avoid that possibility. So whatever
> you do needs to handle that. Details are in f2b85040

Yikes, that code is completely and utterly buggy and does not account
for all the cases why try_module_get can fail.  I think we always have
a reference here and could use __module_get, but what we have is
certainly unsafe and a good reason why the host template should be
constifyed.
diff mbox series

Patch

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index f69b77cbf538..65616a01761a 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -209,7 +209,7 @@  EXPORT_SYMBOL(scsi_remove_host);
 int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 			   struct device *dma_dev)
 {
-	struct scsi_host_template *sht = shost->hostt;
+	const struct scsi_host_template *sht = shost->hostt;
 	int error = -EINVAL;
 
 	shost_printk(KERN_INFO, shost, "%s\n",
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index cdaca13ac1f1..0bca4108aae2 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -58,7 +58,7 @@ 
 #define HOST_RESET_SETTLE_TIME  (10)
 
 static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
-static enum scsi_disposition scsi_try_to_abort_cmd(struct scsi_host_template *,
+static enum scsi_disposition scsi_try_to_abort_cmd(const struct scsi_host_template *,
 						   struct scsi_cmnd *);
 
 void scsi_eh_wakeup(struct Scsi_Host *shost)
@@ -692,7 +692,7 @@  EXPORT_SYMBOL_GPL(scsi_check_sense);
 
 static void scsi_handle_queue_ramp_up(struct scsi_device *sdev)
 {
-	struct scsi_host_template *sht = sdev->host->hostt;
+	const struct scsi_host_template *sht = sdev->host->hostt;
 	struct scsi_device *tmp_sdev;
 
 	if (!sht->track_queue_depth ||
@@ -724,7 +724,7 @@  static void scsi_handle_queue_ramp_up(struct scsi_device *sdev)
 
 static void scsi_handle_queue_full(struct scsi_device *sdev)
 {
-	struct scsi_host_template *sht = sdev->host->hostt;
+	const struct scsi_host_template *sht = sdev->host->hostt;
 	struct scsi_device *tmp_sdev;
 
 	if (!sht->track_queue_depth)
@@ -833,7 +833,7 @@  static enum scsi_disposition scsi_try_host_reset(struct scsi_cmnd *scmd)
 	unsigned long flags;
 	enum scsi_disposition rtn;
 	struct Scsi_Host *host = scmd->device->host;
-	struct scsi_host_template *hostt = host->hostt;
+	const struct scsi_host_template *hostt = host->hostt;
 
 	SCSI_LOG_ERROR_RECOVERY(3,
 		shost_printk(KERN_INFO, host, "Snd Host RST\n"));
@@ -863,7 +863,7 @@  static enum scsi_disposition scsi_try_bus_reset(struct scsi_cmnd *scmd)
 	unsigned long flags;
 	enum scsi_disposition rtn;
 	struct Scsi_Host *host = scmd->device->host;
-	struct scsi_host_template *hostt = host->hostt;
+	const struct scsi_host_template *hostt = host->hostt;
 
 	SCSI_LOG_ERROR_RECOVERY(3, scmd_printk(KERN_INFO, scmd,
 		"%s: Snd Bus RST\n", __func__));
@@ -905,7 +905,7 @@  static enum scsi_disposition scsi_try_target_reset(struct scsi_cmnd *scmd)
 	unsigned long flags;
 	enum scsi_disposition rtn;
 	struct Scsi_Host *host = scmd->device->host;
-	struct scsi_host_template *hostt = host->hostt;
+	const struct scsi_host_template *hostt = host->hostt;
 
 	if (!hostt->eh_target_reset_handler)
 		return FAILED;
@@ -934,7 +934,7 @@  static enum scsi_disposition scsi_try_target_reset(struct scsi_cmnd *scmd)
 static enum scsi_disposition scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
 {
 	enum scsi_disposition rtn;
-	struct scsi_host_template *hostt = scmd->device->host->hostt;
+	const struct scsi_host_template *hostt = scmd->device->host->hostt;
 
 	if (!hostt->eh_device_reset_handler)
 		return FAILED;
@@ -963,7 +963,8 @@  static enum scsi_disposition scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
  *    link down on FibreChannel)
  */
 static enum scsi_disposition
-scsi_try_to_abort_cmd(struct scsi_host_template *hostt, struct scsi_cmnd *scmd)
+scsi_try_to_abort_cmd(const struct scsi_host_template *hostt,
+		      struct scsi_cmnd *scmd)
 {
 	if (!hostt->eh_abort_handler)
 		return FAILED;
diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 95aee1ad1383..6f023a2f951b 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -137,7 +137,7 @@  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 proc_dir_entry *p;
 	char name[10];
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index dc6872e352bd..cdc6e1ab8ce7 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -296,7 +296,7 @@  store_host_reset(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t count)
 {
 	struct Scsi_Host *shost = class_to_shost(dev);
-	struct scsi_host_template *sht = shost->hostt;
+	const struct scsi_host_template *sht = shost->hostt;
 	int ret = -EINVAL;
 	int type;
 
@@ -1017,7 +1017,7 @@  sdev_store_queue_depth(struct device *dev, struct device_attribute *attr,
 {
 	int depth, retval;
 	struct scsi_device *sdev = to_scsi_device(dev);
-	struct scsi_host_template *sht = sdev->host->hostt;
+	const struct scsi_host_template *sht = sdev->host->hostt;
 
 	if (!sht->change_queue_depth)
 		return -EINVAL;
@@ -1584,7 +1584,7 @@  void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 {
 	unsigned long flags;
 	struct Scsi_Host *shost = sdev->host;
-	struct scsi_host_template *hostt = shost->hostt;
+	const struct scsi_host_template *hostt = shost->hostt;
 	struct scsi_target  *starget = sdev->sdev_target;
 
 	device_initialize(&sdev->sdev_gendev);