* [PATCH] scsi host/scsi device ref count cleanup 0/4 @ 2003-07-08 22:24 Mike Anderson 2003-07-08 22:25 ` [PATCH] scsi host/scsi device ref count cleanup 1/4 Mike Anderson 0 siblings, 1 reply; 10+ messages in thread From: Mike Anderson @ 2003-07-08 22:24 UTC (permalink / raw) To: linux-scsi This patch set is against scsi-misc-2.5 This set contains the following patches. stern_present - refresh of Alan's previous patch. shost_present - reduce users of shost_present. shost_state - Add some state flags and reorder cleanup. sdev_state - Add some state flags and reorder cleanup. I still have a couple of outstanding issues - sd_shutdown errors on surprise removal. - scsi_remove_single_device / scsi_remove_device cleanup around the usage of access_count. I tested the surprise removal using the scsi_debug add_host attribute. I created two tgts on each host. One target I created a file system and the other target I ran dd's. Post "-1" into add_host I did a umount -f on the mounted file system while monitoring ref counts. I also checked the reboot notifier list on shutdown to ensure it appeared correct. -andmike -- Michael Anderson andmike@us.ibm.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] scsi host/scsi device ref count cleanup 1/4 2003-07-08 22:24 [PATCH] scsi host/scsi device ref count cleanup 0/4 Mike Anderson @ 2003-07-08 22:25 ` Mike Anderson 2003-07-08 22:26 ` [PATCH] scsi host/scsi device ref count cleanup 2/4 Mike Anderson 0 siblings, 1 reply; 10+ messages in thread From: Mike Anderson @ 2003-07-08 22:25 UTC (permalink / raw) To: linux-scsi -andmike -- Michael Anderson andmike@us.ibm.com DESC This is a refresh of a patch by Alan Stern to correct the removal of scsi procfs host enteries http://marc.theaimsgroup.com/?l=linux-scsi&m=105545175600506&w=2 EDESC drivers/scsi/hosts.c | 4 +-- drivers/scsi/scsi_priv.h | 4 +++ drivers/scsi/scsi_proc.c | 49 ++++++++++++++++++++++++++++++----------------- 3 files changed, 38 insertions(+), 19 deletions(-) diff -puN drivers/scsi/hosts.c~stern_present drivers/scsi/hosts.c --- remove-scsi-misc-2.5/drivers/scsi/hosts.c~stern_present Tue Jul 8 14:19:47 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/hosts.c Tue Jul 8 14:19:47 2003 @@ -112,7 +112,7 @@ void scsi_free_shost(struct Scsi_Host *s shost->eh_notify = NULL; } - shost->hostt->present--; + scsi_proc_hostdir_rm(shost->hostt); scsi_destroy_command_freelist(shost); kfree(shost); } @@ -219,7 +219,7 @@ struct Scsi_Host *scsi_host_alloc(struct kernel_thread((int (*)(void *))scsi_error_handler, shost, 0); wait_for_completion(&complete); shost->eh_notify = NULL; - shost->hostt->present++; + scsi_proc_hostdir_add(shost->hostt); return shost; fail: kfree(shost); diff -puN drivers/scsi/scsi_priv.h~stern_present drivers/scsi/scsi_priv.h --- remove-scsi-misc-2.5/drivers/scsi/scsi_priv.h~stern_present Tue Jul 8 14:19:47 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_priv.h Tue Jul 8 14:19:47 2003 @@ -90,11 +90,15 @@ extern void scsi_exit_queue(void); /* scsi_proc.c */ #ifdef CONFIG_PROC_FS +extern void scsi_proc_hostdir_add(struct scsi_host_template *); +extern void scsi_proc_hostdir_rm(struct scsi_host_template *); extern void scsi_proc_host_add(struct Scsi_Host *); extern void scsi_proc_host_rm(struct Scsi_Host *); extern int scsi_init_procfs(void); extern void scsi_exit_procfs(void); #else +# define scsi_proc_hostdir_add(sht) do { } while (0) +# define scsi_proc_hostdir_rm(sht) do { } while (0) # define scsi_proc_host_add(shost) do { } while (0) # define scsi_proc_host_rm(shost) do { } while (0) # define scsi_init_procfs() (0) diff -puN drivers/scsi/scsi_proc.c~stern_present drivers/scsi/scsi_proc.c --- remove-scsi-misc-2.5/drivers/scsi/scsi_proc.c~stern_present Tue Jul 8 14:19:47 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_proc.c Tue Jul 8 14:19:47 2003 @@ -41,6 +41,8 @@ struct proc_dir_entry *proc_scsi; EXPORT_SYMBOL(proc_scsi); +/* Protect sht->present and sht->proc_dir */ +static DECLARE_MUTEX(global_host_template_sem); static int proc_scsi_read(char *buffer, char **start, off_t offset, int length, int *eof, void *data) @@ -77,16 +79,10 @@ out: return ret; } -void scsi_proc_host_add(struct Scsi_Host *shost) +void scsi_proc_hostdir_add(struct scsi_host_template *sht) { - struct scsi_host_template *sht = shost->hostt; - struct proc_dir_entry *p; - char name[10]; - - if (!sht->proc_info) - return; - - if (!sht->proc_dir) { + down(&global_host_template_sem); + if (!sht->present++) { sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi); if (!sht->proc_dir) { printk(KERN_ERR "%s: proc_mkdir failed for %s\n", @@ -95,6 +91,27 @@ void scsi_proc_host_add(struct Scsi_Host } sht->proc_dir->owner = sht->module; } + up(&global_host_template_sem); +} + +void scsi_proc_hostdir_rm(struct scsi_host_template *sht) +{ + down(&global_host_template_sem); + if (!--sht->present && sht->proc_dir) { + remove_proc_entry(sht->proc_name, proc_scsi); + sht->proc_dir = NULL; + } + up(&global_host_template_sem); +} + +void scsi_proc_host_add(struct Scsi_Host *shost) +{ + struct scsi_host_template *sht = shost->hostt; + struct proc_dir_entry *p; + char name[10]; + + if (!sht->proc_dir) + return; sprintf(name,"%d", shost->host_no); p = create_proc_read_entry(name, S_IFREG | S_IRUGO | S_IWUSR, @@ -107,20 +124,18 @@ void scsi_proc_host_add(struct Scsi_Host } p->write_proc = proc_scsi_write_proc; - p->owner = shost->hostt->module; + p->owner = sht->module; } void scsi_proc_host_rm(struct Scsi_Host *shost) { - struct scsi_host_template *sht = shost->hostt; char name[10]; - if (sht->proc_info) { - sprintf(name,"%d", shost->host_no); - remove_proc_entry(name, sht->proc_dir); - if (!sht->present) - remove_proc_entry(sht->proc_name, proc_scsi); - } + if (!shost->hostt->proc_dir) + return; + + sprintf(name,"%d", shost->host_no); + remove_proc_entry(name, shost->hostt->proc_dir); } static int proc_print_scsidevice(struct device *dev, void *data) _ ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] scsi host/scsi device ref count cleanup 2/4 2003-07-08 22:25 ` [PATCH] scsi host/scsi device ref count cleanup 1/4 Mike Anderson @ 2003-07-08 22:26 ` Mike Anderson 2003-07-08 22:27 ` [PATCH] scsi host/scsi device ref count cleanup 3/4 Mike Anderson 2003-07-09 7:39 ` [PATCH] scsi host/scsi device ref count cleanup 2/4 Christoph Hellwig 0 siblings, 2 replies; 10+ messages in thread From: Mike Anderson @ 2003-07-08 22:26 UTC (permalink / raw) To: linux-scsi -andmike -- Michael Anderson andmike@us.ibm.com DESC This patch reduces the user of scsi_host present. aic79xx_osm and aic7xxx_osm are 2.4 code. ioctl_probe has side effect of needing present value. EDESC drivers/scsi/NCR53c406a.c | 5 +++-- drivers/scsi/aacraid/linit.c | 1 - drivers/scsi/scsi_module.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff -puN drivers/scsi/aacraid/linit.c~shost_present drivers/scsi/aacraid/linit.c --- remove-scsi-misc-2.5/drivers/scsi/aacraid/linit.c~shost_present Tue Jul 8 14:19:50 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/aacraid/linit.c Tue Jul 8 14:19:50 2003 @@ -295,7 +295,6 @@ static int aac_detect(Scsi_Host_Template printk(KERN_WARNING "aacraid: unable to register \"aac\" device.\n"); } - template->present = aac_count; /* # of cards of this type found */ return aac_count; } diff -puN drivers/scsi/NCR53c406a.c~shost_present drivers/scsi/NCR53c406a.c --- remove-scsi-misc-2.5/drivers/scsi/NCR53c406a.c~shost_present Tue Jul 8 14:19:50 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/NCR53c406a.c Tue Jul 8 14:19:50 2003 @@ -450,6 +450,7 @@ static __inline__ int NCR53c406a_pio_wri static int __init NCR53c406a_detect(Scsi_Host_Template * tpnt) { + int present = 0; struct Scsi_Host *shpnt = NULL; #ifndef PORT_BASE int i; @@ -522,7 +523,7 @@ static int __init NCR53c406a_detect(Scsi DEB(printk("NCR53c406a: using port_base 0x%x\n", port_base)); - tpnt->present = 1; + present = 1; tpnt->proc_name = "NCR53c406a"; shpnt = scsi_register(tpnt, 0); @@ -576,7 +577,7 @@ static int __init NCR53c406a_detect(Scsi sprintf(info_msg, "NCR53c406a at 0x%x, IRQ %d, %s PIO mode.", port_base, irq_level, fast_pio ? "fast" : "slow"); #endif - return (tpnt->present); + return (present); #if USE_DMA err_free_irq: diff -puN drivers/scsi/scsi_module.c~shost_present drivers/scsi/scsi_module.c --- remove-scsi-misc-2.5/drivers/scsi/scsi_module.c~shost_present Tue Jul 8 14:19:50 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_module.c Tue Jul 8 14:19:50 2003 @@ -33,7 +33,7 @@ static int __init init_this_scsi_driver( INIT_LIST_HEAD(&sht->legacy_hosts); sht->detect(sht); - if (!sht->present) + if (list_empty(&sht->legacy_hosts)) return -ENODEV; list_for_each_entry(shost, &sht->legacy_hosts, sht_legacy_list) { _ ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] scsi host/scsi device ref count cleanup 3/4 2003-07-08 22:26 ` [PATCH] scsi host/scsi device ref count cleanup 2/4 Mike Anderson @ 2003-07-08 22:27 ` Mike Anderson 2003-07-08 22:27 ` [PATCH] scsi host/scsi device ref count cleanup 4/4 Mike Anderson 2003-07-09 7:53 ` [PATCH] scsi host/scsi device ref count cleanup 3/4 Christoph Hellwig 2003-07-09 7:39 ` [PATCH] scsi host/scsi device ref count cleanup 2/4 Christoph Hellwig 1 sibling, 2 replies; 10+ messages in thread From: Mike Anderson @ 2003-07-08 22:27 UTC (permalink / raw) To: linux-scsi -andmike -- Michael Anderson andmike@us.ibm.com DESC This is a cleanup patch for scsi_host removal. - Addition of a host_flags member to the scsi_host strucuture to contain "state" of the host instance. - Added SHOST_ADD, SHOST_DEL, SHOST_CANCEL, SHOST_RECOVERY states for a scsi host - Addtion / rename of a new function scsi_host_cancel to cancel IOs in flight. - Usage of the host_flags flags member to stop scsi_host_get's and calls to LLDD queucommand under certain states. - Reordered some of the scsi host sysfs unregistration. EDESC drivers/scsi/hosts.c | 62 +++++++++++++++++++++++++++------------------ drivers/scsi/scsi.c | 31 +++++++++++++++------- drivers/scsi/scsi_debug.c | 5 --- drivers/scsi/scsi_error.c | 7 ++--- drivers/scsi/scsi_lib.c | 5 ++- drivers/scsi/scsi_proc.c | 12 ++++---- drivers/scsi/scsi_syms.c | 2 - drivers/scsi/scsi_sysfs.c | 44 ++++++++++++++++++++++--------- drivers/scsi/sg.c | 3 +- drivers/usb/storage/usb.c | 7 ----- include/scsi/scsi_device.h | 2 - include/scsi/scsi_host.h | 16 +++++++++-- 12 files changed, 122 insertions(+), 74 deletions(-) diff -puN drivers/scsi/hosts.c~shost_state drivers/scsi/hosts.c --- remove-scsi-misc-2.5/drivers/scsi/hosts.c~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/hosts.c Tue Jul 8 14:19:52 2003 @@ -39,30 +39,35 @@ static int scsi_host_next_hn; /* host_no for next new host */ + /** - * scsi_remove_host - check a scsi host for release and release - * @shost: a pointer to a scsi host to release - * - * Return value: - * 0 on Success / 1 on Failure + * scsi_host_cancel - cancel outstanding IO to this host + * @shost: pointer to struct Scsi_Host + * recovery: recovery requested to run. **/ -int scsi_remove_host(struct Scsi_Host *shost) +void scsi_host_cancel(struct Scsi_Host *shost, int recovery) { - struct scsi_device *sdev; + unsigned long flags; - /* - * FIXME Do ref counting. We force all of the devices offline to - * help prevent race conditions where other hosts/processors could - * try and get in and queue a command. - */ - list_for_each_entry(sdev, &shost->my_devices, siblings) - sdev->online = FALSE; + spin_lock_irqsave(shost->host_lock, flags); + __set_bit(SHOST_CANCEL, &shost->host_flags); + spin_unlock_irqrestore(shost->host_lock, flags); + device_for_each_child(&shost->host_gendev, &recovery, + scsi_device_cancel); + wait_event(shost->host_wait, (!test_bit(SHOST_RECOVERY, + &shost->host_flags))); +} +/** + * scsi_remove_host - remove a scsi host + * @shost: a pointer to a scsi host to remove + **/ +void scsi_remove_host(struct Scsi_Host *shost) +{ + scsi_host_cancel(shost, 0); scsi_proc_host_rm(shost); scsi_forget_host(shost); scsi_sysfs_remove_host(shost); - - return 0; } /** @@ -93,7 +98,7 @@ int scsi_add_host(struct Scsi_Host *shos scsi_proc_host_add(shost); scsi_scan_host(shost); } - + return error; } @@ -259,21 +264,21 @@ struct Scsi_Host *scsi_host_lookup(unsig { struct class *class = class_get(&shost_class); struct class_device *cdev; - struct Scsi_Host *shost = NULL, *p; + struct Scsi_Host *shost = ERR_PTR(-ENXIO), *p; if (class) { down_read(&class->subsys.rwsem); list_for_each_entry(cdev, &class->children, node) { p = class_to_shost(cdev); if (p->host_no == hostnum) { - scsi_host_get(p); - shost = p; + shost = scsi_host_get(p); break; } } up_read(&class->subsys.rwsem); } + class_put(&shost_class); return shost; } @@ -281,10 +286,20 @@ struct Scsi_Host *scsi_host_lookup(unsig * *scsi_host_get - inc a Scsi_Host ref count * @shost: Pointer to Scsi_Host to inc. **/ -void scsi_host_get(struct Scsi_Host *shost) +struct Scsi_Host * scsi_host_get(struct Scsi_Host *shost) { - get_device(&shost->host_gendev); - class_device_get(&shost->class_dev); + struct Scsi_Host *res_shost = shost; + + if (res_shost) { + if (!test_bit(SHOST_DEL, &shost->host_flags)) { + if (!get_device(&res_shost->host_gendev)) + res_shost = ERR_PTR(-ENXIO); + } else { + res_shost = ERR_PTR(-ENXIO); + } + } + + return res_shost; } /** @@ -293,6 +308,5 @@ void scsi_host_get(struct Scsi_Host *sho **/ void scsi_host_put(struct Scsi_Host *shost) { - class_device_put(&shost->class_dev); put_device(&shost->host_gendev); } diff -puN drivers/scsi/scsi.c~shost_state drivers/scsi/scsi.c --- remove-scsi-misc-2.5/drivers/scsi/scsi.c~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi.c Tue Jul 8 14:19:52 2003 @@ -370,7 +370,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd * struct Scsi_Host *host = cmd->device->host; unsigned long flags = 0; unsigned long timeout; - int rtn = 1; + int rtn = 0; /* Assign a unique nonzero serial_number. */ /* XXX(hch): this is racy */ @@ -444,7 +444,12 @@ int scsi_dispatch_cmd(struct scsi_cmnd * host->hostt->queuecommand)); spin_lock_irqsave(host->host_lock, flags); - rtn = host->hostt->queuecommand(cmd, scsi_done); + if (unlikely(test_bit(SHOST_CANCEL, &host->host_flags))) { + cmd->result = (DID_NO_CONNECT << 16); + scsi_done(cmd); + } else { + rtn = host->hostt->queuecommand(cmd, scsi_done); + } spin_unlock_irqrestore(host->host_lock, flags); if (rtn) { scsi_queue_insert(cmd, @@ -902,17 +907,19 @@ void scsi_device_put(struct scsi_device } /** - * scsi_set_device_offline - set scsi_device offline - * @sdev: pointer to struct scsi_device to offline. + * scsi_device_cancel - cancel outstanding IO to this device + * @sdev: pointer to struct scsi_device + * @data: pointer to cancel value. * - * Locks: host_lock held on entry. **/ -void scsi_set_device_offline(struct scsi_device *sdev) +int scsi_device_cancel(struct device *dev, void *data) { struct scsi_cmnd *scmd; LIST_HEAD(active_list); struct list_head *lh, *lh_sf; unsigned long flags; + struct scsi_device *sdev = to_scsi_device(dev); + unsigned int recovery = *(unsigned int *)data; sdev->online = 0; @@ -934,11 +941,17 @@ void scsi_set_device_offline(struct scsi if (!list_empty(&active_list)) { list_for_each_safe(lh, lh_sf, &active_list) { scmd = list_entry(lh, struct scsi_cmnd, eh_entry); - scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD); + list_del_init(lh); + if (recovery) { + scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD); + } else { + scmd->result = (DID_ABORT << 16); + scsi_finish_command(scmd); + } } - } else { - /* FIXME: Send online state change hotplug event */ } + + return 0; } MODULE_DESCRIPTION("SCSI core"); diff -puN drivers/scsi/scsi_debug.c~shost_state drivers/scsi/scsi_debug.c --- remove-scsi-misc-2.5/drivers/scsi/scsi_debug.c~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_debug.c Tue Jul 8 14:19:52 2003 @@ -1721,10 +1721,7 @@ static int sdebug_driver_remove(struct d return -ENODEV; } - if (scsi_remove_host(sdbg_host->shost)) { - printk(KERN_ERR "%s: scsi_remove_host failed\n", __FUNCTION__); - return -EBUSY; - } + scsi_remove_host(sdbg_host->shost); list_for_each_safe(lh, lh_sf, &sdbg_host->dev_info_list) { sdbg_devinfo = list_entry(lh, struct sdebug_dev_info, diff -puN drivers/scsi/scsi_error.c~shost_state drivers/scsi/scsi_error.c --- remove-scsi-misc-2.5/drivers/scsi/scsi_error.c~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_error.c Tue Jul 8 14:19:52 2003 @@ -84,7 +84,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *s */ scmd->serial_number_at_timeout = scmd->serial_number; list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q); - shost->in_recovery = 1; + set_bit(SHOST_RECOVERY, &shost->host_flags); shost->host_failed++; scsi_eh_wakeup(shost); spin_unlock_irqrestore(shost->host_lock, flags); @@ -187,7 +187,7 @@ void scsi_times_out(struct scsi_cmnd *sc **/ int scsi_block_when_processing_errors(struct scsi_device *sdev) { - wait_event(sdev->host->host_wait, (sdev->host->in_recovery == 0)); + wait_event(sdev->host->host_wait, (!test_bit(SHOST_RECOVERY, &sdev->host->host_flags))); SCSI_LOG_ERROR_RECOVERY(5, printk("%s: rtn: %d\n", __FUNCTION__, sdev->online)); @@ -1389,7 +1389,7 @@ static void scsi_restart_operations(stru SCSI_LOG_ERROR_RECOVERY(3, printk("%s: waking up host to restart\n", __FUNCTION__)); - shost->in_recovery = 0; + clear_bit(SHOST_RECOVERY, &shost->host_flags); wake_up(&shost->host_wait); @@ -1599,7 +1599,6 @@ void scsi_error_handler(void *data) * that's fine. If the user sent a signal to this thing, we are * potentially in real danger. */ - shost->in_recovery = 0; shost->eh_active = 0; shost->ehandler = NULL; diff -puN drivers/scsi/scsi_lib.c~shost_state drivers/scsi/scsi_lib.c --- remove-scsi-misc-2.5/drivers/scsi/scsi_lib.c~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_lib.c Tue Jul 8 14:19:52 2003 @@ -318,7 +318,8 @@ void scsi_device_unbusy(struct scsi_devi spin_lock_irqsave(shost->host_lock, flags); shost->host_busy--; - if (unlikely(shost->in_recovery && shost->host_failed)) + if (unlikely(test_bit(SHOST_RECOVERY, &shost->host_flags) && + shost->host_failed)) scsi_eh_wakeup(shost); spin_unlock(shost->host_lock); spin_lock(&sdev->sdev_lock); @@ -1080,7 +1081,7 @@ static inline int scsi_host_queue_ready( struct Scsi_Host *shost, struct scsi_device *sdev) { - if (shost->in_recovery) + if (test_bit(SHOST_RECOVERY, &shost->host_flags)) return 0; if (shost->host_busy == 0 && shost->host_blocked) { /* diff -puN drivers/scsi/scsi_proc.c~shost_state drivers/scsi/scsi_proc.c --- remove-scsi-misc-2.5/drivers/scsi/scsi_proc.c~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_proc.c Tue Jul 8 14:19:52 2003 @@ -190,11 +190,11 @@ static int scsi_add_single_device(uint h { struct Scsi_Host *shost; struct scsi_device *sdev; - int error = -ENODEV; + int error = -ENXIO; shost = scsi_host_lookup(host); - if (!shost) - return -ENODEV; + if (IS_ERR(shost)) + return PTR_ERR(shost); if (!scsi_find_device(shost, channel, id, lun)) { sdev = scsi_add_device(shost, channel, id, lun); @@ -212,11 +212,11 @@ static int scsi_remove_single_device(uin { struct scsi_device *sdev; struct Scsi_Host *shost; - int error = -ENODEV; + int error = -ENXIO; shost = scsi_host_lookup(host); - if (!shost) - return -ENODEV; + if (IS_ERR(shost)) + return PTR_ERR(shost); sdev = scsi_find_device(shost, channel, id, lun); if (!sdev) goto out; diff -puN drivers/scsi/scsi_syms.c~shost_state drivers/scsi/scsi_syms.c --- remove-scsi-misc-2.5/drivers/scsi/scsi_syms.c~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_syms.c Tue Jul 8 14:19:52 2003 @@ -85,7 +85,7 @@ EXPORT_SYMBOL(scsi_device_get); EXPORT_SYMBOL(scsi_device_put); EXPORT_SYMBOL(scsi_add_device); EXPORT_SYMBOL(scsi_remove_device); -EXPORT_SYMBOL(scsi_set_device_offline); +EXPORT_SYMBOL(scsi_device_cancel); EXPORT_SYMBOL(__scsi_mode_sense); EXPORT_SYMBOL(scsi_mode_sense); diff -puN drivers/scsi/scsi_sysfs.c~shost_state drivers/scsi/scsi_sysfs.c --- remove-scsi-misc-2.5/drivers/scsi/scsi_sysfs.c~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_sysfs.c Tue Jul 8 14:19:52 2003 @@ -44,6 +44,7 @@ shost_rd_attr(host_busy, "%hu\n"); shost_rd_attr(cmd_per_lun, "%hd\n"); shost_rd_attr(sg_tablesize, "%hu\n"); shost_rd_attr(unchecked_isa_dma, "%d\n"); +shost_rd_attr(host_flags, "%lx\n"); struct class_device_attribute *scsi_sysfs_shost_attrs[] = { &class_device_attr_unique_id, @@ -51,11 +52,30 @@ struct class_device_attribute *scsi_sysf &class_device_attr_cmd_per_lun, &class_device_attr_sg_tablesize, &class_device_attr_unchecked_isa_dma, + &class_device_attr_host_flags, NULL }; +static void scsi_host_cls_release(struct class_device *class_dev) +{ + struct Scsi_Host *shost; + + shost = class_to_shost(class_dev); + put_device(&shost->host_gendev); +} + +static void scsi_host_dev_release(struct device *dev) +{ + struct Scsi_Host *shost; + + device_del(dev); + shost = dev_to_shost(dev); + scsi_free_shost(shost); +} + struct class shost_class = { .name = "scsi_host", + .release = scsi_host_cls_release, }; static struct class sdev_class = { @@ -302,16 +322,6 @@ int scsi_register_interface(struct class return class_interface_register(intf); } -static void scsi_host_release(struct device *dev) -{ - struct Scsi_Host *shost; - - shost = dev_to_shost(dev); - if (!shost) - return; - - scsi_free_shost(shost); -} void scsi_sysfs_init_host(struct Scsi_Host *shost) { @@ -320,13 +330,14 @@ void scsi_sysfs_init_host(struct Scsi_Ho shost->host_no); snprintf(shost->host_gendev.name, DEVICE_NAME_SIZE, "%s", shost->hostt->proc_name); - shost->host_gendev.release = scsi_host_release; + shost->host_gendev.release = scsi_host_dev_release; class_device_initialize(&shost->class_dev); shost->class_dev.dev = &shost->host_gendev; shost->class_dev.class = &shost_class; snprintf(shost->class_dev.class_id, BUS_ID_SIZE, "host%d", shost->host_no); + get_device(&shost->host_gendev); } /** @@ -355,6 +366,8 @@ int scsi_sysfs_add_host(struct Scsi_Host if (error) goto clean_class; + set_bit(SHOST_ADD, &shost->host_flags); + return error; clean_class: @@ -371,8 +384,13 @@ clean_device: **/ void scsi_sysfs_remove_host(struct Scsi_Host *shost) { - class_device_del(&shost->class_dev); - device_del(&shost->host_gendev); + unsigned long flags; + + spin_lock_irqsave(shost->host_lock, flags); + __set_bit(SHOST_DEL, &shost->host_flags); + spin_unlock_irqrestore(shost->host_lock, flags); + + class_device_unregister(&shost->class_dev); } /** scsi_sysfs_modify_shost_attribute - modify or add a host class attribute diff -puN drivers/scsi/sg.c~shost_state drivers/scsi/sg.c --- remove-scsi-misc-2.5/drivers/scsi/sg.c~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/sg.c Tue Jul 8 14:19:52 2003 @@ -954,7 +954,8 @@ sg_ioctl(struct inode *inode, struct fil if (sdp->detached) return -ENODEV; if (filp->f_flags & O_NONBLOCK) { - if (sdp->device->host->in_recovery) + if (test_bit(SHOST_RECOVERY, + &sdp->device->host->host_flags)) return -EBUSY; } else if (!scsi_block_when_processing_errors(sdp->device)) return -EBUSY; diff -puN include/scsi/scsi_device.h~shost_state include/scsi/scsi_device.h --- remove-scsi-misc-2.5/include/scsi/scsi_device.h~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/include/scsi/scsi_device.h Tue Jul 8 14:19:52 2003 @@ -94,7 +94,7 @@ struct scsi_device { extern struct scsi_device *scsi_add_device(struct Scsi_Host *, uint, uint, uint); extern int scsi_remove_device(struct scsi_device *); -extern void scsi_set_device_offline(struct scsi_device *); +extern int scsi_device_cancel(struct device *, void *); extern int scsi_device_get(struct scsi_device *); extern void scsi_device_put(struct scsi_device *); diff -puN include/scsi/scsi_host.h~shost_state include/scsi/scsi_host.h --- remove-scsi-misc-2.5/include/scsi/scsi_host.h~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/include/scsi/scsi_host.h Tue Jul 8 14:19:52 2003 @@ -348,6 +348,14 @@ struct scsi_host_template { struct list_head legacy_hosts; }; +/* + * shost flags + */ +#define SHOST_ADD 1 +#define SHOST_DEL 2 +#define SHOST_CANCEL 3 +#define SHOST_RECOVERY 4 + struct Scsi_Host { struct list_head my_devices; struct scsi_host_cmd_pool *cmd_pool; @@ -413,7 +421,6 @@ struct Scsi_Host { short unsigned int sg_tablesize; short unsigned int max_sectors; - unsigned in_recovery:1; unsigned unchecked_isa_dma:1; unsigned use_clustering:1; unsigned highmem_io:1; @@ -454,6 +461,9 @@ struct Scsi_Host { unsigned char n_io_port; unsigned char dma_channel; unsigned int irq; + + + unsigned long host_flags; /* @@ -480,8 +490,8 @@ struct Scsi_Host { extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int); extern int scsi_add_host(struct Scsi_Host *, struct device *); -extern int scsi_remove_host(struct Scsi_Host *); -extern void scsi_host_get(struct Scsi_Host *); +extern void scsi_remove_host(struct Scsi_Host *); +extern struct Scsi_Host *scsi_host_get(struct Scsi_Host *); extern void scsi_host_put(struct Scsi_Host *t); extern struct Scsi_Host *scsi_host_lookup(unsigned short); diff -puN drivers/usb/storage/usb.c~shost_state drivers/usb/storage/usb.c --- remove-scsi-misc-2.5/drivers/usb/storage/usb.c~shost_state Tue Jul 8 14:19:52 2003 +++ remove-scsi-misc-2.5-andmike/drivers/usb/storage/usb.c Tue Jul 8 14:19:52 2003 @@ -993,12 +993,7 @@ static void storage_disconnect(struct us /* Dissociate from the USB device */ dissociate_dev(us); - /* Begin the SCSI host removal sequence */ - if (scsi_remove_host(us->host)) { - US_DEBUGP("-- SCSI refused to remove the host\n"); - BUG(); - return; - } + scsi_remove_host(us->host); /* TODO: somehow, wait for the device to * be 'idle' (tasklet completion) */ _ ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] scsi host/scsi device ref count cleanup 4/4 2003-07-08 22:27 ` [PATCH] scsi host/scsi device ref count cleanup 3/4 Mike Anderson @ 2003-07-08 22:27 ` Mike Anderson 2003-07-09 8:15 ` (unknown) Christoph Hellwig 2003-07-09 7:53 ` [PATCH] scsi host/scsi device ref count cleanup 3/4 Christoph Hellwig 1 sibling, 1 reply; 10+ messages in thread From: Mike Anderson @ 2003-07-08 22:27 UTC (permalink / raw) To: linux-scsi -andmike -- Michael Anderson andmike@us.ibm.com DESC This is a cleanup patch for scsi_device removal. - Addition of device_flags member to the scsi_device strucuture to contain "state" of the device instance. - Added SDEV_ADD, SDEV_DEL, SDEV_CANCEL, SDEV_RECOVERY states for a scsi device - Built on previous patch that renamed scsi_set_device_offline to scsi_device_cancel to use SDEV_CANCEL value. - Usage of the device_flags flags member to stop scsi_device_get's and final cleanup on access_count going to zero. - Added newer release functions and reordered scsi device sysfs unregister. EDESC drivers/scsi/scsi.c | 36 ++++++++++++++++++++++----- drivers/scsi/scsi_priv.h | 1 drivers/scsi/scsi_sysfs.c | 60 ++++++++++++++++++++++++++++++--------------- include/scsi/scsi_device.h | 13 +++++++++ 4 files changed, 84 insertions(+), 26 deletions(-) diff -puN drivers/scsi/scsi.c~sdev_state drivers/scsi/scsi.c --- remove-scsi-misc-2.5/drivers/scsi/scsi.c~sdev_state Tue Jul 8 14:19:55 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi.c Tue Jul 8 14:19:55 2003 @@ -893,17 +893,41 @@ int scsi_track_queue_full(struct scsi_de int scsi_device_get(struct scsi_device *sdev) { - if (!try_module_get(sdev->host->hostt->module)) - return -ENXIO; + struct class *class = class_get(&sdev_class); + int error = -ENXIO; - sdev->access_count++; - return 0; + if (class) { + down_write(&class->subsys.rwsem); + if (!test_bit(SDEV_DEL, &sdev->device_flags)) + if (try_module_get(sdev->host->hostt->module)) + if (get_device(&sdev->sdev_driverfs_dev)) { + sdev->access_count++; + error = 0; + } + up_write(&class->subsys.rwsem); + class_put(&sdev_class); + } + + return error; } void scsi_device_put(struct scsi_device *sdev) { - sdev->access_count--; + struct class *class = class_get(&sdev_class); + + if (!class) + return; + + down_write(&class->subsys.rwsem); module_put(sdev->host->hostt->module); + if (--sdev->access_count == 0) { + if (test_bit(SDEV_DEL, &sdev->device_flags)) + device_del(&sdev->sdev_driverfs_dev); + } + put_device(&sdev->sdev_driverfs_dev); + up_write(&class->subsys.rwsem); + + class_put(&sdev_class); } /** @@ -921,7 +945,7 @@ int scsi_device_cancel(struct device *de struct scsi_device *sdev = to_scsi_device(dev); unsigned int recovery = *(unsigned int *)data; - sdev->online = 0; + set_bit(SDEV_CANCEL, &sdev->device_flags); spin_lock_irqsave(&sdev->list_lock, flags); list_for_each_entry(scmd, &sdev->cmd_list, list) { diff -puN drivers/scsi/scsi_sysfs.c~sdev_state drivers/scsi/scsi_sysfs.c --- remove-scsi-misc-2.5/drivers/scsi/scsi_sysfs.c~sdev_state Tue Jul 8 14:19:55 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_sysfs.c Tue Jul 8 14:19:55 2003 @@ -78,8 +78,25 @@ struct class shost_class = { .release = scsi_host_cls_release, }; -static struct class sdev_class = { +static void scsi_device_cls_release(struct class_device *class_dev) +{ + struct scsi_device *sdev; + + sdev = class_to_sdev(class_dev); + put_device(&sdev->sdev_driverfs_dev); +} + +static void scsi_device_dev_release(struct device *dev) +{ + struct scsi_device *sdev; + + sdev = to_scsi_device(dev); + scsi_free_sdev(sdev); +} + +struct class sdev_class = { .name = "scsi_device", + .release = scsi_device_cls_release, }; /* all probing is done in the individual ->probe routines */ @@ -129,7 +146,7 @@ void scsi_sysfs_unregister(void) */ #define sdev_show_function(field, format_string) \ static ssize_t \ -show_##field (struct device *dev, char *buf) \ +sdev_show_##field (struct device *dev, char *buf) \ { \ struct scsi_device *sdev; \ sdev = to_scsi_device(dev); \ @@ -142,7 +159,7 @@ show_##field (struct device *dev, char * */ #define sdev_rd_attr(field, format_string) \ sdev_show_function(field, format_string) \ -static DEVICE_ATTR(field, S_IRUGO, show_##field, NULL) +static DEVICE_ATTR(field, S_IRUGO, sdev_show_##field, NULL) /* @@ -160,7 +177,7 @@ sdev_store_##field (struct device *dev, snscanf (buf, 20, format_string, &sdev->field); \ return count; \ } \ -static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, show_##field, sdev_store_##field) +static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field) /* * sdev_rd_attr: create a function and attribute variable for a @@ -182,7 +199,7 @@ sdev_store_##field (struct device *dev, } \ return ret; \ } \ -static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, show_##field, sdev_store_##field) +static DEVICE_ATTR(field, S_IRUGO | S_IWUSR, sdev_show_##field, sdev_store_##field) /* * scsi_sdev_check_buf_bit: return 0 if buf is "0", return 1 if buf is "1", @@ -212,6 +229,7 @@ sdev_rd_attr (access_count, "%d\n"); sdev_rd_attr (vendor, "%.8s\n"); sdev_rd_attr (model, "%.16s\n"); sdev_rd_attr (rev, "%.4s\n"); +sdev_rd_attr (device_flags, "%lx\n"); sdev_rw_attr_bit (online); static ssize_t @@ -233,20 +251,12 @@ struct device_attribute *scsi_sysfs_sdev &dev_attr_vendor, &dev_attr_model, &dev_attr_rev, + &dev_attr_device_flags, &dev_attr_online, &dev_attr_rescan, NULL }; -static void scsi_device_release(struct device *dev) -{ - struct scsi_device *sdev; - - sdev = to_scsi_device(dev); - if (!sdev) - return; - scsi_free_sdev(sdev); -} /** * scsi_device_register - register a scsi device with the scsi bus @@ -259,12 +269,13 @@ int scsi_device_register(struct scsi_dev { int error = 0, i; + __set_bit(SDEV_ADD, &sdev->device_flags); device_initialize(&sdev->sdev_driverfs_dev); sprintf(sdev->sdev_driverfs_dev.bus_id,"%d:%d:%d:%d", sdev->host->host_no, sdev->channel, sdev->id, sdev->lun); sdev->sdev_driverfs_dev.parent = &sdev->host->host_gendev; sdev->sdev_driverfs_dev.bus = &scsi_bus_type; - sdev->sdev_driverfs_dev.release = scsi_device_release; + sdev->sdev_driverfs_dev.release = scsi_device_dev_release; class_device_initialize(&sdev->sdev_classdev); sdev->sdev_classdev.dev = &sdev->sdev_driverfs_dev; @@ -284,6 +295,8 @@ int scsi_device_register(struct scsi_dev return error; } + get_device(&sdev->sdev_driverfs_dev); + for (i = 0; !error && sdev->host->hostt->sdev_attrs[i] != NULL; i++) error = device_create_file(&sdev->sdev_driverfs_dev, sdev->host->hostt->sdev_attrs[i]); @@ -300,12 +313,21 @@ int scsi_device_register(struct scsi_dev **/ void scsi_device_unregister(struct scsi_device *sdev) { - int i; + struct class *class = class_get(&sdev_class); - for (i = 0; sdev->host->hostt->sdev_attrs[i] != NULL; i++) - device_remove_file(&sdev->sdev_driverfs_dev, sdev->host->hostt->sdev_attrs[i]); + if (class) { + down_write(&class->subsys.rwsem); + __set_bit(SDEV_DEL, &sdev->device_flags); + if (sdev->access_count == 0) + device_del(&sdev->sdev_driverfs_dev); + up_write(&class->subsys.rwsem); + } + class_device_unregister(&sdev->sdev_classdev); - device_unregister(&sdev->sdev_driverfs_dev); + + put_device(&sdev->sdev_driverfs_dev); + + class_put(&sdev_class); } int scsi_register_driver(struct device_driver *drv) diff -puN include/scsi/scsi_device.h~sdev_state include/scsi/scsi_device.h --- remove-scsi-misc-2.5/include/scsi/scsi_device.h~sdev_state Tue Jul 8 14:19:55 2003 +++ remove-scsi-misc-2.5-andmike/include/scsi/scsi_device.h Tue Jul 8 14:19:55 2003 @@ -10,6 +10,14 @@ struct scsi_cmnd; struct scsi_mode_data; +/* + * sdev flags + */ +#define SDEV_ADD 1 +#define SDEV_DEL 2 +#define SDEV_CANCEL 3 +#define SDEV_RECOVERY 4 + struct scsi_device { struct class_device sdev_classdev; @@ -87,9 +95,13 @@ struct scsi_device { #define SCSI_DEFAULT_DEVICE_BLOCKED 3 struct device sdev_driverfs_dev; + + unsigned long device_flags; }; #define to_scsi_device(d) \ container_of(d, struct scsi_device, sdev_driverfs_dev) +#define class_to_sdev(d) \ + container_of(d, struct scsi_device, sdev_classdev) extern struct scsi_device *scsi_add_device(struct Scsi_Host *, uint, uint, uint); @@ -107,5 +119,4 @@ extern int scsi_set_medium_removal(struc extern int scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage, unsigned char *buffer, int len, int timeout, int retries, struct scsi_mode_data *data); - #endif /* _SCSI_SCSI_DEVICE_H */ diff -puN drivers/scsi/scsi_priv.h~sdev_state drivers/scsi/scsi_priv.h --- remove-scsi-misc-2.5/drivers/scsi/scsi_priv.h~sdev_state Tue Jul 8 14:19:55 2003 +++ remove-scsi-misc-2.5-andmike/drivers/scsi/scsi_priv.h Tue Jul 8 14:19:55 2003 @@ -127,6 +127,7 @@ extern struct class_device_attribute *sc extern struct device_attribute *scsi_sysfs_sdev_attrs[]; extern struct class shost_class; +extern struct class sdev_class; extern struct bus_type scsi_bus_type; #endif /* _SCSI_PRIV_H */ _ ^ permalink raw reply [flat|nested] 10+ messages in thread
* (unknown) 2003-07-08 22:27 ` [PATCH] scsi host/scsi device ref count cleanup 4/4 Mike Anderson @ 2003-07-09 8:15 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2003-07-09 8:15 UTC (permalink / raw) To: linux-scsi > + __set_bit(SDEV_ADD, &sdev->device_flags); Same atomic vs non atomic issue as in the previous patch. > +/* > + * sdev flags > + */ > +#define SDEV_ADD 1 > +#define SDEV_DEL 2 > +#define SDEV_CANCEL 3 > +#define SDEV_RECOVERY 4 Enum again? > + unsigned long device_flags; sdev_state? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi host/scsi device ref count cleanup 3/4 2003-07-08 22:27 ` [PATCH] scsi host/scsi device ref count cleanup 3/4 Mike Anderson 2003-07-08 22:27 ` [PATCH] scsi host/scsi device ref count cleanup 4/4 Mike Anderson @ 2003-07-09 7:53 ` Christoph Hellwig 2003-07-10 14:03 ` Mike Anderson 1 sibling, 1 reply; 10+ messages in thread From: Christoph Hellwig @ 2003-07-09 7:53 UTC (permalink / raw) To: linux-scsi On Tue, Jul 08, 2003 at 03:27:07PM -0700, Mike Anderson wrote: > -andmike > -- > Michael Anderson > andmike@us.ibm.com > > DESC > This is a cleanup patch for scsi_host removal. > - Addition of a host_flags member to the scsi_host strucuture to > contain "state" of the host instance. > - Added SHOST_ADD, SHOST_DEL, SHOST_CANCEL, SHOST_RECOVERY states > for a scsi host Hmm, I'd call this shost_state. > + spin_lock_irqsave(shost->host_lock, flags); > + __set_bit(SHOST_CANCEL, &shost->host_flags); > + spin_unlock_irqrestore(shost->host_lock, flags); > + device_for_each_child(&shost->host_gendev, &recovery, > + scsi_device_cancel); > + wait_event(shost->host_wait, (!test_bit(SHOST_RECOVERY, > + &shost->host_flags))); Using the non-atomic and the atomic bitops on one variable doesn't get you atomicity. Always use the atomic ones if you need them sometimes. > +/** > + * scsi_remove_host - remove a scsi host > + * @shost: a pointer to a scsi host to remove > + **/ > +void scsi_remove_host(struct Scsi_Host *shost) A, cool I already had a patch pending to make this a void retval. > -void scsi_host_get(struct Scsi_Host *shost) > +struct Scsi_Host * scsi_host_get(struct Scsi_Host *shost) struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost) > { > - get_device(&shost->host_gendev); > - class_device_get(&shost->class_dev); > + struct Scsi_Host *res_shost = shost; > + > + if (res_shost) { I don't think we should allow for a NULL argument here. Also can't we just return NULL in the error case? That would simplify the code to: struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost) { if (test_bit(SHOST_DEL, &shost->host_flags) || !get_device(&res_shost->host_gendev)) return NULL; return shost; } > void scsi_host_put(struct Scsi_Host *shost) > { > - class_device_put(&shost->class_dev); Can you explain why we don't need that anymore? /me wants to learn a bit more about deep LDM magic.. > -void scsi_set_device_offline(struct scsi_device *sdev) > +int scsi_device_cancel(struct device *dev, void *data) > { > struct scsi_cmnd *scmd; > LIST_HEAD(active_list); > struct list_head *lh, *lh_sf; > unsigned long flags; > + struct scsi_device *sdev = to_scsi_device(dev); > + unsigned int recovery = *(unsigned int *)data; I think the prototype should be int scsi_device_cancel(struct scsi_device *sdev, int recovery) and there should be a small wrapper for the LDM iterator. > +shost_rd_attr(host_flags, "%lx\n"); Shouldn't these better be printed in ascii instead of the binary presentation that might aswell change form one kernelrelease to another? > +/* > + * shost flags > + */ > +#define SHOST_ADD 1 > +#define SHOST_DEL 2 > +#define SHOST_CANCEL 3 > +#define SHOST_RECOVERY 4 Make this an enum? > /* Dissociate from the USB device */ > dissociate_dev(us); > > - /* Begin the SCSI host removal sequence */ > - if (scsi_remove_host(us->host)) { > - US_DEBUGP("-- SCSI refused to remove the host\n"); > - BUG(); > - return; > - } > + scsi_remove_host(us->host); Also in usb-storage the loop over my_devices to set them offline just before scsi_remove_host should be nuked now, shouldn't it? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi host/scsi device ref count cleanup 3/4 2003-07-09 7:53 ` [PATCH] scsi host/scsi device ref count cleanup 3/4 Christoph Hellwig @ 2003-07-10 14:03 ` Mike Anderson 2003-07-13 13:39 ` Christoph Hellwig 0 siblings, 1 reply; 10+ messages in thread From: Mike Anderson @ 2003-07-10 14:03 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-scsi Christoph Hellwig [hch@infradead.org] wrote: > On Tue, Jul 08, 2003 at 03:27:07PM -0700, Mike Anderson wrote: > > -andmike > > -- > > Michael Anderson > > andmike@us.ibm.com > > > > DESC > > This is a cleanup patch for scsi_host removal. > > - Addition of a host_flags member to the scsi_host strucuture to > > contain "state" of the host instance. > > - Added SHOST_ADD, SHOST_DEL, SHOST_CANCEL, SHOST_RECOVERY states > > for a scsi host > > Hmm, I'd call this shost_state. > Done. > > > + spin_lock_irqsave(shost->host_lock, flags); > > + __set_bit(SHOST_CANCEL, &shost->host_flags); > > + spin_unlock_irqrestore(shost->host_lock, flags); > > + device_for_each_child(&shost->host_gendev, &recovery, > > + scsi_device_cancel); > > + wait_event(shost->host_wait, (!test_bit(SHOST_RECOVERY, > > + &shost->host_flags))); > > Using the non-atomic and the atomic bitops on one variable doesn't > get you atomicity. Always use the atomic ones if you need them sometimes. Yes, I will clean this up. I went through many different iterations of state implementations and got sloppy. > > -void scsi_host_get(struct Scsi_Host *shost) > > +struct Scsi_Host * scsi_host_get(struct Scsi_Host *shost) > > struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost) > > > { > > - get_device(&shost->host_gendev); > > - class_device_get(&shost->class_dev); > > + struct Scsi_Host *res_shost = shost; > > + > > + if (res_shost) { > > I don't think we should allow for a NULL argument here. Also > can't we just return NULL in the error case? That would > simplify the code to: > > struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost) > { > if (test_bit(SHOST_DEL, &shost->host_flags) || > !get_device(&res_shost->host_gendev)) > return NULL; > return shost; > } > Done. > > void scsi_host_put(struct Scsi_Host *shost) > > { > > - class_device_put(&shost->class_dev); > > Can you explain why we don't need that anymore? /me wants to learn a bit > more about deep LDM magic.. > I do not know if I can reveal the magic as some of this is bread crumbs created by previous bad paths taken :-). I think SCSI core has unique issues stuck between the bus layer struct device LDM registration and the block layer pure kobjects. I removed the class get/puts to simplify the scsi_host_get code and reduce ordering problems between the class and device release methods. We could have chosen either the structure device or the class device to ref count on. The struct device appears to be a better object to ref count against as it is the child of the adapter struct device and the parent of the scsi device struct device. We also had an issue previously discussed by Alan of no class device release method which could cause a problem if a user had an open on a scsi_host class attribute.. So now our scsi host ref counting is like the following: (Note: scsi_sysfs_remove_host has device_del as the patch I sent out was wrong in this area and had some old code used to debug a issue with scsi_sysfs_init_host: (1) device_initialize host_gendev (ref = 1) (2) class_device_initialize shost_classdev (ref = 1) (3) get_device on host_gendev (ref+1 (2)) (This will keep the host_gendev in place until release of the shost_classdev) scsi_sysfs_add_host (1) device_add on host_gendev (ref+1 (3)) and (ref+1) on parent (2) get_device on parent to hold them in place post device_del. (3) class_device_add on shost_classdev (ref+1 (2)) scsi_sysfs_remove_host (1) class_device_unregister on shost_classdev (ref-2) (del and put will result in release if no users have opens on host class attributes) (2) device_del on shost_gendev (ref-1) and (ref-1) on parent scsi_host_cls_release put_device on host_gendev (ref-1) (This will allow the host_gendev release to be called) scsi_host_dev_release (1) put_device on parent (ref-1) (2) scsi_free_shost scsi_host_get get_device on host_gendev (ref+1) scsi_host_put put_device on shost_gendev (ref-1) Each scsi_device_register / scsi_device_unregister will incr / dec the host_gendev ref. > > -void scsi_set_device_offline(struct scsi_device *sdev) > > +int scsi_device_cancel(struct device *dev, void *data) > > { > > struct scsi_cmnd *scmd; > > LIST_HEAD(active_list); > > struct list_head *lh, *lh_sf; > > unsigned long flags; > > + struct scsi_device *sdev = to_scsi_device(dev); > > + unsigned int recovery = *(unsigned int *)data; > > I think the prototype should be > > int scsi_device_cancel(struct scsi_device *sdev, int recovery) > > and there should be a small wrapper for the LDM iterator. > ok I created a wrapper function called scsi_device_cancel_cb (seems like others where using callback but that seemed to long). > > +shost_rd_attr(host_flags, "%lx\n"); > > Shouldn't these better be printed in ascii instead of the binary > presentation that might aswell change form one kernelrelease to > another? > Yes, I was lazy and did this initially for debug. Do have a suggested format (i.e., comma separated or colon separated or ??). > > +/* > > + * shost flags > > + */ > > +#define SHOST_ADD 1 > > +#define SHOST_DEL 2 > > +#define SHOST_CANCEL 3 > > +#define SHOST_RECOVERY 4 > > Make this an enum? > Done. I guess also since I am using set_bit that I should use the zero shift (not that we will need all these states). > > /* Dissociate from the USB device */ > > dissociate_dev(us); > > > > - /* Begin the SCSI host removal sequence */ > > - if (scsi_remove_host(us->host)) { > > - US_DEBUGP("-- SCSI refused to remove the host\n"); > > - BUG(); > > - return; > > - } > > + scsi_remove_host(us->host); > > Also in usb-storage the loop over my_devices to set them offline > just before scsi_remove_host should be nuked now, shouldn't it? Yes it should. I made this a separate patch in the series in case Matthew / Alan see other changes needed with usb storage shutdown. -andmike -- Michael Anderson andmike@us.ibm.com ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi host/scsi device ref count cleanup 3/4 2003-07-10 14:03 ` Mike Anderson @ 2003-07-13 13:39 ` Christoph Hellwig 0 siblings, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2003-07-13 13:39 UTC (permalink / raw) To: Christoph Hellwig, linux-scsi On Thu, Jul 10, 2003 at 07:03:40AM -0700, Mike Anderson wrote: > > Can you explain why we don't need that anymore? /me wants to learn a bit > > more about deep LDM magic.. > > > > I do not know if I can reveal the magic as some of this is bread crumbs > created by previous bad paths taken :-). Hehe. > I think SCSI core has unique issues stuck between the bus layer struct > device LDM registration and the block layer pure kobjects. > > I removed the class get/puts to simplify the scsi_host_get code and > reduce ordering problems between the class and device release methods. > We could have chosen either the structure device or the class device to > ref count on. The struct device appears to be a better object to ref > count against as it is the child of the adapter struct device and the > parent of the scsi device struct device. Agreed. What do we need the classdev for at all? > ok I created a wrapper function called scsi_device_cancel_cb (seems like > others where using callback but that seemed to long). or just name it cancel_callback (I assume it's static) > > Shouldn't these better be printed in ascii instead of the binary > > presentation that might aswell change form one kernelrelease to > > another? > > > > Yes, I was lazy and did this initially for debug. Do have a suggested > format (i.e., comma separated or colon separated or ??). coma separated looks fine to me. > > Yes it should. I made this a separate patch in the series > in case Matthew / Alan see other changes needed with usb storage > shutdown. Ok. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi host/scsi device ref count cleanup 2/4 2003-07-08 22:26 ` [PATCH] scsi host/scsi device ref count cleanup 2/4 Mike Anderson 2003-07-08 22:27 ` [PATCH] scsi host/scsi device ref count cleanup 3/4 Mike Anderson @ 2003-07-09 7:39 ` Christoph Hellwig 1 sibling, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2003-07-09 7:39 UTC (permalink / raw) To: linux-scsi On Tue, Jul 08, 2003 at 03:26:22PM -0700, Mike Anderson wrote: > aic79xx_osm and aic7xxx_osm are 2.4 code. ioctl_probe has side effect of > needing present value. ioctl_probe is pretty buggy anyways. I wonder whether we should just kill it. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2003-07-13 13:24 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2003-07-08 22:24 [PATCH] scsi host/scsi device ref count cleanup 0/4 Mike Anderson 2003-07-08 22:25 ` [PATCH] scsi host/scsi device ref count cleanup 1/4 Mike Anderson 2003-07-08 22:26 ` [PATCH] scsi host/scsi device ref count cleanup 2/4 Mike Anderson 2003-07-08 22:27 ` [PATCH] scsi host/scsi device ref count cleanup 3/4 Mike Anderson 2003-07-08 22:27 ` [PATCH] scsi host/scsi device ref count cleanup 4/4 Mike Anderson 2003-07-09 8:15 ` (unknown) Christoph Hellwig 2003-07-09 7:53 ` [PATCH] scsi host/scsi device ref count cleanup 3/4 Christoph Hellwig 2003-07-10 14:03 ` Mike Anderson 2003-07-13 13:39 ` Christoph Hellwig 2003-07-09 7:39 ` [PATCH] scsi host/scsi device ref count cleanup 2/4 Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).