fpga: add attribute groups
cover fpga: add attribute groups
Make it easy to add device attribute groups when registering an FPGA manager, bridge, or region. This way, the sysfs attributes are added in device_add instead of afterwards which can lead to race conditions [1].
Patch 1: add groups to the structs that are used when a manager, bridge, or region is registered.
Patch 2: changes to altera cvp FPGA manager to use this method of registering.
I've tested this method on platform devices. Patch 2 builds but is untested.
This patchset is dependent on my "non-dt support" patchset currently under review.
[1] http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/
Alan Tull (2):
fpga: add attribute groups
fpga: altera-cvp: register attribute groups using ops
drivers/fpga/altera-cvp.c | 39 +++++++++++++++++++-------------------- drivers/fpga/fpga-bridge.c | 1 + drivers/fpga/fpga-mgr.c | 1 + drivers/fpga/fpga-region.c | 1 + include/linux/fpga/fpga-bridge.h | 2 ++ include/linux/fpga/fpga-mgr.h | 2 ++ include/linux/fpga/fpga-region.h | 2 ++ 7 files changed, 28 insertions(+), 20 deletions(-)
Signature · 1 line
2.7.4
PATCH 1/2 fpga: add attribute groups
Make it easy to add attributes to low level FPGA drivers the right way. Add attribute groups pointers to structures that are used when registering a manager, bridge, or group. When the low level driver registers, set the device attribute group. The attributes are created in device_add.
Signed-off-by Alan Tull <[email protected]>
--- drivers/fpga/fpga-bridge.c | 1 + drivers/fpga/fpga-mgr.c | 1 + drivers/fpga/fpga-region.c | 1 + include/linux/fpga/fpga-bridge.h | 2 ++ include/linux/fpga/fpga-mgr.h | 2 ++ include/linux/fpga/fpga-region.h | 2 ++ 6 files changed, 9 insertions(+)
drivers/fpga/fpga-bridge.c +1 -0
@@ -367,6 +367,7 @@ int fpga_bridge_register(struct device *dev, const char *name, bridge->priv = priv;device_initialize (&bridge->dev); + bridge->dev.groups = br_ops->groups; bridge->dev.class = fpga_bridge_class; bridge->dev.parent = dev; bridge->dev.of_node = dev->of_node;
drivers/fpga/fpga-mgr.c +1 -0
@@ -560,6 +560,7 @@ int fpga_mgr_register(struct device *dev, const char *name,device_initialize (&mgr->dev); mgr->dev.class = fpga_mgr_class; + mgr->dev.groups = mops->groups; mgr->dev.parent = dev; mgr->dev.of_node = dev->of_node; mgr->dev.id = id;
drivers/fpga/fpga-region.c +1 -0
@@ -173,6 +173,7 @@ int fpga_region_register(struct device *dev, struct fpga_region *region)mutex_init (®ion->mutex);INIT_LIST_HEAD (®ion->bridge_list);device_initialize (®ion->dev); + region->dev.groups = region->groups; region->dev.class = fpga_region_class; region->dev.parent = dev; region->dev.of_node = dev->of_node;
include/linux/fpga/fpga-bridge.h +2 -0
@@ -11,11 +11,13 @@ struct fpga_bridge;* @enable_show: returns the FPGA bridge's status * @enable_set: set a FPGA bridge as enabled or disabled * @fpga_bridge_remove: set FPGA into a specific state during driver remove +* @groups: optional attribute groups. */ struct fpga_bridge_ops {int (*enable_show)(struct fpga_bridge *bridge);int (*enable_set)(struct fpga_bridge *bridge,bool enable);void (*fpga_bridge_remove)(struct fpga_bridge *bridge); +const struct attribute_group **groups; };/**
include/linux/fpga/fpga-mgr.h +2 -0
@@ -113,6 +113,7 @@ struct fpga_image_info {* @write_sg: write the scatter list of configuration data to the FPGA * @write_complete: set FPGA to operating state after writing is done * @fpga_remove: optional: Set FPGA into a specific state during driver remove +* @groups: optional attribute groups. * * fpga_manager_ops are the low level functions implemented by a specific * fpga manager driver. The optional ones are tested for NULL before being
@@ -129,6 +130,7 @@ struct fpga_manager_ops {int (*write_complete)(struct fpga_manager *mgr,struct fpga_image_info *info);void (*fpga_remove)(struct fpga_manager *mgr); +const struct attribute_group **groups; };/**
include/linux/fpga/fpga-region.h +2 -0
@@ -14,6 +14,7 @@* @info: FPGA image info * @priv: private data * @get_bridges: optional function to get bridges to a list +* @groups: optional attribute groups. */ struct fpga_region {struct device dev;
@@ -23,6 +24,7 @@ struct fpga_region {struct fpga_image_info *info;void *priv;int (*get_bridges)(struct fpga_region *region); +const struct attribute_group **groups; };#define to_fpga_region (d) container_of(d, struct fpga_region, dev)
Signature · 1 line
2.7.4
PATCH 2/2 fpga: altera-cvp: register attribute groups using ops
In reply to Alan Tull--- drivers/fpga/altera-cvp.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-)
drivers/fpga/altera-cvp.c +19 -20
@@ -354,20 +354,14 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,return ret; } -static const struct fpga_manager_ops altera_cvp_ops = { - .state = altera_cvp_state, - .write_init = altera_cvp_write_init, - .write = altera_cvp_write, - .write_complete = altera_cvp_write_complete, -}; - -static ssize_t show_chkcfg (struct device_driver *dev,char *buf) +static ssize_t show_chkcfg (struct device *dev, +struct device_attribute *attr,char *buf) {return snprintf (buf,3 ,"%d\n" , altera_cvp_chkcfg); } -static ssize_t store_chkcfg (struct device_driver *drv,const char *buf, -size_t count) +static ssize_t store_chkcfg (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) {int ret;
@@ -378,7 +372,21 @@ static ssize_t store_chkcfg(struct device_driver *drv, const char *buf,return count; } -static DRIVER_ATTR(chkcfg, 0600, show_chkcfg, store_chkcfg); +static DEVICE_ATTR(chkcfg, 0600, show_chkcfg, store_chkcfg); + +static struct attribute *altera_cvp_attrs[] = { + &dev_attr_chkcfg.attr, +NULL , +}; +ATTRIBUTE_GROUPS (altera_cvp); + +static const struct fpga_manager_ops altera_cvp_ops = { + .state = altera_cvp_state, + .write_init = altera_cvp_write_init, + .write = altera_cvp_write, + .write_complete = altera_cvp_write_complete, + .groups = altera_cvp_groups, +};static int altera_cvp_probe (struct pci_dev *pdev,const struct pci_device_id *dev_id);
@@ -459,14 +467,6 @@ static int altera_cvp_probe(struct pci_dev *pdev,if (ret) goto err_unmap; - ret =driver_create_file (&altera_cvp_driver.driver, - &driver_attr_chkcfg); -if (ret) { -dev_err (&pdev->dev,"Can't create sysfs chkcfg file\n" ); -fpga_mgr_unregister (&pdev->dev); - goto err_unmap; - } -return 0 ; err_unmap:
@@ -484,7 +484,6 @@ static void altera_cvp_remove(struct pci_dev *pdev)struct altera_cvp_conf *conf = mgr->priv;u16 cmd; -driver_remove_file (&altera_cvp_driver.driver, &driver_attr_chkcfg);fpga_mgr_unregister (&pdev->dev);pci_iounmap (pdev, conf->map);pci_release_region (pdev,CVP_BAR );
Signature · 1 line
2.7.4
Re: [PATCH 2/2] fpga: altera-cvp: register attribute groups using ops
On Tue, Aug 22, 2017 at 10:53 AM, Alan Tull <[email protected]> wrote:
OK so obviously, this patch doesn't have the necessary header or signoff. Will fix that.
Alan
Quoted 82 lines
--- drivers/fpga/altera-cvp.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-)drivers/fpga/altera-cvp.c +19
-21@@ -354,20 +354,14 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,return ret; } -static const struct fpga_manager_ops altera_cvp_ops = { - .state = altera_cvp_state, - .write_init = altera_cvp_write_init, - .write = altera_cvp_write, - .write_complete = altera_cvp_write_complete, -}; - -static ssize_t show_chkcfg (struct device_driver *dev,char *buf) +static ssize_t show_chkcfg (struct device *dev, +struct device_attribute *attr,char *buf) {return snprintf (buf,3 ,"%d\n" , altera_cvp_chkcfg); } -static ssize_t store_chkcfg (struct device_driver *drv,const char *buf, -size_t count) +static ssize_t store_chkcfg (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) {int ret;@@ -378,7 +372,21 @@ static ssize_t store_chkcfg(struct device_driver *drv, const char *buf,return count; } -static DRIVER_ATTR(chkcfg, 0600, show_chkcfg, store_chkcfg); +static DEVICE_ATTR(chkcfg, 0600, show_chkcfg, store_chkcfg); + +static struct attribute *altera_cvp_attrs[] = { + &dev_attr_chkcfg.attr, +NULL , +}; +ATTRIBUTE_GROUPS (altera_cvp); + +static const struct fpga_manager_ops altera_cvp_ops = { + .state = altera_cvp_state, + .write_init = altera_cvp_write_init, + .write = altera_cvp_write, + .write_complete = altera_cvp_write_complete, + .groups = altera_cvp_groups, +};static int altera_cvp_probe (struct pci_dev *pdev,const struct pci_device_id *dev_id);@@ -459,14 +467,6 @@ static int altera_cvp_probe(struct pci_dev *pdev,if (ret) goto err_unmap; - ret =driver_create_file (&altera_cvp_driver.driver, - &driver_attr_chkcfg); -if (ret) { -dev_err (&pdev->dev,"Can't create sysfs chkcfg file\n" ); -fpga_mgr_unregister (&pdev->dev); - goto err_unmap; - } -return 0 ; err_unmap:@@ -484,7 +484,6 @@ static void altera_cvp_remove(struct pci_dev *pdev)struct altera_cvp_conf *conf = mgr->priv;u16 cmd; -driver_remove_file (&altera_cvp_driver.driver, &driver_attr_chkcfg);fpga_mgr_unregister (&pdev->dev);pci_iounmap (pdev, conf->map);pci_release_region (pdev,CVP_BAR ); --2.7.4
Re: [PATCH 2/2] fpga: altera-cvp: register attribute groups using ops
Quoted 1 line · Alan Tull
On Tue, Aug 22, 2017 at 10:53 AM, Alan Tull <[email protected]> wrote:
On second thought, this patch is probably not really helpful as it changes the path of the existing sysfs file from
/sys/bus/pci/drivers/altera-cvp/chkcfg
to
/sys/class/fpga_manager/fpga*/chkcfg
which there is no need to do. Patch 1/2 is still fine for manager drivers in the future that are adding device attributes.
Alan
Quoted 88 lines
OK so obviously, this patch doesn't have the necessary header or signoff. Will fix that.
Alan
Quoted 82 lines
--- drivers/fpga/altera-cvp.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-)drivers/fpga/altera-cvp.c +19
-21@@ -354,20 +354,14 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,return ret; } -static const struct fpga_manager_ops altera_cvp_ops = { - .state = altera_cvp_state, - .write_init = altera_cvp_write_init, - .write = altera_cvp_write, - .write_complete = altera_cvp_write_complete, -}; - -static ssize_t show_chkcfg (struct device_driver *dev,char *buf) +static ssize_t show_chkcfg (struct device *dev, +struct device_attribute *attr,char *buf) {return snprintf (buf,3 ,"%d\n" , altera_cvp_chkcfg); } -static ssize_t store_chkcfg (struct device_driver *drv,const char *buf, -size_t count) +static ssize_t store_chkcfg (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) {int ret;@@ -378,7 +372,21 @@ static ssize_t store_chkcfg(struct device_driver *drv, const char *buf,return count; } -static DRIVER_ATTR(chkcfg, 0600, show_chkcfg, store_chkcfg); +static DEVICE_ATTR(chkcfg, 0600, show_chkcfg, store_chkcfg); + +static struct attribute *altera_cvp_attrs[] = { + &dev_attr_chkcfg.attr, +NULL , +}; +ATTRIBUTE_GROUPS (altera_cvp); + +static const struct fpga_manager_ops altera_cvp_ops = { + .state = altera_cvp_state, + .write_init = altera_cvp_write_init, + .write = altera_cvp_write, + .write_complete = altera_cvp_write_complete, + .groups = altera_cvp_groups, +};static int altera_cvp_probe (struct pci_dev *pdev,const struct pci_device_id *dev_id);@@ -459,14 +467,6 @@ static int altera_cvp_probe(struct pci_dev *pdev,if (ret) goto err_unmap; - ret =driver_create_file (&altera_cvp_driver.driver, - &driver_attr_chkcfg); -if (ret) { -dev_err (&pdev->dev,"Can't create sysfs chkcfg file\n" ); -fpga_mgr_unregister (&pdev->dev); - goto err_unmap; - } -return 0 ; err_unmap:@@ -484,7 +484,6 @@ static void altera_cvp_remove(struct pci_dev *pdev)struct altera_cvp_conf *conf = mgr->priv;u16 cmd; -driver_remove_file (&altera_cvp_driver.driver, &driver_attr_chkcfg);fpga_mgr_unregister (&pdev->dev);pci_iounmap (pdev, conf->map);pci_release_region (pdev,CVP_BAR ); --2.7.4