Re: [PATCH 1/3] fpga: bridge: make fpga_bridge_class a static const structure

[Ivan Orlov]

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@xxxxxxxxxxxxxxxxxxx>
> >
> > Signed-off-by: Ivan Orlov <ivan.orlov0322@xxxxxxxxx>
> > ---
> >   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