2023-08-10 17:48:44

by Ivan Orlov

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

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 <[email protected]>

Signed-off-by: Ivan Orlov <[email protected]>
---
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;

/* 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);
}

--
2.34.1

2023-08-10 18:24:27

by Ivan Orlov

Subject: [PATCH 3/3] fpga: region: make fpga_region_class a static const structure

Now that the driver core allows for struct class to be in read-only
memory, move the fpga_region_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 <[email protected]>
Signed-off-by: Ivan Orlov <[email protected]>
---
drivers/fpga/fpga-region.c | 64 ++++++++++++++++++--------------------
1 file changed, 31 insertions(+), 33 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index ccf6fdab1360..01cf4c2f83d1 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -16,21 +16,6 @@
#include <linux/spinlock.h>

static DEFINE_IDA(fpga_region_ida);
-static struct class *fpga_region_class;
-
-struct fpga_region *
-fpga_region_class_find(struct device *start, const void *data,
- int (*match)(struct device *, const void *))
-{
- struct device *dev;
-
- dev = class_find_device(fpga_region_class, start, data, match);
- if (!dev)
- return NULL;
-
- return to_fpga_region(dev);
-}
-EXPORT_SYMBOL_GPL(fpga_region_class_find);

/**
* fpga_region_get - get an exclusive reference to an fpga region
@@ -179,6 +164,34 @@ static struct attribute *fpga_region_attrs[] = {
};
ATTRIBUTE_GROUPS(fpga_region);

+static void fpga_region_dev_release(struct device *dev)
+{
+ struct fpga_region *region = to_fpga_region(dev);
+
+ ida_free(&fpga_region_ida, region->dev.id);
+ kfree(region);
+}
+
+static const struct class fpga_region_class = {
+ .name = "fpga_region",
+ .dev_groups = fpga_region_groups,
+ .dev_release = fpga_region_dev_release,
+};
+
+struct fpga_region *
+fpga_region_class_find(struct device *start, const void *data,
+ int (*match)(struct device *, const void *))
+{
+ struct device *dev;
+
+ dev = class_find_device(&fpga_region_class, start, data, match);
+ if (!dev)
+ return NULL;
+
+ return to_fpga_region(dev);
+}
+EXPORT_SYMBOL_GPL(fpga_region_class_find);
+
/**
* fpga_region_register_full - create and register an FPGA Region device
* @parent: device parent
@@ -216,7 +229,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
mutex_init(&region->mutex);
INIT_LIST_HEAD(&region->bridge_list);

- region->dev.class = fpga_region_class;
+ region->dev.class = &fpga_region_class;
region->dev.parent = parent;
region->dev.of_node = parent->of_node;
region->dev.id = id;
@@ -279,33 +292,18 @@ void fpga_region_unregister(struct fpga_region *region)
}
EXPORT_SYMBOL_GPL(fpga_region_unregister);

-static void fpga_region_dev_release(struct device *dev)
-{
- struct fpga_region *region = to_fpga_region(dev);
-
- ida_free(&fpga_region_ida, region->dev.id);
- kfree(region);
-}
-
/**
* fpga_region_init - init function for fpga_region class
* Creates the fpga_region class and registers a reconfig notifier.
*/
static int __init fpga_region_init(void)
{
- fpga_region_class = class_create("fpga_region");
- if (IS_ERR(fpga_region_class))
- return PTR_ERR(fpga_region_class);
-
- fpga_region_class->dev_groups = fpga_region_groups;
- fpga_region_class->dev_release = fpga_region_dev_release;
-
- return 0;
+ return class_register(&fpga_region_class);
}

static void __exit fpga_region_exit(void)
{
- class_destroy(fpga_region_class);
+ class_unregister(&fpga_region_class);
ida_destroy(&fpga_region_ida);
}

--
2.34.1

2023-08-10 18:25:43

by Ivan Orlov

Subject: [PATCH 2/3] fpga: fpga-mgr: make fpga_mgr_class a static const structure

Now that the driver core allows for struct class to be in read-only
memory, move the fpga_mgr_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 <[email protected]>
Signed-off-by: Ivan Orlov <[email protected]>
---
drivers/fpga/fpga-mgr.c | 40 +++++++++++++++++++---------------------
1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index eb583f86a0b9..cd5b7371495b 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -19,7 +19,6 @@
#include <linux/highmem.h>

static DEFINE_IDA(fpga_mgr_ida);
-static struct class *fpga_mgr_class;

struct fpga_mgr_devres {
struct fpga_manager *mgr;
@@ -664,6 +663,20 @@ static struct attribute *fpga_mgr_attrs[] = {
};
ATTRIBUTE_GROUPS(fpga_mgr);

+static void fpga_mgr_dev_release(struct device *dev)
+{
+ struct fpga_manager *mgr = to_fpga_manager(dev);
+
+ ida_free(&fpga_mgr_ida, mgr->dev.id);
+ kfree(mgr);
+}
+
+static const struct class fpga_mgr_class = {
+ .name = "fpga_manager",
+ .dev_groups = fpga_mgr_groups,
+ .dev_release = fpga_mgr_dev_release,
+};
+
static struct fpga_manager *__fpga_mgr_get(struct device *dev)
{
struct fpga_manager *mgr;
@@ -693,7 +706,7 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data)
*/
struct fpga_manager *fpga_mgr_get(struct device *dev)
{
- struct device *mgr_dev = class_find_device(fpga_mgr_class, NULL, dev,
+ struct device *mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev,
fpga_mgr_dev_match);
if (!mgr_dev)
return ERR_PTR(-ENODEV);
@@ -713,7 +726,7 @@ struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
{
struct device *dev;

- dev = class_find_device_by_of_node(fpga_mgr_class, node);
+ dev = class_find_device_by_of_node(&fpga_mgr_class, node);
if (!dev)
return ERR_PTR(-ENODEV);

@@ -809,7 +822,7 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in
mgr->priv = info->priv;
mgr->compat_id = info->compat_id;

- mgr->dev.class = fpga_mgr_class;
+ mgr->dev.class = &fpga_mgr_class;
mgr->dev.groups = mops->groups;
mgr->dev.parent = parent;
mgr->dev.of_node = parent->of_node;
@@ -959,31 +972,16 @@ devm_fpga_mgr_register(struct device *parent, const char *name,
}
EXPORT_SYMBOL_GPL(devm_fpga_mgr_register);

-static void fpga_mgr_dev_release(struct device *dev)
-{
- struct fpga_manager *mgr = to_fpga_manager(dev);
-
- ida_free(&fpga_mgr_ida, mgr->dev.id);
- kfree(mgr);
-}
-
static int __init fpga_mgr_class_init(void)
{
pr_info("FPGA manager framework\n");

- fpga_mgr_class = class_create("fpga_manager");
- if (IS_ERR(fpga_mgr_class))
- return PTR_ERR(fpga_mgr_class);
-
- fpga_mgr_class->dev_groups = fpga_mgr_groups;
- fpga_mgr_class->dev_release = fpga_mgr_dev_release;
-
- return 0;
+ return class_register(&fpga_mgr_class);
}

static void __exit fpga_mgr_class_exit(void)
{
- class_destroy(fpga_mgr_class);
+ class_unregister(&fpga_mgr_class);
ida_destroy(&fpga_mgr_ida);
}

--
2.34.1

2023-08-11 03:42:39

by Xu Yilun

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 <[email protected]>
>
> Signed-off-by: Ivan Orlov <[email protected]>
> ---
> 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
>

2023-08-11 03:45:11

by Xu Yilun

Subject: Re: [PATCH 3/3] fpga: region: make fpga_region_class a static const structure

On 2023-08-10 at 21:22:10 +0400, Ivan Orlov wrote:
> Now that the driver core allows for struct class to be in read-only
> memory, move the fpga_region_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 <[email protected]>
> Signed-off-by: Ivan Orlov <[email protected]>
> ---
> drivers/fpga/fpga-region.c | 64 ++++++++++++++++++--------------------
> 1 file changed, 31 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index ccf6fdab1360..01cf4c2f83d1 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -16,21 +16,6 @@
> #include <linux/spinlock.h>
>
> static DEFINE_IDA(fpga_region_ida);
> -static struct class *fpga_region_class;
> -
> -struct fpga_region *
> -fpga_region_class_find(struct device *start, const void *data,
> - int (*match)(struct device *, const void *))
> -{
> - struct device *dev;
> -
> - dev = class_find_device(fpga_region_class, start, data, match);
> - if (!dev)
> - return NULL;
> -
> - return to_fpga_region(dev);
> -}
> -EXPORT_SYMBOL_GPL(fpga_region_class_find);
>
> /**
> * fpga_region_get - get an exclusive reference to an fpga region
> @@ -179,6 +164,34 @@ static struct attribute *fpga_region_attrs[] = {
> };
> ATTRIBUTE_GROUPS(fpga_region);
>
> +static void fpga_region_dev_release(struct device *dev)
> +{
> + struct fpga_region *region = to_fpga_region(dev);
> +
> + ida_free(&fpga_region_ida, region->dev.id);
> + kfree(region);
> +}
> +
> +static const struct class fpga_region_class = {
> + .name = "fpga_region",
> + .dev_groups = fpga_region_groups,
> + .dev_release = fpga_region_dev_release,
> +};

Same concern as previous patches。

Thanks,
Yilun

> +
> +struct fpga_region *
> +fpga_region_class_find(struct device *start, const void *data,
> + int (*match)(struct device *, const void *))
> +{
> + struct device *dev;
> +
> + dev = class_find_device(&fpga_region_class, start, data, match);
> + if (!dev)
> + return NULL;
> +
> + return to_fpga_region(dev);
> +}
> +EXPORT_SYMBOL_GPL(fpga_region_class_find);
> +
> /**
> * fpga_region_register_full - create and register an FPGA Region device
> * @parent: device parent
> @@ -216,7 +229,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
> mutex_init(&region->mutex);
> INIT_LIST_HEAD(&region->bridge_list);
>
> - region->dev.class = fpga_region_class;
> + region->dev.class = &fpga_region_class;
> region->dev.parent = parent;
> region->dev.of_node = parent->of_node;
> region->dev.id = id;
> @@ -279,33 +292,18 @@ void fpga_region_unregister(struct fpga_region *region)
> }
> EXPORT_SYMBOL_GPL(fpga_region_unregister);
>
> -static void fpga_region_dev_release(struct device *dev)
> -{
> - struct fpga_region *region = to_fpga_region(dev);
> -
> - ida_free(&fpga_region_ida, region->dev.id);
> - kfree(region);
> -}
> -
> /**
> * fpga_region_init - init function for fpga_region class
> * Creates the fpga_region class and registers a reconfig notifier.
> */
> static int __init fpga_region_init(void)
> {
> - fpga_region_class = class_create("fpga_region");
> - if (IS_ERR(fpga_region_class))
> - return PTR_ERR(fpga_region_class);
> -
> - fpga_region_class->dev_groups = fpga_region_groups;
> - fpga_region_class->dev_release = fpga_region_dev_release;
> -
> - return 0;
> + return class_register(&fpga_region_class);
> }
>
> static void __exit fpga_region_exit(void)
> {
> - class_destroy(fpga_region_class);
> + class_unregister(&fpga_region_class);
> ida_destroy(&fpga_region_ida);
> }
>
> --
> 2.34.1
>

2023-08-11 03:52:44

by Xu Yilun

Subject: Re: [PATCH 2/3] fpga: fpga-mgr: make fpga_mgr_class a static const structure

On 2023-08-10 at 21:22:09 +0400, Ivan Orlov wrote:
> Now that the driver core allows for struct class to be in read-only
> memory, move the fpga_mgr_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 <[email protected]>
> Signed-off-by: Ivan Orlov <[email protected]>
> ---
> drivers/fpga/fpga-mgr.c | 40 +++++++++++++++++++---------------------
> 1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index eb583f86a0b9..cd5b7371495b 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -19,7 +19,6 @@
> #include <linux/highmem.h>
>
> static DEFINE_IDA(fpga_mgr_ida);
> -static struct class *fpga_mgr_class;
>
> struct fpga_mgr_devres {
> struct fpga_manager *mgr;
> @@ -664,6 +663,20 @@ static struct attribute *fpga_mgr_attrs[] = {
> };
> ATTRIBUTE_GROUPS(fpga_mgr);
>
> +static void fpga_mgr_dev_release(struct device *dev)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> + ida_free(&fpga_mgr_ida, mgr->dev.id);
> + kfree(mgr);
> +}
> +
> +static const struct class fpga_mgr_class = {
> + .name = "fpga_manager",
> + .dev_groups = fpga_mgr_groups,
> + .dev_release = fpga_mgr_dev_release,
> +};

Same concern as fpga_bridge, keep the forward declaration and put the
definiton below.

Thanks,
Yilun

> +
> static struct fpga_manager *__fpga_mgr_get(struct device *dev)
> {
> struct fpga_manager *mgr;
> @@ -693,7 +706,7 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data)
> */
> struct fpga_manager *fpga_mgr_get(struct device *dev)
> {
> - struct device *mgr_dev = class_find_device(fpga_mgr_class, NULL, dev,
> + struct device *mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev,
> fpga_mgr_dev_match);
> if (!mgr_dev)
> return ERR_PTR(-ENODEV);
> @@ -713,7 +726,7 @@ struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
> {
> struct device *dev;
>
> - dev = class_find_device_by_of_node(fpga_mgr_class, node);
> + dev = class_find_device_by_of_node(&fpga_mgr_class, node);
> if (!dev)
> return ERR_PTR(-ENODEV);
>
> @@ -809,7 +822,7 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in
> mgr->priv = info->priv;
> mgr->compat_id = info->compat_id;
>
> - mgr->dev.class = fpga_mgr_class;
> + mgr->dev.class = &fpga_mgr_class;
> mgr->dev.groups = mops->groups;
> mgr->dev.parent = parent;
> mgr->dev.of_node = parent->of_node;
> @@ -959,31 +972,16 @@ devm_fpga_mgr_register(struct device *parent, const char *name,
> }
> EXPORT_SYMBOL_GPL(devm_fpga_mgr_register);
>
> -static void fpga_mgr_dev_release(struct device *dev)
> -{
> - struct fpga_manager *mgr = to_fpga_manager(dev);
> -
> - ida_free(&fpga_mgr_ida, mgr->dev.id);
> - kfree(mgr);
> -}
> -
> static int __init fpga_mgr_class_init(void)
> {
> pr_info("FPGA manager framework\n");
>
> - fpga_mgr_class = class_create("fpga_manager");
> - if (IS_ERR(fpga_mgr_class))
> - return PTR_ERR(fpga_mgr_class);
> -
> - fpga_mgr_class->dev_groups = fpga_mgr_groups;
> - fpga_mgr_class->dev_release = fpga_mgr_dev_release;
> -
> - return 0;
> + return class_register(&fpga_mgr_class);
> }
>
> static void __exit fpga_mgr_class_exit(void)
> {
> - class_destroy(fpga_mgr_class);
> + class_unregister(&fpga_mgr_class);
> ida_destroy(&fpga_mgr_ida);
> }
>
> --
> 2.34.1
>

2023-08-11 08:54:03

by Ivan Orlov

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

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 <[email protected]>
>>
>> Signed-off-by: Ivan Orlov <[email protected]>
>> ---
>> 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