| Message ID | 1281712686-31308-7-git-send-email-alexandre.bounine@idt.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kumar Gala |
| Headers | show |
Alexandre Bounine wrote: > - if (!rdev->rswitch) > - goto out; > - Is it safe? All devices have a switch? > @@ -63,10 +59,11 @@ struct device_attribute rio_dev_attrs[] = { > __ATTR_RO(asm_did), > __ATTR_RO(asm_vid), > __ATTR_RO(asm_rev), > - __ATTR_RO(routes), > __ATTR_NULL, > }; > > +static DEVICE_ATTR(routes, S_IRUGO, routes_show, NULL); > + This seems a separate change from the sw_sysfs? Why make it separate? > */ > struct rio_switch { > @@ -256,6 +257,7 @@ struct rio_switch { > u8 *sw_domain); > int (*em_init) (struct rio_dev *dev); > int (*em_handle) (struct rio_dev *dev, u8 swport); > + int (*sw_sysfs) (struct rio_dev *dev, int create); > struct rio_dev *nextdev[0]; > }; Why not make a sw_sysfs_create and sw_sysfs_remove? Is better for readability. Now you call 'sw_sysfs(dev, 0)' or 'sw_sysfs(dev, 1)'; Micha
Micha Nelissen wrote: > > Alexandre Bounine wrote: > > - if (!rdev->rswitch) > > - goto out; > > - > > Is it safe? All devices have a switch? Yes. Because end-points should not have the "routes" attribute at all (corrected by this patch). > > > @@ -63,10 +59,11 @@ struct device_attribute rio_dev_attrs[] = { > > __ATTR_RO(asm_did), > > __ATTR_RO(asm_vid), > > __ATTR_RO(asm_rev), > > - __ATTR_RO(routes), > > __ATTR_NULL, > > }; > > > > +static DEVICE_ATTR(routes, S_IRUGO, routes_show, NULL); > > + > > This seems a separate change from the sw_sysfs? Why make it separate? I assume that your question was "Why do not make it separate?" Both changes are specific to switches, both address sysfs and both are not big enough to justify a separate patch. I agree that make separate patches would give more clarity, so would do better description. Because there are changes that should be made to other patches in this set, I will regenerate this patch with better description. > > > */ > > struct rio_switch { > > @@ -256,6 +257,7 @@ struct rio_switch { > > u8 *sw_domain); > > int (*em_init) (struct rio_dev *dev); > > int (*em_handle) (struct rio_dev *dev, u8 swport); > > + int (*sw_sysfs) (struct rio_dev *dev, int create); > > struct rio_dev *nextdev[0]; > > }; > > Why not make a sw_sysfs_create and sw_sysfs_remove? Is better for > readability. Now you call 'sw_sysfs(dev, 0)' or 'sw_sysfs(dev, 1)'; I just do not want to have an extra member here. Not every switch will require own sysfs attributes, but every switch will be presented by a data structure. Based on its intended use I do not see any problem here.
Bounine, Alexandre wrote: >> Why not make a sw_sysfs_create and sw_sysfs_remove? Is better for >> readability. Now you call 'sw_sysfs(dev, 0)' or 'sw_sysfs(dev, 1)'; > > I just do not want to have an extra member here. Not every switch will > require own sysfs attributes, but every switch will be presented by a > data structure. Based on its intended use I do not see any problem here. It's not problematic, but personally I find function calls that pass 0 or 1 as an argument hard to read. Likewise for boolean parameters. An alternative would be to have defines SW_SYSFS_CREATE etc. It's a minor item. Micha
Micha Nelissen wrote: > > It's not problematic, but personally I find function calls that pass 0 > or 1 as an argument hard to read. Likewise for boolean parameters. An > alternative would be to have defines SW_SYSFS_CREATE etc. It's a minor item. > I will add defines.
diff --git a/drivers/rapidio/rio-sysfs.c b/drivers/rapidio/rio-sysfs.c index 00b4756..bfc483b 100644 --- a/drivers/rapidio/rio-sysfs.c +++ b/drivers/rapidio/rio-sysfs.c @@ -40,9 +40,6 @@ static ssize_t routes_show(struct device *dev, struct device_attribute *attr, ch char *str = buf; int i; - if (!rdev->rswitch) - goto out; - for (i = 0; i < RIO_MAX_ROUTE_ENTRIES(rdev->net->hport->sys_size); i++) { if (rdev->rswitch->route_table[i] == RIO_INVALID_ROUTE) @@ -52,7 +49,6 @@ static ssize_t routes_show(struct device *dev, struct device_attribute *attr, ch rdev->rswitch->route_table[i]); } - out: return (str - buf); } @@ -63,10 +59,11 @@ struct device_attribute rio_dev_attrs[] = { __ATTR_RO(asm_did), __ATTR_RO(asm_vid), __ATTR_RO(asm_rev), - __ATTR_RO(routes), __ATTR_NULL, }; +static DEVICE_ATTR(routes, S_IRUGO, routes_show, NULL); + static ssize_t rio_read_config(struct file *filp, struct kobject *kobj, struct bin_attribute *bin_attr, @@ -218,7 +215,17 @@ int rio_create_sysfs_dev_files(struct rio_dev *rdev) { int err = 0; - err = sysfs_create_bin_file(&rdev->dev.kobj, &rio_config_attr); + err = device_create_bin_file(&rdev->dev, &rio_config_attr); + + if (!err && rdev->rswitch) { + err = device_create_file(&rdev->dev, &dev_attr_routes); + if (!err && rdev->rswitch->sw_sysfs) + err = rdev->rswitch->sw_sysfs(rdev, 1); + } + + if (err) + pr_warning("RIO: Failed to create attribute file(s) for %s\n", + rio_name(rdev)); return err; } @@ -231,5 +238,10 @@ int rio_create_sysfs_dev_files(struct rio_dev *rdev) */ void rio_remove_sysfs_dev_files(struct rio_dev *rdev) { - sysfs_remove_bin_file(&rdev->dev.kobj, &rio_config_attr); + device_remove_bin_file(&rdev->dev, &rio_config_attr); + if (rdev->rswitch) { + device_remove_file(&rdev->dev, &dev_attr_routes); + if (rdev->rswitch->sw_sysfs) + rdev->rswitch->sw_sysfs(rdev, 0); + } } diff --git a/include/linux/rio.h b/include/linux/rio.h index 754895c..8f19fb2 100644 --- a/include/linux/rio.h +++ b/include/linux/rio.h @@ -233,6 +233,7 @@ struct rio_net { * @get_domain: Callback for switch-specific domain get function * @em_init: Callback for switch-specific error management initialization function * @em_handle: Callback for switch-specific error management handler function + * @sw_sysfs: Callback that initializes switch-specific sysfs attributes * @nextdev: Array of per-port pointers to the next attached device */ struct rio_switch { @@ -256,6 +257,7 @@ struct rio_switch { u8 *sw_domain); int (*em_init) (struct rio_dev *dev); int (*em_handle) (struct rio_dev *dev, u8 swport); + int (*sw_sysfs) (struct rio_dev *dev, int create); struct rio_dev *nextdev[0]; };