greybus: 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
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(-)


---
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