2024-02-20 18:46:21

by Ricardo B. Marliere

[permalink] [raw]
Subject: [PATCH] power: supply: sysfs: constify the struct device_type usage

Since commit aed65af1cc2f ("drivers: make device_type const"), the driver
core can properly handle constant struct device_type. Move the
power_supply_dev_type variable to be a constant structure as well, placing
it into read-only memory which can not be modified at runtime.

Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Ricardo B. Marliere <[email protected]>
---
drivers/power/supply/power_supply.h | 4 ++--
drivers/power/supply/power_supply_core.c | 2 +-
drivers/power/supply/power_supply_sysfs.c | 9 ++++++---
3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 645eee4d6b6a..d547dbe5676f 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -15,12 +15,12 @@ struct power_supply;

#ifdef CONFIG_SYSFS

-extern void power_supply_init_attrs(struct device_type *dev_type);
+extern void power_supply_init_attrs(const struct device_type *dev_type);
extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);

#else

-static inline void power_supply_init_attrs(struct device_type *dev_type) {}
+static inline void power_supply_init_attrs(const struct device_type *dev_type) {}
#define power_supply_uevent NULL

#endif /* CONFIG_SYSFS */
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index ecef35ac3b7e..fda21cf4111c 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -31,7 +31,7 @@ EXPORT_SYMBOL_GPL(power_supply_class);

static BLOCKING_NOTIFIER_HEAD(power_supply_notifier);

-static struct device_type power_supply_dev_type;
+static const struct device_type power_supply_dev_type;

#define POWER_SUPPLY_DEFERRED_REGISTER_TIME msecs_to_jiffies(10)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 977611e16373..ed365ca54c90 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -399,12 +399,15 @@ static const struct attribute_group *power_supply_attr_groups[] = {
NULL,
};

-void power_supply_init_attrs(struct device_type *dev_type)
+static const struct device_type power_supply_dev_type = {
+ .name = "power_supply",
+ .groups = power_supply_attr_groups,
+};
+
+void power_supply_init_attrs(const struct device_type *dev_type)
{
int i;

- dev_type->groups = power_supply_attr_groups;
-
for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
struct device_attribute *attr;


---
base-commit: a9b254892ce1a447b06c5019cbf0e9caeb48c138
change-id: 20240220-device_cleanup-power-037594022cb1

Best regards,
--
Ricardo B. Marliere <[email protected]>



2024-02-21 20:46:26

by Sebastian Reichel

[permalink] [raw]
Subject: Re: [PATCH] power: supply: sysfs: constify the struct device_type usage

Hi,

On Tue, Feb 20, 2024 at 03:40:06PM -0300, Ricardo B. Marliere wrote:
> Since commit aed65af1cc2f ("drivers: make device_type const"), the driver
> core can properly handle constant struct device_type. Move the
> power_supply_dev_type variable to be a constant structure as well, placing
> it into read-only memory which can not be modified at runtime.
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Ricardo B. Marliere <[email protected]>
> ---
> drivers/power/supply/power_supply.h | 4 ++--
> drivers/power/supply/power_supply_core.c | 2 +-
> drivers/power/supply/power_supply_sysfs.c | 9 ++++++---
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 645eee4d6b6a..d547dbe5676f 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -15,12 +15,12 @@ struct power_supply;
>
> #ifdef CONFIG_SYSFS
>
> -extern void power_supply_init_attrs(struct device_type *dev_type);
> +extern void power_supply_init_attrs(const struct device_type *dev_type);
> extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);
>
> #else
>
> -static inline void power_supply_init_attrs(struct device_type *dev_type) {}
> +static inline void power_supply_init_attrs(const struct device_type *dev_type) {}
> #define power_supply_uevent NULL
>
> #endif /* CONFIG_SYSFS */
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index ecef35ac3b7e..fda21cf4111c 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -31,7 +31,7 @@ EXPORT_SYMBOL_GPL(power_supply_class);
>
> static BLOCKING_NOTIFIER_HEAD(power_supply_notifier);
>
> -static struct device_type power_supply_dev_type;
> +static const struct device_type power_supply_dev_type;

This creates an empty struct, which is being used in this file...

>
> #define POWER_SUPPLY_DEFERRED_REGISTER_TIME msecs_to_jiffies(10)
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 977611e16373..ed365ca54c90 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -399,12 +399,15 @@ static const struct attribute_group *power_supply_attr_groups[] = {
> NULL,
> };
>
> -void power_supply_init_attrs(struct device_type *dev_type)
> +static const struct device_type power_supply_dev_type = {
> + .name = "power_supply",
> + .groups = power_supply_attr_groups,
> +};

... and this creates the correct one in power_supply_sysfs.c, but it
is not being used at all. Maybe get some sleep and/or read again
what 'static' means for a global variable?

> +void power_supply_init_attrs(const struct device_type *dev_type)
> {

This function no longer uses dev_type argument, so you can remove
it.

-- Sebastian

> int i;
>
> - dev_type->groups = power_supply_attr_groups;
> -

> for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
> struct device_attribute *attr;
>
>
> ---
> base-commit: a9b254892ce1a447b06c5019cbf0e9caeb48c138
> change-id: 20240220-device_cleanup-power-037594022cb1
>
> Best regards,
> --
> Ricardo B. Marliere <[email protected]>
>


Attachments:
(No filename) (3.36 kB)
signature.asc (849.00 B)
Download all attachments

2024-02-22 21:17:28

by Ricardo B. Marliere

[permalink] [raw]
Subject: Re: [PATCH] power: supply: sysfs: constify the struct device_type usage

Hi Sebastian,

On 21 Feb 21:46, Sebastian Reichel wrote:
> Hi,
>
> On Tue, Feb 20, 2024 at 03:40:06PM -0300, Ricardo B. Marliere wrote:
> > Since commit aed65af1cc2f ("drivers: make device_type const"), the driver
> > core can properly handle constant struct device_type. Move the
> > power_supply_dev_type variable to be a constant structure as well, placing
> > it into read-only memory which can not be modified at runtime.
> >
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Signed-off-by: Ricardo B. Marliere <[email protected]>
> > ---
> > drivers/power/supply/power_supply.h | 4 ++--
> > drivers/power/supply/power_supply_core.c | 2 +-
> > drivers/power/supply/power_supply_sysfs.c | 9 ++++++---
> > 3 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> > index 645eee4d6b6a..d547dbe5676f 100644
> > --- a/drivers/power/supply/power_supply.h
> > +++ b/drivers/power/supply/power_supply.h
> > @@ -15,12 +15,12 @@ struct power_supply;
> >
> > #ifdef CONFIG_SYSFS
> >
> > -extern void power_supply_init_attrs(struct device_type *dev_type);
> > +extern void power_supply_init_attrs(const struct device_type *dev_type);
> > extern int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env);
> >
> > #else
> >
> > -static inline void power_supply_init_attrs(struct device_type *dev_type) {}
> > +static inline void power_supply_init_attrs(const struct device_type *dev_type) {}
> > #define power_supply_uevent NULL
> >
> > #endif /* CONFIG_SYSFS */
> > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> > index ecef35ac3b7e..fda21cf4111c 100644
> > --- a/drivers/power/supply/power_supply_core.c
> > +++ b/drivers/power/supply/power_supply_core.c
> > @@ -31,7 +31,7 @@ EXPORT_SYMBOL_GPL(power_supply_class);
> >
> > static BLOCKING_NOTIFIER_HEAD(power_supply_notifier);
> >
> > -static struct device_type power_supply_dev_type;
> > +static const struct device_type power_supply_dev_type;
>
> This creates an empty struct, which is being used in this file...
>
> >
> > #define POWER_SUPPLY_DEFERRED_REGISTER_TIME msecs_to_jiffies(10)
> >
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index 977611e16373..ed365ca54c90 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -399,12 +399,15 @@ static const struct attribute_group *power_supply_attr_groups[] = {
> > NULL,
> > };
> >
> > -void power_supply_init_attrs(struct device_type *dev_type)
> > +static const struct device_type power_supply_dev_type = {
> > + .name = "power_supply",
> > + .groups = power_supply_attr_groups,
> > +};
>
> ... and this creates the correct one in power_supply_sysfs.c, but it
> is not being used at all. Maybe get some sleep and/or read again
> what 'static' means for a global variable?

Oh, silly me. This was a stupid patch indeed, sorry about that. I'll
send a proper fix later!

Thanks for reviewing,
- Ricardo.


>
> > +void power_supply_init_attrs(const struct device_type *dev_type)
> > {
>
> This function no longer uses dev_type argument, so you can remove
> it.
>
> -- Sebastian
>
> > int i;
> >
> > - dev_type->groups = power_supply_attr_groups;
> > -
>
> > for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
> > struct device_attribute *attr;
> >
> >
> > ---
> > base-commit: a9b254892ce1a447b06c5019cbf0e9caeb48c138
> > change-id: 20240220-device_cleanup-power-037594022cb1
> >
> > Best regards,
> > --
> > Ricardo B. Marliere <[email protected]>
> >