| Message ID | 20220408103027.311624-1-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/4] scsi: core: constify pointer to scsi_host_template | expand |
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? Thanks, john > 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(-) > > 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
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. 2. The more const variables, the easier function contents and its impact is to understand. This is actually the biggest benefit when dealing with code touching different structures. In general, constifying is a common practice everywhere in the kernel. Best regards, Krzysztof
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. To me this seems better, but I think that some drivers might modify their scsi_host_template (so not possible) > 2. The more const variables, the easier function contents and its impact > is to understand. This is actually the biggest benefit when dealing with > code touching different structures. > > In general, constifying is a common practice everywhere in the kernel. Thanks, John
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. > > 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. -Ewan > > > 2. The more const variables, the easier function contents and its impact > > is to understand. This is actually the biggest benefit when dealing with > > code touching different structures. > > > > In general, constifying is a common practice everywhere in the kernel. > > Thanks, > John >
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(-)