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);
> >
>
_______________________________________________
greybus-dev mailing list
greybus-dev@xxxxxxxxxxxxxxxx
https://lists.linaro.org/mailman/listinfo/greybus-dev
|