[v2] driver core: allow kobj_to_dev() to take a const pointer

  If a const * to a kobject is passed to kobj_to_dev(), we want to return
back a const * to a device as the driver core shouldn't be modifying a
constant structure.  But when dealing with container_of() the pointer
const attribute is cast away, so we need to manually handle this by
determining the type of the pointer passed in to know the type of the
pointer to pass out.

Luckily _Generic can do this type of magic, and as the kernel now
supports C11 it is availble to us to handle this type of build-time type
detection.

Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2 - use _Generic() to make this type safe as pointed out by Sakari

 include/linux/device.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)
  

Hi Greg,

On Sun, Oct 16, 2022 at 12:41:26PM +0200, Greg Kroah-Hartman wrote:
> If a const * to a kobject is passed to kobj_to_dev(), we want to return
> back a const * to a device as the driver core shouldn't be modifying a
> constant structure.  But when dealing with container_of() the pointer
> const attribute is cast away, so we need to manually handle this by
> determining the type of the pointer passed in to know the type of the
> pointer to pass out.

Alternatively container_of() could be fixed, but that will likely produce
lots of warnings currently.

> 
> Luckily _Generic can do this type of magic, and as the kernel now
> supports C11 it is availble to us to handle this type of build-time type
> detection.
> 
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2 - use _Generic() to make this type safe as pointed out by Sakari
> 
>  include/linux/device.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 424b55df0272..023ea50b1916 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -680,11 +680,27 @@ struct device_link {
>  	bool supplier_preactivated; /* Owned by consumer probe. */
>  };
>  
> -static inline struct device *kobj_to_dev(struct kobject *kobj)
> +static inline struct device *__kobj_to_dev(struct kobject *kobj)
>  {
>  	return container_of(kobj, struct device, kobj);
>  }
>  
> +static inline const struct device *__kobj_to_dev_const(const struct kobject *kobj)
> +{
> +	return container_of(kobj, const struct device, kobj);
> +}
> +
> +/*
> + * container_of() will happily take a const * and spit back a non-const * as it
> + * is just doing pointer math.  But we want to be a bit more careful in the
> + * driver code, so manually force any const * of a kobject to also be a const *
> + * to a device.
> + */

container_of() documentation has (probably?) never warned about this.

Wouldn't such a comment be more appropriate there? Albeit it wouldn't be
needed if container_of() were fixed.

> +#define kobj_to_dev(kobj)					\
> +	_Generic((kobj),					\
> +		 const struct kobject *: __kobj_to_dev_const,	\
> +		 struct kobject *: __kobj_to_dev)(kobj)
> +
>  /**
>   * device_iommu_mapped - Returns true when the device DMA is translated
>   *			 by an IOMMU
  

On Mon, Oct 17, 2022 at 07:54:52AM +0000, Sakari Ailus wrote:
> Hi Greg,
> 
> On Sun, Oct 16, 2022 at 12:41:26PM +0200, Greg Kroah-Hartman wrote:
> > If a const * to a kobject is passed to kobj_to_dev(), we want to return
> > back a const * to a device as the driver core shouldn't be modifying a
> > constant structure.  But when dealing with container_of() the pointer
> > const attribute is cast away, so we need to manually handle this by
> > determining the type of the pointer passed in to know the type of the
> > pointer to pass out.
> 
> Alternatively container_of() could be fixed, but that will likely produce
> lots of warnings currently.

Yeah, we can not do that because, as you found out, there's just too
many warnings that it would cause.  Let's work on the individual
subsystems to clean them all up first before worrying about the core
container_of() macro as that should fix the majority of the build
warnings.

> > Luckily _Generic can do this type of magic, and as the kernel now
> > supports C11 it is availble to us to handle this type of build-time type
> > detection.
> > 
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > v2 - use _Generic() to make this type safe as pointed out by Sakari
> > 
> >  include/linux/device.h | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 424b55df0272..023ea50b1916 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -680,11 +680,27 @@ struct device_link {
> >  	bool supplier_preactivated; /* Owned by consumer probe. */
> >  };
> >  
> > -static inline struct device *kobj_to_dev(struct kobject *kobj)
> > +static inline struct device *__kobj_to_dev(struct kobject *kobj)
> >  {
> >  	return container_of(kobj, struct device, kobj);
> >  }
> >  
> > +static inline const struct device *__kobj_to_dev_const(const struct kobject *kobj)
> > +{
> > +	return container_of(kobj, const struct device, kobj);
> > +}
> > +
> > +/*
> > + * container_of() will happily take a const * and spit back a non-const * as it
> > + * is just doing pointer math.  But we want to be a bit more careful in the
> > + * driver code, so manually force any const * of a kobject to also be a const *
> > + * to a device.
> > + */
> 
> container_of() documentation has (probably?) never warned about this.

We never thought of it before :(

> Wouldn't such a comment be more appropriate there? Albeit it wouldn't be
> needed if container_of() were fixed.

Some comment added to container_of() would be great, but that does not
remove the need to keep this one.

thanks,

greg k-h
  

On Sun, Oct 16, 2022 at 12:41:26PM +0200, Greg Kroah-Hartman wrote:
> If a const * to a kobject is passed to kobj_to_dev(), we want to return
> back a const * to a device as the driver core shouldn't be modifying a
> constant structure.  But when dealing with container_of() the pointer
> const attribute is cast away, so we need to manually handle this by
> determining the type of the pointer passed in to know the type of the
> pointer to pass out.
> 
> Luckily _Generic can do this type of magic, and as the kernel now
> supports C11 it is availble to us to handle this type of build-time type
> detection.

I was following this in your branch and I find it good,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2 - use _Generic() to make this type safe as pointed out by Sakari
> 
>  include/linux/device.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 424b55df0272..023ea50b1916 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -680,11 +680,27 @@ struct device_link {
>  	bool supplier_preactivated; /* Owned by consumer probe. */
>  };
>  
> -static inline struct device *kobj_to_dev(struct kobject *kobj)
> +static inline struct device *__kobj_to_dev(struct kobject *kobj)
>  {
>  	return container_of(kobj, struct device, kobj);
>  }
>  
> +static inline const struct device *__kobj_to_dev_const(const struct kobject *kobj)
> +{
> +	return container_of(kobj, const struct device, kobj);
> +}
> +
> +/*
> + * container_of() will happily take a const * and spit back a non-const * as it
> + * is just doing pointer math.  But we want to be a bit more careful in the
> + * driver code, so manually force any const * of a kobject to also be a const *
> + * to a device.
> + */
> +#define kobj_to_dev(kobj)					\
> +	_Generic((kobj),					\
> +		 const struct kobject *: __kobj_to_dev_const,	\
> +		 struct kobject *: __kobj_to_dev)(kobj)
> +
>  /**
>   * device_iommu_mapped - Returns true when the device DMA is translated
>   *			 by an IOMMU
> -- 
> 2.38.0
>
  

Hi Greg,

On Mon, Oct 17, 2022 at 10:04:14AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 17, 2022 at 07:54:52AM +0000, Sakari Ailus wrote:
> > Hi Greg,
> > 
> > On Sun, Oct 16, 2022 at 12:41:26PM +0200, Greg Kroah-Hartman wrote:
> > > If a const * to a kobject is passed to kobj_to_dev(), we want to return
> > > back a const * to a device as the driver core shouldn't be modifying a
> > > constant structure.  But when dealing with container_of() the pointer
> > > const attribute is cast away, so we need to manually handle this by
> > > determining the type of the pointer passed in to know the type of the
> > > pointer to pass out.
> > 
> > Alternatively container_of() could be fixed, but that will likely produce
> > lots of warnings currently.
> 
> Yeah, we can not do that because, as you found out, there's just too
> many warnings that it would cause.  Let's work on the individual
> subsystems to clean them all up first before worrying about the core
> container_of() macro as that should fix the majority of the build
> warnings.

Sounds reasonable.

> 
> > > Luckily _Generic can do this type of magic, and as the kernel now
> > > supports C11 it is availble to us to handle this type of build-time type
> > > detection.
> > > 
> > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > > v2 - use _Generic() to make this type safe as pointed out by Sakari
> > > 
> > >  include/linux/device.h | 18 +++++++++++++++++-
> > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 424b55df0272..023ea50b1916 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -680,11 +680,27 @@ struct device_link {
> > >  	bool supplier_preactivated; /* Owned by consumer probe. */
> > >  };
> > >  
> > > -static inline struct device *kobj_to_dev(struct kobject *kobj)
> > > +static inline struct device *__kobj_to_dev(struct kobject *kobj)
> > >  {
> > >  	return container_of(kobj, struct device, kobj);
> > >  }
> > >  
> > > +static inline const struct device *__kobj_to_dev_const(const struct kobject *kobj)
> > > +{
> > > +	return container_of(kobj, const struct device, kobj);
> > > +}
> > > +
> > > +/*
> > > + * container_of() will happily take a const * and spit back a non-const * as it
> > > + * is just doing pointer math.  But we want to be a bit more careful in the
> > > + * driver code, so manually force any const * of a kobject to also be a const *
> > > + * to a device.
> > > + */
> > 
> > container_of() documentation has (probably?) never warned about this.
> 
> We never thought of it before :(
> 
> > Wouldn't such a comment be more appropriate there? Albeit it wouldn't be
> > needed if container_of() were fixed.
> 
> Some comment added to container_of() would be great, but that does not
> remove the need to keep this one.

I can send a patch for that.

For this one:

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
  

From: Greg Kroah-Hartman
> Sent: 17 October 2022 09:04
> 
> On Mon, Oct 17, 2022 at 07:54:52AM +0000, Sakari Ailus wrote:
> > Hi Greg,
> >
> > On Sun, Oct 16, 2022 at 12:41:26PM +0200, Greg Kroah-Hartman wrote:
> > > If a const * to a kobject is passed to kobj_to_dev(), we want to return
> > > back a const * to a device as the driver core shouldn't be modifying a
> > > constant structure.  But when dealing with container_of() the pointer
> > > const attribute is cast away, so we need to manually handle this by
> > > determining the type of the pointer passed in to know the type of the
> > > pointer to pass out.
> >
> > Alternatively container_of() could be fixed, but that will likely produce
> > lots of warnings currently.
> 
> Yeah, we can not do that because, as you found out, there's just too
> many warnings that it would cause.  Let's work on the individual
> subsystems to clean them all up first before worrying about the core
> container_of() macro as that should fix the majority of the build
> warnings.

Is it possible to generate a fixed container_of() with a
different name and then use that to clean up the subsystems?
Then finally rename it back?

That you probably be a lot less churn.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Mon, Oct 17, 2022 at 11:24:26AM +0000, David Laight wrote:
> From: Greg Kroah-Hartman
> > Sent: 17 October 2022 09:04
> > 
> > On Mon, Oct 17, 2022 at 07:54:52AM +0000, Sakari Ailus wrote:
> > > Hi Greg,
> > >
> > > On Sun, Oct 16, 2022 at 12:41:26PM +0200, Greg Kroah-Hartman wrote:
> > > > If a const * to a kobject is passed to kobj_to_dev(), we want to return
> > > > back a const * to a device as the driver core shouldn't be modifying a
> > > > constant structure.  But when dealing with container_of() the pointer
> > > > const attribute is cast away, so we need to manually handle this by
> > > > determining the type of the pointer passed in to know the type of the
> > > > pointer to pass out.
> > >
> > > Alternatively container_of() could be fixed, but that will likely produce
> > > lots of warnings currently.
> > 
> > Yeah, we can not do that because, as you found out, there's just too
> > many warnings that it would cause.  Let's work on the individual
> > subsystems to clean them all up first before worrying about the core
> > container_of() macro as that should fix the majority of the build
> > warnings.
> 
> Is it possible to generate a fixed container_of() with a
> different name and then use that to clean up the subsystems?
> Then finally rename it back?
> 
> That you probably be a lot less churn.

That's the identical churn.