From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: [PATCH] rework shost/sdev attribute handling Date: Sat, 28 Jun 2003 10:49:26 +0200 Sender: linux-scsi-owner@vger.kernel.org Message-ID: <20030628084926.GA29596@lst.de> References: <20030627181053.GA20375@lst.de> <20030627193940.GB3338@beaverton.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from verein.lst.de ([212.34.189.10]:43239 "EHLO mail.lst.de") by vger.kernel.org with ESMTP id S265112AbTF1IfP (ORCPT ); Sat, 28 Jun 2003 04:35:15 -0400 Content-Disposition: inline In-Reply-To: <20030627193940.GB3338@beaverton.ibm.com> List-Id: linux-scsi@vger.kernel.org To: James.Bottomley@steeleye.com, linux-scsi@vger.kernel.org On Fri, Jun 27, 2003 at 12:39:40PM -0700, Mike Anderson wrote: > > I was told that you do not need to do individual removal of files if you are in the > process of unregistering. You only need to do this if you just want to remove > the attribute, but are still going to stay around. The files are cleaned > up in the sysfs_remove_dir function call. > > The comment on sysfs_remove_dir indicates this also. You're right - I looked for {,class_}device_remove_file calls but didn't notice it's handled at a much lower level. Here's an updated patch which also removes the superflous device_remove_file calls for the sdev attributes. diff -Nru a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c --- a/drivers/scsi/53c700.c Fri Jun 27 12:59:05 2003 +++ b/drivers/scsi/53c700.c Fri Jun 27 12:59:05 2003 @@ -172,7 +172,7 @@ STATIC int NCR_700_slave_configure(Scsi_Device *SDpnt); STATIC void NCR_700_slave_destroy(Scsi_Device *SDpnt); -static struct device_attribute **NCR_700_dev_attrs = NULL; +STATIC struct device_attribute *NCR_700_dev_attrs[]; static char *NCR_700_phase[] = { "", @@ -1990,6 +1990,13 @@ } static ssize_t +NCR_700_show_queue_depth(struct device *dev, char *buf) +{ + struct scsi_device *SDp = to_scsi_device(dev); + return snprintf(buf, 20, "%d\n", SDp->queue_depth); +} + +static ssize_t NCR_700_store_queue_depth(struct device *dev, const char *buf, size_t count) { int depth; @@ -2014,9 +2021,10 @@ static struct device_attribute NCR_700_queue_depth_attr = { .attr = { .name = "queue_depth", - .mode = S_IWUSR, + .mode = S_IRUGO|S_IWUSR, }, .store = NCR_700_store_queue_depth, + .show = NCR_700_show_queue_depth, }; static struct device_attribute NCR_700_active_tags_attr = { @@ -2027,25 +2035,12 @@ .show = NCR_700_show_active_tags, }; -STATIC int __init -NCR_700_init(void) -{ - scsi_sysfs_modify_sdev_attribute(&NCR_700_dev_attrs, - &NCR_700_queue_depth_attr); - scsi_sysfs_modify_sdev_attribute(&NCR_700_dev_attrs, - &NCR_700_active_tags_attr); - return 0; -} - -/* NULL exit routine to keep modutils happy */ -STATIC void __exit -NCR_700_exit(void) -{ -} +STATIC struct device_attribute *NCR_700_dev_attrs[] = { + &NCR_700_queue_depth_attr, + &NCR_700_active_tags_attr, + NULL, +}; EXPORT_SYMBOL(NCR_700_detect); EXPORT_SYMBOL(NCR_700_release); EXPORT_SYMBOL(NCR_700_intr); - -module_init(NCR_700_init); -module_exit(NCR_700_exit); diff -Nru a/drivers/scsi/NCR_D700.c b/drivers/scsi/NCR_D700.c --- a/drivers/scsi/NCR_D700.c Fri Jun 27 12:59:05 2003 +++ b/drivers/scsi/NCR_D700.c Fri Jun 27 12:59:05 2003 @@ -387,7 +387,6 @@ static void __exit NCR_D700_exit(void) { mca_unregister_driver(&NCR_D700_driver); - scsi_sysfs_release_attributes(&NCR_D700_driver_template); } module_init(NCR_D700_init); diff -Nru a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c --- a/drivers/scsi/hosts.c Fri Jun 27 12:59:05 2003 +++ b/drivers/scsi/hosts.c Fri Jun 27 12:59:05 2003 @@ -151,12 +151,6 @@ dump_stack(); } - /* if its not set in the template, use the default */ - if (!sht->shost_attrs) - sht->shost_attrs = scsi_sysfs_shost_attrs; - if (!sht->sdev_attrs) - sht->sdev_attrs = scsi_sysfs_sdev_attrs; - shost = kmalloc(sizeof(struct Scsi_Host) + privsize, gfp_mask); if (!shost) return NULL; diff -Nru a/drivers/scsi/lasi700.c b/drivers/scsi/lasi700.c --- a/drivers/scsi/lasi700.c Fri Jun 27 12:59:05 2003 +++ b/drivers/scsi/lasi700.c Fri Jun 27 12:59:05 2003 @@ -165,7 +165,6 @@ lasi700_exit(void) { unregister_parisc_driver(&lasi700_driver); - scsi_sysfs_release_attributes(&lasi700_template); } module_init(lasi700_init); diff -Nru a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h --- a/drivers/scsi/scsi_priv.h Fri Jun 27 12:59:05 2003 +++ b/drivers/scsi/scsi_priv.h Fri Jun 27 12:59:05 2003 @@ -117,11 +117,6 @@ extern int scsi_sysfs_register(void); extern void scsi_sysfs_unregister(void); -/* definitions for the linker default sections covering the host - * class and device attributes */ -extern struct class_device_attribute *scsi_sysfs_shost_attrs[]; -extern struct device_attribute *scsi_sysfs_sdev_attrs[]; - extern struct class shost_class; extern struct bus_type scsi_bus_type; diff -Nru a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c --- a/drivers/scsi/scsi_sysfs.c Fri Jun 27 12:59:05 2003 +++ b/drivers/scsi/scsi_sysfs.c Fri Jun 27 12:59:05 2003 @@ -45,7 +45,7 @@ shost_rd_attr(sg_tablesize, "%hu\n"); shost_rd_attr(unchecked_isa_dma, "%d\n"); -struct class_device_attribute *scsi_sysfs_shost_attrs[] = { +static struct class_device_attribute *scsi_sysfs_shost_attrs[] = { &class_device_attr_unique_id, &class_device_attr_host_busy, &class_device_attr_cmd_per_lun, @@ -204,7 +204,7 @@ static DEVICE_ATTR(rescan, S_IWUSR, NULL, store_rescan_field) /* Default template for device attributes. May NOT be modified */ -struct device_attribute *scsi_sysfs_sdev_attrs[] = { +static struct device_attribute *scsi_sysfs_sdev_attrs[] = { &dev_attr_device_blocked, &dev_attr_queue_depth, &dev_attr_type, @@ -228,6 +228,20 @@ scsi_free_sdev(sdev); } +static int attr_overridden(struct device_attribute **drv_attrs, + struct device_attribute *attr) +{ + int i; + + if (!drv_attrs) + return 0; + for (i = 0; drv_attrs[i]; i++) + if (!strcmp(drv_attrs[i]->attr.name, attr->attr.name)) + return 1; + + return 0; +} + /** * scsi_device_register - register a scsi device with the scsi bus * @sdev: scsi_device to register @@ -264,12 +278,24 @@ return error; } - for (i = 0; !error && sdev->host->hostt->sdev_attrs[i] != NULL; i++) - error = device_create_file(&sdev->sdev_gendev, - sdev->host->hostt->sdev_attrs[i]); - - if (error) - scsi_device_unregister(sdev); + if (sdev->host->hostt->sdev_attrs) { + for (i = 0; sdev->host->hostt->sdev_attrs[i]; i++) { + error = device_create_file(&sdev->sdev_gendev, + sdev->host->hostt->sdev_attrs[i]); + if (error) + scsi_device_unregister(sdev); + } + } + + for (i = 0; scsi_sysfs_sdev_attrs[i]; i++) { + if (!attr_overridden(sdev->host->hostt->sdev_attrs, + scsi_sysfs_sdev_attrs[i])) { + error = device_create_file(&sdev->sdev_gendev, + scsi_sysfs_sdev_attrs[i]); + if (error) + scsi_device_unregister(sdev); + } + } return error; } @@ -280,10 +306,6 @@ **/ void scsi_device_unregister(struct scsi_device *sdev) { - int i; - - for (i = 0; sdev->host->hostt->sdev_attrs[i] != NULL; i++) - device_remove_file(&sdev->sdev_gendev, sdev->host->hostt->sdev_attrs[i]); class_device_unregister(&sdev->sdev_classdev); device_unregister(&sdev->sdev_gendev); } @@ -329,6 +351,20 @@ shost->host_no); } +static int class_attr_overridden(struct class_device_attribute **drv_attrs, + struct class_device_attribute *attr) +{ + int i; + + if (!drv_attrs) + return 0; + for (i = 0; drv_attrs[i]; i++) + if (!strcmp(drv_attrs[i]->attr.name, attr->attr.name)) + return 1; + + return 0; +} + /** * scsi_sysfs_add_host - add scsi host to subsystem * @shost: scsi host struct to add to subsystem @@ -336,7 +372,7 @@ **/ int scsi_sysfs_add_host(struct Scsi_Host *shost, struct device *dev) { - int i, error; + int error, i; if (!shost->shost_gendev.parent) shost->shost_gendev.parent = dev ? dev : &legacy_bus; @@ -349,11 +385,24 @@ if (error) goto clean_device; - for (i = 0; !error && shost->hostt->shost_attrs[i] != NULL; i++) - error = class_device_create_file(&shost->shost_classdev, - shost->hostt->shost_attrs[i]); - if (error) - goto clean_class; + if (shost->hostt->shost_attrs) { + for (i = 0; shost->hostt->shost_attrs[i]; i++) { + error = class_device_create_file(&shost->shost_classdev, + shost->hostt->shost_attrs[i]); + if (error) + goto clean_class; + } + } + + for (i = 0; scsi_sysfs_shost_attrs[i]; i++) { + if (!class_attr_overridden(shost->hostt->shost_attrs, + scsi_sysfs_shost_attrs[i])) { + error = class_device_create_file(&shost->shost_classdev, + scsi_sysfs_shost_attrs[i]); + if (error) + goto clean_class; + } + } return error; @@ -374,130 +423,3 @@ class_device_del(&shost->shost_classdev); device_del(&shost->shost_gendev); } - -/** scsi_sysfs_modify_shost_attribute - modify or add a host class attribute - * - * @class_attrs:host class attribute list to be added to or modified - * @attr: individual attribute to change or added - * - * returns zero if successful or error if not - **/ -int scsi_sysfs_modify_shost_attribute( - struct class_device_attribute ***class_attrs, - struct class_device_attribute *attr) -{ - int modify = -1; - int num_attrs; - - if(*class_attrs == NULL) - *class_attrs = scsi_sysfs_shost_attrs; - - for(num_attrs=0; (*class_attrs)[num_attrs] != NULL; num_attrs++) - if(strcmp((*class_attrs)[num_attrs]->attr.name, - attr->attr.name) == 0) - modify = num_attrs; - - if(*class_attrs == scsi_sysfs_shost_attrs || modify < 0) { - /* note: need space for null at the end as well */ - struct class_device_attribute **tmp_attrs = - kmalloc(sizeof(*tmp_attrs) * - (num_attrs + (modify >= 0 ? 1 : 2)), - GFP_KERNEL); - if(tmp_attrs == NULL) - return -ENOMEM; - memcpy(tmp_attrs, *class_attrs, sizeof(*tmp_attrs) * - (num_attrs + 1)); - if(*class_attrs != scsi_sysfs_shost_attrs) - kfree(*class_attrs); - *class_attrs = tmp_attrs; - } - if(modify >= 0) { - /* spare the caller from having to copy things it's - * not interested in */ - struct class_device_attribute *old_attr = - (*class_attrs)[modify]; - /* extend permissions */ - attr->attr.mode |= old_attr->attr.mode; - - /* override null show/store with default */ - if(attr->show == NULL) - attr->show = old_attr->show; - if(attr->store == NULL) - attr->store = old_attr->store; - (*class_attrs)[modify] = attr; - } else { - (*class_attrs)[num_attrs++] = attr; - (*class_attrs)[num_attrs] = NULL; - } - - return 0; -} -EXPORT_SYMBOL(scsi_sysfs_modify_shost_attribute); - -/** scsi_sysfs_modify_sdev_attribute - modify or add a host device attribute - * - * @dev_attrs: pointer to the attribute list to be added to or modified - * @attr: individual attribute to change or added - * - * returns zero if successful or error if not - **/ -int scsi_sysfs_modify_sdev_attribute(struct device_attribute ***dev_attrs, - struct device_attribute *attr) -{ - int modify = -1; - int num_attrs; - - if(*dev_attrs == NULL) - *dev_attrs = scsi_sysfs_sdev_attrs; - - for(num_attrs=0; (*dev_attrs)[num_attrs] != NULL; num_attrs++) - if(strcmp((*dev_attrs)[num_attrs]->attr.name, - attr->attr.name) == 0) - modify = num_attrs; - - if(*dev_attrs == scsi_sysfs_sdev_attrs || modify < 0) { - /* note: need space for null at the end as well */ - struct device_attribute **tmp_attrs = - kmalloc(sizeof(*tmp_attrs) * - (num_attrs + (modify >= 0 ? 1 : 2)), - GFP_KERNEL); - if(tmp_attrs == NULL) - return -ENOMEM; - memcpy(tmp_attrs, *dev_attrs, sizeof(*tmp_attrs) * - (num_attrs + 1)); - if(*dev_attrs != scsi_sysfs_sdev_attrs) - kfree(*dev_attrs); - *dev_attrs = tmp_attrs; - } - if(modify >= 0) { - /* spare the caller from having to copy things it's - * not interested in */ - struct device_attribute *old_attr = - (*dev_attrs)[modify]; - /* extend permissions */ - attr->attr.mode |= old_attr->attr.mode; - - /* override null show/store with default */ - if(attr->show == NULL) - attr->show = old_attr->show; - if(attr->store == NULL) - attr->store = old_attr->store; - (*dev_attrs)[modify] = attr; - } else { - (*dev_attrs)[num_attrs++] = attr; - (*dev_attrs)[num_attrs] = NULL; - } - - return 0; -} -EXPORT_SYMBOL(scsi_sysfs_modify_sdev_attribute); - -void scsi_sysfs_release_attributes(struct scsi_host_template *hostt) -{ - if(hostt->sdev_attrs != scsi_sysfs_sdev_attrs) - kfree(hostt->sdev_attrs); - - if(hostt->shost_attrs != scsi_sysfs_shost_attrs) - kfree(hostt->shost_attrs); -} -EXPORT_SYMBOL(scsi_sysfs_release_attributes); diff -Nru a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h --- a/include/scsi/scsi_host.h Fri Jun 27 12:59:05 2003 +++ b/include/scsi/scsi_host.h Fri Jun 27 12:59:05 2003 @@ -329,12 +329,12 @@ #define SCSI_DEFAULT_HOST_BLOCKED 7 /* - * Pointer to the sysfs class properties for this host + * Pointer to the sysfs class properties for this host, NULL terminated. */ struct class_device_attribute **shost_attrs; /* - * Pointer to the SCSI device properties for this host + * Pointer to the SCSI device properties for this host, NULL terminated. */ struct device_attribute **sdev_attrs; @@ -498,8 +498,6 @@ { return shost->shost_gendev.parent; } - -extern void scsi_sysfs_release_attributes(struct scsi_host_template *); extern void scsi_unblock_requests(struct Scsi_Host *); extern void scsi_block_requests(struct Scsi_Host *);