Since commit aed65af1cc2f ("drivers: make device_type const"), the driver core can properly handle constant struct device_type. Move the greybus_hd_type, greybus_module_type, greybus_interface_type, greybus_control_type, greybus_bundle_type and greybus_svc_type variables to be constant structures as well, placing it into read-only memory which can not be modified at runtime.
Signed-off-by: Ricardo B. Marliere ricardo@marliere.net --- drivers/greybus/bundle.c | 2 +- drivers/greybus/control.c | 2 +- drivers/greybus/hd.c | 2 +- drivers/greybus/interface.c | 2 +- drivers/greybus/module.c | 2 +- drivers/greybus/svc.c | 2 +- include/linux/greybus.h | 12 ++++++------ 7 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/greybus/bundle.c b/drivers/greybus/bundle.c index 84660729538b..a6e1cca06172 100644 --- a/drivers/greybus/bundle.c +++ b/drivers/greybus/bundle.c @@ -166,7 +166,7 @@ static const struct dev_pm_ops gb_bundle_pm_ops = { SET_RUNTIME_PM_OPS(gb_bundle_suspend, gb_bundle_resume, gb_bundle_idle) };
-struct device_type greybus_bundle_type = { +const struct device_type greybus_bundle_type = { .name = "greybus_bundle", .release = gb_bundle_release, .pm = &gb_bundle_pm_ops, diff --git a/drivers/greybus/control.c b/drivers/greybus/control.c index 359a25841973..b5cf49d09df2 100644 --- a/drivers/greybus/control.c +++ b/drivers/greybus/control.c @@ -436,7 +436,7 @@ static void gb_control_release(struct device *dev) kfree(control); }
-struct device_type greybus_control_type = { +const struct device_type greybus_control_type = { .name = "greybus_control", .release = gb_control_release, }; diff --git a/drivers/greybus/hd.c b/drivers/greybus/hd.c index 72b21bf2d7d3..e2f3496bddc3 100644 --- a/drivers/greybus/hd.c +++ b/drivers/greybus/hd.c @@ -116,7 +116,7 @@ static void gb_hd_release(struct device *dev) kfree(hd); }
-struct device_type greybus_hd_type = { +const struct device_type greybus_hd_type = { .name = "greybus_host_device", .release = gb_hd_release, }; diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c index 9ec949a438ef..a88dc701289c 100644 --- a/drivers/greybus/interface.c +++ b/drivers/greybus/interface.c @@ -765,7 +765,7 @@ static const struct dev_pm_ops gb_interface_pm_ops = { gb_interface_runtime_idle) };
-struct device_type greybus_interface_type = { +const struct device_type greybus_interface_type = { .name = "greybus_interface", .release = gb_interface_release, .pm = &gb_interface_pm_ops, diff --git a/drivers/greybus/module.c b/drivers/greybus/module.c index 36f77f9e1d74..7f7153a1dd60 100644 --- a/drivers/greybus/module.c +++ b/drivers/greybus/module.c @@ -81,7 +81,7 @@ static void gb_module_release(struct device *dev) kfree(module); }
-struct device_type greybus_module_type = { +const struct device_type greybus_module_type = { .name = "greybus_module", .release = gb_module_release, }; diff --git a/drivers/greybus/svc.c b/drivers/greybus/svc.c index 0d7e749174a4..4256467fcd35 100644 --- a/drivers/greybus/svc.c +++ b/drivers/greybus/svc.c @@ -1305,7 +1305,7 @@ static void gb_svc_release(struct device *dev) kfree(svc); }
-struct device_type greybus_svc_type = { +const struct device_type greybus_svc_type = { .name = "greybus_svc", .release = gb_svc_release, }; diff --git a/include/linux/greybus.h b/include/linux/greybus.h index 18c0fb958b74..5f9791fae3c0 100644 --- a/include/linux/greybus.h +++ b/include/linux/greybus.h @@ -106,12 +106,12 @@ struct dentry *gb_debugfs_get(void);
extern struct bus_type greybus_bus_type;
-extern struct device_type greybus_hd_type; -extern struct device_type greybus_module_type; -extern struct device_type greybus_interface_type; -extern struct device_type greybus_control_type; -extern struct device_type greybus_bundle_type; -extern struct device_type greybus_svc_type; +extern const struct device_type greybus_hd_type; +extern const struct device_type greybus_module_type; +extern const struct device_type greybus_interface_type; +extern const struct device_type greybus_control_type; +extern const struct device_type greybus_bundle_type; +extern const struct device_type greybus_svc_type;
static inline int is_gb_host_device(const struct device *dev) {
--- base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d change-id: 20240219-device_cleanup-greybus-c97c1ef52458
Best regards,
On 2/19/24 6:40 AM, Ricardo B. Marliere wrote:
Since commit aed65af1cc2f ("drivers: make device_type const"), the driver core can properly handle constant struct device_type. Move the greybus_hd_type, greybus_module_type, greybus_interface_type, greybus_control_type, greybus_bundle_type and greybus_svc_type variables to be constant structures as well, placing it into read-only memory which can not be modified at runtime.
Signed-off-by: Ricardo B. Marliere ricardo@marliere.net
This looks good to me. Assuming it compiles cleanly:
Reviewed-by: Alex Elder elder@linaro.org
On another subject:
Johan might disagree, but I think it would be nice to make the definitions of the Greybus device types as static (private) and make the is_gb_host_device() etc. functions real functions rather than static inlines in <linux/greybus.h>.
It turns out that all of the is_gb_*() functions are called only from drivers/greybus/core.c; they could all be moved there rather than advertising them in <linux/greybus.h>.
-Alex
drivers/greybus/bundle.c | 2 +- drivers/greybus/control.c | 2 +- drivers/greybus/hd.c | 2 +- drivers/greybus/interface.c | 2 +- drivers/greybus/module.c | 2 +- drivers/greybus/svc.c | 2 +- include/linux/greybus.h | 12 ++++++------ 7 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/greybus/bundle.c b/drivers/greybus/bundle.c index 84660729538b..a6e1cca06172 100644 --- a/drivers/greybus/bundle.c +++ b/drivers/greybus/bundle.c @@ -166,7 +166,7 @@ static const struct dev_pm_ops gb_bundle_pm_ops = { SET_RUNTIME_PM_OPS(gb_bundle_suspend, gb_bundle_resume, gb_bundle_idle) }; -struct device_type greybus_bundle_type = { +const struct device_type greybus_bundle_type = { .name = "greybus_bundle", .release = gb_bundle_release, .pm = &gb_bundle_pm_ops, diff --git a/drivers/greybus/control.c b/drivers/greybus/control.c index 359a25841973..b5cf49d09df2 100644 --- a/drivers/greybus/control.c +++ b/drivers/greybus/control.c @@ -436,7 +436,7 @@ static void gb_control_release(struct device *dev) kfree(control); } -struct device_type greybus_control_type = { +const struct device_type greybus_control_type = { .name = "greybus_control", .release = gb_control_release, }; diff --git a/drivers/greybus/hd.c b/drivers/greybus/hd.c index 72b21bf2d7d3..e2f3496bddc3 100644 --- a/drivers/greybus/hd.c +++ b/drivers/greybus/hd.c @@ -116,7 +116,7 @@ static void gb_hd_release(struct device *dev) kfree(hd); } -struct device_type greybus_hd_type = { +const struct device_type greybus_hd_type = { .name = "greybus_host_device", .release = gb_hd_release, }; diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c index 9ec949a438ef..a88dc701289c 100644 --- a/drivers/greybus/interface.c +++ b/drivers/greybus/interface.c @@ -765,7 +765,7 @@ static const struct dev_pm_ops gb_interface_pm_ops = { gb_interface_runtime_idle) }; -struct device_type greybus_interface_type = { +const struct device_type greybus_interface_type = { .name = "greybus_interface", .release = gb_interface_release, .pm = &gb_interface_pm_ops, diff --git a/drivers/greybus/module.c b/drivers/greybus/module.c index 36f77f9e1d74..7f7153a1dd60 100644 --- a/drivers/greybus/module.c +++ b/drivers/greybus/module.c @@ -81,7 +81,7 @@ static void gb_module_release(struct device *dev) kfree(module); } -struct device_type greybus_module_type = { +const struct device_type greybus_module_type = { .name = "greybus_module", .release = gb_module_release, }; diff --git a/drivers/greybus/svc.c b/drivers/greybus/svc.c index 0d7e749174a4..4256467fcd35 100644 --- a/drivers/greybus/svc.c +++ b/drivers/greybus/svc.c @@ -1305,7 +1305,7 @@ static void gb_svc_release(struct device *dev) kfree(svc); } -struct device_type greybus_svc_type = { +const struct device_type greybus_svc_type = { .name = "greybus_svc", .release = gb_svc_release, }; diff --git a/include/linux/greybus.h b/include/linux/greybus.h index 18c0fb958b74..5f9791fae3c0 100644 --- a/include/linux/greybus.h +++ b/include/linux/greybus.h @@ -106,12 +106,12 @@ struct dentry *gb_debugfs_get(void); extern struct bus_type greybus_bus_type; -extern struct device_type greybus_hd_type; -extern struct device_type greybus_module_type; -extern struct device_type greybus_interface_type; -extern struct device_type greybus_control_type; -extern struct device_type greybus_bundle_type; -extern struct device_type greybus_svc_type; +extern const struct device_type greybus_hd_type; +extern const struct device_type greybus_module_type; +extern const struct device_type greybus_interface_type; +extern const struct device_type greybus_control_type; +extern const struct device_type greybus_bundle_type; +extern const struct device_type greybus_svc_type; static inline int is_gb_host_device(const struct device *dev) {
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d change-id: 20240219-device_cleanup-greybus-c97c1ef52458
Best regards,
On 24 Feb 09:43, Alex Elder wrote:
On 2/19/24 6:40 AM, Ricardo B. Marliere wrote:
Since commit aed65af1cc2f ("drivers: make device_type const"), the driver core can properly handle constant struct device_type. Move the greybus_hd_type, greybus_module_type, greybus_interface_type, greybus_control_type, greybus_bundle_type and greybus_svc_type variables to be constant structures as well, placing it into read-only memory which can not be modified at runtime.
Signed-off-by: Ricardo B. Marliere ricardo@marliere.net
This looks good to me. Assuming it compiles cleanly:
Reviewed-by: Alex Elder elder@linaro.org
Hi Alex!
Thanks for reviewing.
On another subject:
Johan might disagree, but I think it would be nice to make the definitions of the Greybus device types as static (private) and make the is_gb_host_device() etc. functions real functions rather than static inlines in <linux/greybus.h>.
It turns out that all of the is_gb_*() functions are called only from drivers/greybus/core.c; they could all be moved there rather than advertising them in <linux/greybus.h>.
I guess it depends whether they would be used somewhere else in the future. Perhaps it was left there with that intention when it was first being developed? I agree, though. Will happily send a patch with this if desired.
Best regards, - Ricardo.
-Alex
drivers/greybus/bundle.c | 2 +- drivers/greybus/control.c | 2 +- drivers/greybus/hd.c | 2 +- drivers/greybus/interface.c | 2 +- drivers/greybus/module.c | 2 +- drivers/greybus/svc.c | 2 +- include/linux/greybus.h | 12 ++++++------ 7 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/greybus/bundle.c b/drivers/greybus/bundle.c index 84660729538b..a6e1cca06172 100644 --- a/drivers/greybus/bundle.c +++ b/drivers/greybus/bundle.c @@ -166,7 +166,7 @@ static const struct dev_pm_ops gb_bundle_pm_ops = { SET_RUNTIME_PM_OPS(gb_bundle_suspend, gb_bundle_resume, gb_bundle_idle) }; -struct device_type greybus_bundle_type = { +const struct device_type greybus_bundle_type = { .name = "greybus_bundle", .release = gb_bundle_release, .pm = &gb_bundle_pm_ops, diff --git a/drivers/greybus/control.c b/drivers/greybus/control.c index 359a25841973..b5cf49d09df2 100644 --- a/drivers/greybus/control.c +++ b/drivers/greybus/control.c @@ -436,7 +436,7 @@ static void gb_control_release(struct device *dev) kfree(control); } -struct device_type greybus_control_type = { +const struct device_type greybus_control_type = { .name = "greybus_control", .release = gb_control_release, }; diff --git a/drivers/greybus/hd.c b/drivers/greybus/hd.c index 72b21bf2d7d3..e2f3496bddc3 100644 --- a/drivers/greybus/hd.c +++ b/drivers/greybus/hd.c @@ -116,7 +116,7 @@ static void gb_hd_release(struct device *dev) kfree(hd); } -struct device_type greybus_hd_type = { +const struct device_type greybus_hd_type = { .name = "greybus_host_device", .release = gb_hd_release, }; diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c index 9ec949a438ef..a88dc701289c 100644 --- a/drivers/greybus/interface.c +++ b/drivers/greybus/interface.c @@ -765,7 +765,7 @@ static const struct dev_pm_ops gb_interface_pm_ops = { gb_interface_runtime_idle) }; -struct device_type greybus_interface_type = { +const struct device_type greybus_interface_type = { .name = "greybus_interface", .release = gb_interface_release, .pm = &gb_interface_pm_ops, diff --git a/drivers/greybus/module.c b/drivers/greybus/module.c index 36f77f9e1d74..7f7153a1dd60 100644 --- a/drivers/greybus/module.c +++ b/drivers/greybus/module.c @@ -81,7 +81,7 @@ static void gb_module_release(struct device *dev) kfree(module); } -struct device_type greybus_module_type = { +const struct device_type greybus_module_type = { .name = "greybus_module", .release = gb_module_release, }; diff --git a/drivers/greybus/svc.c b/drivers/greybus/svc.c index 0d7e749174a4..4256467fcd35 100644 --- a/drivers/greybus/svc.c +++ b/drivers/greybus/svc.c @@ -1305,7 +1305,7 @@ static void gb_svc_release(struct device *dev) kfree(svc); } -struct device_type greybus_svc_type = { +const struct device_type greybus_svc_type = { .name = "greybus_svc", .release = gb_svc_release, }; diff --git a/include/linux/greybus.h b/include/linux/greybus.h index 18c0fb958b74..5f9791fae3c0 100644 --- a/include/linux/greybus.h +++ b/include/linux/greybus.h @@ -106,12 +106,12 @@ struct dentry *gb_debugfs_get(void); extern struct bus_type greybus_bus_type; -extern struct device_type greybus_hd_type; -extern struct device_type greybus_module_type; -extern struct device_type greybus_interface_type; -extern struct device_type greybus_control_type; -extern struct device_type greybus_bundle_type; -extern struct device_type greybus_svc_type; +extern const struct device_type greybus_hd_type; +extern const struct device_type greybus_module_type; +extern const struct device_type greybus_interface_type; +extern const struct device_type greybus_control_type; +extern const struct device_type greybus_bundle_type; +extern const struct device_type greybus_svc_type; static inline int is_gb_host_device(const struct device *dev) {
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d change-id: 20240219-device_cleanup-greybus-c97c1ef52458
Best regards,
On Sat, Feb 24, 2024 at 05:22:39PM -0300, Ricardo B. Marliere wrote:
On 24 Feb 09:43, Alex Elder wrote:
On 2/19/24 6:40 AM, Ricardo B. Marliere wrote:
Since commit aed65af1cc2f ("drivers: make device_type const"), the driver core can properly handle constant struct device_type. Move the greybus_hd_type, greybus_module_type, greybus_interface_type, greybus_control_type, greybus_bundle_type and greybus_svc_type variables to be constant structures as well, placing it into read-only memory which can not be modified at runtime.
Signed-off-by: Ricardo B. Marliere ricardo@marliere.net
This looks good to me. Assuming it compiles cleanly:
Reviewed-by: Alex Elder elder@linaro.org
Hi Alex!
Thanks for reviewing.
On another subject:
Johan might disagree, but I think it would be nice to make the definitions of the Greybus device types as static (private) and make the is_gb_host_device() etc. functions real functions rather than static inlines in <linux/greybus.h>.
It turns out that all of the is_gb_*() functions are called only from drivers/greybus/core.c; they could all be moved there rather than advertising them in <linux/greybus.h>.
I guess it depends whether they would be used somewhere else in the future. Perhaps it was left there with that intention when it was first being developed? I agree, though. Will happily send a patch with this if desired.
Let's clean the code up for what we have today. If it's needed in the future, we can move the structures then.
thanks,
greg k-h
On 25 Feb 09:33, Greg Kroah-Hartman wrote:
On Sat, Feb 24, 2024 at 05:22:39PM -0300, Ricardo B. Marliere wrote:
On 24 Feb 09:43, Alex Elder wrote:
On 2/19/24 6:40 AM, Ricardo B. Marliere wrote:
Since commit aed65af1cc2f ("drivers: make device_type const"), the driver core can properly handle constant struct device_type. Move the greybus_hd_type, greybus_module_type, greybus_interface_type, greybus_control_type, greybus_bundle_type and greybus_svc_type variables to be constant structures as well, placing it into read-only memory which can not be modified at runtime.
Signed-off-by: Ricardo B. Marliere ricardo@marliere.net
This looks good to me. Assuming it compiles cleanly:
Reviewed-by: Alex Elder elder@linaro.org
Hi Alex!
Thanks for reviewing.
On another subject:
Johan might disagree, but I think it would be nice to make the definitions of the Greybus device types as static (private) and make the is_gb_host_device() etc. functions real functions rather than static inlines in <linux/greybus.h>.
It turns out that all of the is_gb_*() functions are called only from drivers/greybus/core.c; they could all be moved there rather than advertising them in <linux/greybus.h>.
I guess it depends whether they would be used somewhere else in the future. Perhaps it was left there with that intention when it was first being developed? I agree, though. Will happily send a patch with this if desired.
Let's clean the code up for what we have today. If it's needed in the future, we can move the structures then.
Sounds good to me, will send a v2 then!
Thank you, - Ricardo.
thanks,
greg k-h
On 2/25/24 5:04 AM, Ricardo B. Marliere wrote:
On another subject:
Johan might disagree, but I think it would be nice to make the definitions of the Greybus device types as static (private) and make the is_gb_host_device() etc. functions real functions rather than static inlines in <linux/greybus.h>.
It turns out that all of the is_gb_*() functions are called only from drivers/greybus/core.c; they could all be moved there rather than advertising them in <linux/greybus.h>.
I guess it depends whether they would be used somewhere else in the future. Perhaps it was left there with that intention when it was first being developed? I agree, though. Will happily send a patch with this if desired.
Let's clean the code up for what we have today. If it's needed in the future, we can move the structures then.
Sounds good to me, will send a v2 then!
I might be misinterpreting Greg's response; I *think* he agrees with my suggestion.
In any case, please do *not* send v2 with the purpose of including my suggestion.
If you send a v2, keep it focused on this original patch. You can then implement the other suggestion as a follow-on patch (or series).
-Alex
On 26 Feb 13:50, Alex Elder wrote:
On 2/25/24 5:04 AM, Ricardo B. Marliere wrote:
On another subject:
Johan might disagree, but I think it would be nice to make the definitions of the Greybus device types as static (private) and make the is_gb_host_device() etc. functions real functions rather than static inlines in <linux/greybus.h>.
It turns out that all of the is_gb_*() functions are called only from drivers/greybus/core.c; they could all be moved there rather than advertising them in <linux/greybus.h>.
I guess it depends whether they would be used somewhere else in the future. Perhaps it was left there with that intention when it was first being developed? I agree, though. Will happily send a patch with this if desired.
Let's clean the code up for what we have today. If it's needed in the future, we can move the structures then.
Sounds good to me, will send a v2 then!
I might be misinterpreting Greg's response; I *think* he agrees with my suggestion.
That's what I thought too.
In any case, please do *not* send v2 with the purpose of including my suggestion.
If you send a v2, keep it focused on this original patch. You can then implement the other suggestion as a follow-on patch (or series).
Indeed, this one is good as is but I thought of converting it into a series so that they can be taken with no dependency on this one. So it would look like:
Patch 1: move "is_gb_*()" into drivers/greybus/core.c Patch 2: move "device_type greybus_*" into drivers/greybus/core.c Patch 3: make "device_type greybus_*" const
But you're right. I could simply send 1 and 2 after this one has been applied. If I were to send them separately, how would I communicate that there's a dependency? Something like:
--- This series depends on [1]. [1]: lore://link-to-this-patch
?
Thanks and sorry for the noobishness
-Alex
On 26 Feb 17:21, Ricardo B. Marliere wrote:
On 26 Feb 13:50, Alex Elder wrote:
On 2/25/24 5:04 AM, Ricardo B. Marliere wrote:
On another subject:
Johan might disagree, but I think it would be nice to make the definitions of the Greybus device types as static (private) and make the is_gb_host_device() etc. functions real functions rather than static inlines in <linux/greybus.h>.
It turns out that all of the is_gb_*() functions are called only from drivers/greybus/core.c; they could all be moved there rather than advertising them in <linux/greybus.h>.
I guess it depends whether they would be used somewhere else in the future. Perhaps it was left there with that intention when it was first being developed? I agree, though. Will happily send a patch with this if desired.
Let's clean the code up for what we have today. If it's needed in the future, we can move the structures then.
Sounds good to me, will send a v2 then!
I might be misinterpreting Greg's response; I *think* he agrees with my suggestion.
That's what I thought too.
In any case, please do *not* send v2 with the purpose of including my suggestion.
If you send a v2, keep it focused on this original patch. You can then implement the other suggestion as a follow-on patch (or series).
Indeed, this one is good as is but I thought of converting it into a series so that they can be taken with no dependency on this one. So it would look like:
Patch 1: move "is_gb_*()" into drivers/greybus/core.c Patch 2: move "device_type greybus_*" into drivers/greybus/core.c
Sorry, this made no sense!
Patch 3: make "device_type greybus_*" const
But you're right. I could simply send 1 and 2 after this one has been applied. If I were to send them separately, how would I communicate that there's a dependency? Something like:
This series depends on [1]. [1]: lore://link-to-this-patch
?
Thanks and sorry for the noobishness
-Alex
-
Alex Elder -
Greg Kroah-Hartman -
Ricardo B. Marliere