| Message ID | 20230810172210.6338-1-ivan.orlov0322@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] fpga: bridge: make fpga_bridge_class a static const structure | expand |
On 2023-08-10 at 21:22:08 +0400, Ivan Orlov wrote: > Now that the driver core allows for struct class to be in read-only > memory, move the fpga_bridge_class structure to be declared at build > time placing it into read-only memory, instead of having to be > dynamically allocated at boot time. > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com> > --- > drivers/fpga/fpga-bridge.c | 106 ++++++++++++++++++------------------- > 1 file changed, 52 insertions(+), 54 deletions(-) > > diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c > index a6c25dee9cc1..6e38ddaf16cf 100644 > --- a/drivers/fpga/fpga-bridge.c > +++ b/drivers/fpga/fpga-bridge.c > @@ -14,7 +14,6 @@ > #include <linux/spinlock.h> > > static DEFINE_IDA(fpga_bridge_ida); > -static struct class *fpga_bridge_class; Could we still use the forward declaration, to avoid moving too much code block. > > /* Lock for adding/removing bridges to linked lists*/ > static DEFINE_SPINLOCK(bridge_list_lock); > @@ -84,6 +83,53 @@ static struct fpga_bridge *__fpga_bridge_get(struct device *dev, > return ERR_PTR(ret); > } > > +static ssize_t name_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct fpga_bridge *bridge = to_fpga_bridge(dev); > + > + return sprintf(buf, "%s\n", bridge->name); > +} > + > +static ssize_t state_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct fpga_bridge *bridge = to_fpga_bridge(dev); > + int state = 1; > + > + if (bridge->br_ops && bridge->br_ops->enable_show) { > + state = bridge->br_ops->enable_show(bridge); > + if (state < 0) > + return state; > + } > + > + return sysfs_emit(buf, "%s\n", state ? "enabled" : "disabled"); > +} > + > +static DEVICE_ATTR_RO(name); > +static DEVICE_ATTR_RO(state); > + > +static struct attribute *fpga_bridge_attrs[] = { > + &dev_attr_name.attr, > + &dev_attr_state.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(fpga_bridge); > + > +static void fpga_bridge_dev_release(struct device *dev) > +{ > + struct fpga_bridge *bridge = to_fpga_bridge(dev); > + > + ida_free(&fpga_bridge_ida, bridge->dev.id); > + kfree(bridge); > +} > + > +static const struct class fpga_bridge_class = { > + .name = "fpga_bridge", > + .dev_groups = fpga_bridge_groups, > + .dev_release = fpga_bridge_dev_release, > +}; Insert them between __fpga_bridge_get() and of_fpga_bridge_get() is not preferred. See below comments. > + > /** > * of_fpga_bridge_get - get an exclusive reference to an fpga bridge > * > @@ -99,7 +145,7 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, > { > struct device *dev; > > - dev = class_find_device_by_of_node(fpga_bridge_class, np); > + dev = class_find_device_by_of_node(&fpga_bridge_class, np); > if (!dev) > return ERR_PTR(-ENODEV); > > @@ -126,7 +172,7 @@ struct fpga_bridge *fpga_bridge_get(struct device *dev, > { > struct device *bridge_dev; > > - bridge_dev = class_find_device(fpga_bridge_class, NULL, dev, > + bridge_dev = class_find_device(&fpga_bridge_class, NULL, dev, > fpga_bridge_dev_match); > if (!bridge_dev) > return ERR_PTR(-ENODEV); > @@ -281,39 +327,6 @@ int fpga_bridge_get_to_list(struct device *dev, > } > EXPORT_SYMBOL_GPL(fpga_bridge_get_to_list); > > -static ssize_t name_show(struct device *dev, > - struct device_attribute *attr, char *buf) > -{ > - struct fpga_bridge *bridge = to_fpga_bridge(dev); > - > - return sprintf(buf, "%s\n", bridge->name); > -} > - > -static ssize_t state_show(struct device *dev, > - struct device_attribute *attr, char *buf) > -{ > - struct fpga_bridge *bridge = to_fpga_bridge(dev); > - int state = 1; > - > - if (bridge->br_ops && bridge->br_ops->enable_show) { > - state = bridge->br_ops->enable_show(bridge); > - if (state < 0) > - return state; > - } > - > - return sysfs_emit(buf, "%s\n", state ? "enabled" : "disabled"); > -} > - > -static DEVICE_ATTR_RO(name); > -static DEVICE_ATTR_RO(state); > - > -static struct attribute *fpga_bridge_attrs[] = { > - &dev_attr_name.attr, > - &dev_attr_state.attr, > - NULL, > -}; > -ATTRIBUTE_GROUPS(fpga_bridge); > - > /** > * fpga_bridge_register - create and register an FPGA Bridge device > * @parent: FPGA bridge device from pdev > @@ -359,7 +372,7 @@ fpga_bridge_register(struct device *parent, const char *name, > bridge->priv = priv; > > bridge->dev.groups = br_ops->groups; > - bridge->dev.class = fpga_bridge_class; > + bridge->dev.class = &fpga_bridge_class; > bridge->dev.parent = parent; > bridge->dev.of_node = parent->of_node; > bridge->dev.id = id; > @@ -407,29 +420,14 @@ void fpga_bridge_unregister(struct fpga_bridge *bridge) > } > EXPORT_SYMBOL_GPL(fpga_bridge_unregister); > > -static void fpga_bridge_dev_release(struct device *dev) > -{ > - struct fpga_bridge *bridge = to_fpga_bridge(dev); > - > - ida_free(&fpga_bridge_ida, bridge->dev.id); > - kfree(bridge); > -} > - How about put the fpga_bridge_class definition here? Thanks, Yilun > static int __init fpga_bridge_dev_init(void) > { > - fpga_bridge_class = class_create("fpga_bridge"); > - if (IS_ERR(fpga_bridge_class)) > - return PTR_ERR(fpga_bridge_class); > - > - fpga_bridge_class->dev_groups = fpga_bridge_groups; > - fpga_bridge_class->dev_release = fpga_bridge_dev_release; > - > - return 0; > + return class_register(&fpga_bridge_class); > } > > static void __exit fpga_bridge_dev_exit(void) > { > - class_destroy(fpga_bridge_class); > + class_unregister(&fpga_bridge_class); > ida_destroy(&fpga_bridge_ida); > } > > -- > 2.34.1 >
On 8/11/23 07:09, Xu Yilun wrote: > On 2023-08-10 at 21:22:08 +0400, Ivan Orlov wrote: >> Now that the driver core allows for struct class to be in read-only >> memory, move the fpga_bridge_class structure to be declared at build >> time placing it into read-only memory, instead of having to be >> dynamically allocated at boot time. >> >> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> >> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com> >> --- >> drivers/fpga/fpga-bridge.c | 106 ++++++++++++++++++------------------- >> 1 file changed, 52 insertions(+), 54 deletions(-) >> >> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c >> index a6c25dee9cc1..6e38ddaf16cf 100644 >> --- a/drivers/fpga/fpga-bridge.c >> +++ b/drivers/fpga/fpga-bridge.c >> @@ -14,7 +14,6 @@ >> #include <linux/spinlock.h> >> >> static DEFINE_IDA(fpga_bridge_ida); >> -static struct class *fpga_bridge_class; > > Could we still use the forward declaration, to avoid moving too > much code block. > >> >> /* Lock for adding/removing bridges to linked lists*/ >> static DEFINE_SPINLOCK(bridge_list_lock); >> @@ -84,6 +83,53 @@ static struct fpga_bridge *__fpga_bridge_get(struct device *dev, >> return ERR_PTR(ret); >> } >> >> +static ssize_t name_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct fpga_bridge *bridge = to_fpga_bridge(dev); >> + >> + return sprintf(buf, "%s\n", bridge->name); >> +} >> + >> +static ssize_t state_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct fpga_bridge *bridge = to_fpga_bridge(dev); >> + int state = 1; >> + >> + if (bridge->br_ops && bridge->br_ops->enable_show) { >> + state = bridge->br_ops->enable_show(bridge); >> + if (state < 0) >> + return state; >> + } >> + >> + return sysfs_emit(buf, "%s\n", state ? "enabled" : "disabled"); >> +} >> + >> +static DEVICE_ATTR_RO(name); >> +static DEVICE_ATTR_RO(state); >> + >> +static struct attribute *fpga_bridge_attrs[] = { >> + &dev_attr_name.attr, >> + &dev_attr_state.attr, >> + NULL, >> +}; >> +ATTRIBUTE_GROUPS(fpga_bridge); >> + >> +static void fpga_bridge_dev_release(struct device *dev) >> +{ >> + struct fpga_bridge *bridge = to_fpga_bridge(dev); >> + >> + ida_free(&fpga_bridge_ida, bridge->dev.id); >> + kfree(bridge); >> +} >> + >> +static const struct class fpga_bridge_class = { >> + .name = "fpga_bridge", >> + .dev_groups = fpga_bridge_groups, >> + .dev_release = fpga_bridge_dev_release, >> +}; > > Insert them between __fpga_bridge_get() and of_fpga_bridge_get() is not > preferred. See below comments. > >> + >> /** >> * of_fpga_bridge_get - get an exclusive reference to an fpga bridge >> * >> @@ -99,7 +145,7 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, >> { >> struct device *dev; >> >> - dev = class_find_device_by_of_node(fpga_bridge_class, np); >> + dev = class_find_device_by_of_node(&fpga_bridge_class, np); >> if (!dev) >> return ERR_PTR(-ENODEV); >> >> @@ -126,7 +172,7 @@ struct fpga_bridge *fpga_bridge_get(struct device *dev, >> { >> struct device *bridge_dev; >> >> - bridge_dev = class_find_device(fpga_bridge_class, NULL, dev, >> + bridge_dev = class_find_device(&fpga_bridge_class, NULL, dev, >> fpga_bridge_dev_match); >> if (!bridge_dev) >> return ERR_PTR(-ENODEV); >> @@ -281,39 +327,6 @@ int fpga_bridge_get_to_list(struct device *dev, >> } >> EXPORT_SYMBOL_GPL(fpga_bridge_get_to_list); >> >> -static ssize_t name_show(struct device *dev, >> - struct device_attribute *attr, char *buf) >> -{ >> - struct fpga_bridge *bridge = to_fpga_bridge(dev); >> - >> - return sprintf(buf, "%s\n", bridge->name); >> -} >> - >> -static ssize_t state_show(struct device *dev, >> - struct device_attribute *attr, char *buf) >> -{ >> - struct fpga_bridge *bridge = to_fpga_bridge(dev); >> - int state = 1; >> - >> - if (bridge->br_ops && bridge->br_ops->enable_show) { >> - state = bridge->br_ops->enable_show(bridge); >> - if (state < 0) >> - return state; >> - } >> - >> - return sysfs_emit(buf, "%s\n", state ? "enabled" : "disabled"); >> -} >> - >> -static DEVICE_ATTR_RO(name); >> -static DEVICE_ATTR_RO(state); >> - >> -static struct attribute *fpga_bridge_attrs[] = { >> - &dev_attr_name.attr, >> - &dev_attr_state.attr, >> - NULL, >> -}; >> -ATTRIBUTE_GROUPS(fpga_bridge); >> - >> /** >> * fpga_bridge_register - create and register an FPGA Bridge device >> * @parent: FPGA bridge device from pdev >> @@ -359,7 +372,7 @@ fpga_bridge_register(struct device *parent, const char *name, >> bridge->priv = priv; >> >> bridge->dev.groups = br_ops->groups; >> - bridge->dev.class = fpga_bridge_class; >> + bridge->dev.class = &fpga_bridge_class; >> bridge->dev.parent = parent; >> bridge->dev.of_node = parent->of_node; >> bridge->dev.id = id; >> @@ -407,29 +420,14 @@ void fpga_bridge_unregister(struct fpga_bridge *bridge) >> } >> EXPORT_SYMBOL_GPL(fpga_bridge_unregister); >> >> -static void fpga_bridge_dev_release(struct device *dev) >> -{ >> - struct fpga_bridge *bridge = to_fpga_bridge(dev); >> - >> - ida_free(&fpga_bridge_ida, bridge->dev.id); >> - kfree(bridge); >> -} >> - > > How about put the fpga_bridge_class definition here? > > Thanks, > Yilun Hi Yilun, Thank you for the review, I agree that forward declaration is better in this case. I'll send you the V2 after making suggested changes. -- Kind regards, Ivan Orlov
diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c index a6c25dee9cc1..6e38ddaf16cf 100644 --- a/drivers/fpga/fpga-bridge.c +++ b/drivers/fpga/fpga-bridge.c @@ -14,7 +14,6 @@ #include <linux/spinlock.h> static DEFINE_IDA(fpga_bridge_ida); -static struct class *fpga_bridge_class; /* Lock for adding/removing bridges to linked lists*/ static DEFINE_SPINLOCK(bridge_list_lock); @@ -84,6 +83,53 @@ static struct fpga_bridge *__fpga_bridge_get(struct device *dev, return ERR_PTR(ret); } +static ssize_t name_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct fpga_bridge *bridge = to_fpga_bridge(dev); + + return sprintf(buf, "%s\n", bridge->name); +} + +static ssize_t state_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct fpga_bridge *bridge = to_fpga_bridge(dev); + int state = 1; + + if (bridge->br_ops && bridge->br_ops->enable_show) { + state = bridge->br_ops->enable_show(bridge); + if (state < 0) + return state; + } + + return sysfs_emit(buf, "%s\n", state ? "enabled" : "disabled"); +} + +static DEVICE_ATTR_RO(name); +static DEVICE_ATTR_RO(state); + +static struct attribute *fpga_bridge_attrs[] = { + &dev_attr_name.attr, + &dev_attr_state.attr, + NULL, +}; +ATTRIBUTE_GROUPS(fpga_bridge); + +static void fpga_bridge_dev_release(struct device *dev) +{ + struct fpga_bridge *bridge = to_fpga_bridge(dev); + + ida_free(&fpga_bridge_ida, bridge->dev.id); + kfree(bridge); +} + +static const struct class fpga_bridge_class = { + .name = "fpga_bridge", + .dev_groups = fpga_bridge_groups, + .dev_release = fpga_bridge_dev_release, +}; + /** * of_fpga_bridge_get - get an exclusive reference to an fpga bridge * @@ -99,7 +145,7 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, { struct device *dev; - dev = class_find_device_by_of_node(fpga_bridge_class, np); + dev = class_find_device_by_of_node(&fpga_bridge_class, np); if (!dev) return ERR_PTR(-ENODEV); @@ -126,7 +172,7 @@ struct fpga_bridge *fpga_bridge_get(struct device *dev, { struct device *bridge_dev; - bridge_dev = class_find_device(fpga_bridge_class, NULL, dev, + bridge_dev = class_find_device(&fpga_bridge_class, NULL, dev, fpga_bridge_dev_match); if (!bridge_dev) return ERR_PTR(-ENODEV); @@ -281,39 +327,6 @@ int fpga_bridge_get_to_list(struct device *dev, } EXPORT_SYMBOL_GPL(fpga_bridge_get_to_list); -static ssize_t name_show(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct fpga_bridge *bridge = to_fpga_bridge(dev); - - return sprintf(buf, "%s\n", bridge->name); -} - -static ssize_t state_show(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct fpga_bridge *bridge = to_fpga_bridge(dev); - int state = 1; - - if (bridge->br_ops && bridge->br_ops->enable_show) { - state = bridge->br_ops->enable_show(bridge); - if (state < 0) - return state; - } - - return sysfs_emit(buf, "%s\n", state ? "enabled" : "disabled"); -} - -static DEVICE_ATTR_RO(name); -static DEVICE_ATTR_RO(state); - -static struct attribute *fpga_bridge_attrs[] = { - &dev_attr_name.attr, - &dev_attr_state.attr, - NULL, -}; -ATTRIBUTE_GROUPS(fpga_bridge); - /** * fpga_bridge_register - create and register an FPGA Bridge device * @parent: FPGA bridge device from pdev @@ -359,7 +372,7 @@ fpga_bridge_register(struct device *parent, const char *name, bridge->priv = priv; bridge->dev.groups = br_ops->groups; - bridge->dev.class = fpga_bridge_class; + bridge->dev.class = &fpga_bridge_class; bridge->dev.parent = parent; bridge->dev.of_node = parent->of_node; bridge->dev.id = id; @@ -407,29 +420,14 @@ void fpga_bridge_unregister(struct fpga_bridge *bridge) } EXPORT_SYMBOL_GPL(fpga_bridge_unregister); -static void fpga_bridge_dev_release(struct device *dev) -{ - struct fpga_bridge *bridge = to_fpga_bridge(dev); - - ida_free(&fpga_bridge_ida, bridge->dev.id); - kfree(bridge); -} - static int __init fpga_bridge_dev_init(void) { - fpga_bridge_class = class_create("fpga_bridge"); - if (IS_ERR(fpga_bridge_class)) - return PTR_ERR(fpga_bridge_class); - - fpga_bridge_class->dev_groups = fpga_bridge_groups; - fpga_bridge_class->dev_release = fpga_bridge_dev_release; - - return 0; + return class_register(&fpga_bridge_class); } static void __exit fpga_bridge_dev_exit(void) { - class_destroy(fpga_bridge_class); + class_unregister(&fpga_bridge_class); ida_destroy(&fpga_bridge_ida); }
Now that the driver core allows for struct class to be in read-only memory, move the fpga_bridge_class structure to be declared at build time placing it into read-only memory, instead of having to be dynamically allocated at boot time. Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com> --- drivers/fpga/fpga-bridge.c | 106 ++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 54 deletions(-)