Message-ID: <ZNWmhLCeEu07AMPK@yilunxu-OptiPlex-7050>
Date:   Fri, 11 Aug 2023 11:09:56 +0800
From:   Xu Yilun <yilun.xu@...el.com>
To:     Ivan Orlov <ivan.orlov0322@...il.com>
Cc:     mdf@...nel.org, hao.wu@...el.com, trix@...hat.com,
        linux-fpga@...r.kernel.org, linux-kernel@...r.kernel.org,
        gregkh@...uxfoundation.org
Subject: Re: [PATCH 1/3] fpga: bridge: make fpga_bridge_class a static const
 structure

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@...uxfoundation.org>
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@...il.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
> 

Powered by blists - more mailing lists