Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver

[Madhumitha Prabakaran]

On 04/17  :41, Alex Elder wrote:
> On 4/16/19 5:13 PM, Madhumitha Prabakaran wrote:
> > Fix a blank line after structure declarations. Also, convert
> > macros into inline functions in order to maintain Linux kernel
> > coding style based on which the inline function is
> > preferable over the macro.
> 
> Madhumitha, here is my explanation for why *not* to convert these
> container_of() macros to inline functions.  It's not just because
> "we do this all over the kernel."  The reason is that it actually
> does not improve the code.
> 
> Inline functions are often better than macros because they are
> explicit about types, allowing the compiler to tell you if you
> are passing parameters of the right type, and possibly assigning
> results to objects of the right type.


This makes more sense now.

> 
> Here is the definition of the container_of() macro in <linux/kernel.h>:
> 
> #define container_of(ptr, type, member) ({                      \
>         const typeof(((type *)0)->member) * __mptr = (ptr);     \
>         (type *)((char *)__mptr - offsetof(type, member)); })
> 
> You see that ptr is explicitly assigned to a local variable
> of the type of the member field, so the compiler is able to
> check that assignment.  And the return value is similarly
> cast to the type of the containing structure, so the type
> compatibility of the assignment can also be checked.  It is
> assumed that where this macro is used, the caller knows it
> is passing an appropriate address.
> 
> There is another thing about this particular definition make
> it just as good as an inline function.  A macro expansion can
> result in one of its parameters being used more than once, and
> that allows those parameters to be *evaluated* more than once
> in a single statement, which can produce incorrect code.
> 
> This macro is defined using the "statement expression"
> extension to C--where a compound statement is enclosed in
> parentheses--({ ... }).  This allows a local variable to be
> used in the macro expansion, which avoids any reuse of the
> macro parameters which might cause side-effects.
> 
> So anyway, I don't think there is any benefit to replacing
> macros like this that do container_of() with inline functions.
> 
> 					-Alex

Thanks for taking time to explain it in detailed way.

> 
> > Blank line fixes are suggested by checkpatch.pl
> > 
> > Signed-off-by: Madhumitha Prabakaran <madhumithabiw@xxxxxxxxx>
> > 
> > Changes in v2:
> > - To maintain consistency in driver greybus, all the instances of macro
> > with container_of are fixed in a single patch.
> > ---
> >  drivers/staging/greybus/bundle.h    |  6 +++++-
> >  drivers/staging/greybus/control.h   |  6 +++++-
> >  drivers/staging/greybus/gbphy.h     | 12 ++++++++++--
> >  drivers/staging/greybus/greybus.h   |  6 +++++-
> >  drivers/staging/greybus/hd.h        |  6 +++++-
> >  drivers/staging/greybus/interface.h |  6 +++++-
> >  drivers/staging/greybus/module.h    |  6 +++++-
> >  drivers/staging/greybus/svc.h       |  6 +++++-
> >  8 files changed, 45 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/bundle.h b/drivers/staging/greybus/bundle.h
> > index 8734d2055657..84956eedb1c4 100644
> > --- a/drivers/staging/greybus/bundle.h
> > +++ b/drivers/staging/greybus/bundle.h
> > @@ -31,7 +31,11 @@ struct gb_bundle {
> >  
> >  	struct list_head	links;	/* interface->bundles */
> >  };
> > -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
> > +
> > +static inline struct gb_bundle *to_gb_bundle(struct device *d)
> > +{
> > +	return container_of(d, struct gb_bundle, dev);
> > +}
> >  
> >  /* Greybus "private" definitions" */
> >  struct gb_bundle *gb_bundle_create(struct gb_interface *intf, u8 bundle_id,
> > diff --git a/drivers/staging/greybus/control.h b/drivers/staging/greybus/control.h
> > index 3a29ec05f631..a681ef74e7fe 100644
> > --- a/drivers/staging/greybus/control.h
> > +++ b/drivers/staging/greybus/control.h
> > @@ -24,7 +24,11 @@ struct gb_control {
> >  	char *vendor_string;
> >  	char *product_string;
> >  };
> > -#define to_gb_control(d) container_of(d, struct gb_control, dev)
> > +
> > +static inline struct gb_control *to_gb_control(struct device *d)
> > +{
> > +	return container_of(d, struct gb_control, dev);
> > +}
> >  
> >  struct gb_control *gb_control_create(struct gb_interface *intf);
> >  int gb_control_enable(struct gb_control *control);
> > diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h
> > index 99463489d7d6..20307f6dfcb9 100644
> > --- a/drivers/staging/greybus/gbphy.h
> > +++ b/drivers/staging/greybus/gbphy.h
> > @@ -15,7 +15,11 @@ struct gbphy_device {
> >  	struct list_head list;
> >  	struct device dev;
> >  };
> > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> > +
> > +static inline struct gbphy_device *to_gbphy_dev(struct device *d)
> > +{
> > +	return container_of(d, struct gbphy_device, dev);
> > +}
> >  
> >  static inline void *gb_gbphy_get_data(struct gbphy_device *gdev)
> >  {
> > @@ -43,7 +47,11 @@ struct gbphy_driver {
> >  
> >  	struct device_driver driver;
> >  };
> > -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver)
> > +
> > +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *d)
> > +{
> > +	return container_of(d, struct gbphy_driver, driver);
> > +}
> >  
> >  int gb_gbphy_register_driver(struct gbphy_driver *driver,
> >  			     struct module *owner, const char *mod_name);
> > diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h
> > index d03ddb7c9df0..a82d5002b4d5 100644
> > --- a/drivers/staging/greybus/greybus.h
> > +++ b/drivers/staging/greybus/greybus.h
> > @@ -64,7 +64,11 @@ struct greybus_driver {
> >  
> >  	struct device_driver driver;
> >  };
> > -#define to_greybus_driver(d) container_of(d, struct greybus_driver, driver)
> > +
> > +static inline struct greybus_driver *to_greybus_driver(struct device_driver *d)
> > +{
> > +	return container_of(d, struct greybus_driver, driver);
> > +}
> >  
> >  static inline void greybus_set_drvdata(struct gb_bundle *bundle, void *data)
> >  {
> > diff --git a/drivers/staging/greybus/hd.h b/drivers/staging/greybus/hd.h
> > index 6cf024a20a58..de7c49d05266 100644
> > --- a/drivers/staging/greybus/hd.h
> > +++ b/drivers/staging/greybus/hd.h
> > @@ -57,7 +57,11 @@ struct gb_host_device {
> >  	/* Private data for the host driver */
> >  	unsigned long hd_priv[0] __aligned(sizeof(s64));
> >  };
> > -#define to_gb_host_device(d) container_of(d, struct gb_host_device, dev)
> > +
> > +static inline struct gb_host_device *to_gb_host_device(struct device *d)
> > +{
> > +	return container_of(d, struct gb_host_device, dev);
> > +}
> >  
> >  int gb_hd_cport_reserve(struct gb_host_device *hd, u16 cport_id);
> >  void gb_hd_cport_release_reserved(struct gb_host_device *hd, u16 cport_id);
> > diff --git a/drivers/staging/greybus/interface.h b/drivers/staging/greybus/interface.h
> > index 1c00c5bb3ec9..f86c0d596dbe 100644
> > --- a/drivers/staging/greybus/interface.h
> > +++ b/drivers/staging/greybus/interface.h
> > @@ -63,7 +63,11 @@ struct gb_interface {
> >  	struct work_struct mode_switch_work;
> >  	struct completion mode_switch_completion;
> >  };
> > -#define to_gb_interface(d) container_of(d, struct gb_interface, dev)
> > +
> > +static inline struct gb_interface *to_gb_interface(struct device *d)
> > +{
> > +	return container_of(d, struct gb_interface, dev);
> > +}
> >  
> >  struct gb_interface *gb_interface_create(struct gb_module *module,
> >  					 u8 interface_id);
> > diff --git a/drivers/staging/greybus/module.h b/drivers/staging/greybus/module.h
> > index b1ebcc6636db..c427b788b677 100644
> > --- a/drivers/staging/greybus/module.h
> > +++ b/drivers/staging/greybus/module.h
> > @@ -22,7 +22,11 @@ struct gb_module {
> >  
> >  	struct gb_interface *interfaces[0];
> >  };
> > -#define to_gb_module(d) container_of(d, struct gb_module, dev)
> > +
> > +static inline struct gb_module *to_gb_module(struct device *d)
> > +{
> > +	return container_of(d, struct gb_module, dev);
> > +}
> >  
> >  struct gb_module *gb_module_create(struct gb_host_device *hd, u8 module_id,
> >  				   size_t num_interfaces);
> > diff --git a/drivers/staging/greybus/svc.h b/drivers/staging/greybus/svc.h
> > index ad01783bac9c..4e35ac9ed0ff 100644
> > --- a/drivers/staging/greybus/svc.h
> > +++ b/drivers/staging/greybus/svc.h
> > @@ -52,7 +52,11 @@ struct gb_svc {
> >  	struct dentry *debugfs_dentry;
> >  	struct svc_debugfs_pwrmon_rail *pwrmon_rails;
> >  };
> > -#define to_gb_svc(d) container_of(d, struct gb_svc, dev)
> > +
> > +static inline struct gb_svc *to_gb_svc(struct device *d)
> > +{
> > +	return container_of(d, struct gb_svc, dev);
> > +}
> >  
> >  struct gb_svc *gb_svc_create(struct gb_host_device *hd);
> >  int gb_svc_add(struct gb_svc *svc);
> > 
>