| 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 |
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
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.
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
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
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
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.
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
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.
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
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
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
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
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
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 --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);
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(-)