Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
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 :
>
> #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
> >
> > 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_headlinks; /* 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);
> > +}
> >
> >
Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
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.
Here is the definition of the container_of() macro in :
#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
> Blank line fixes are suggested by checkpatch.pl
>
> Signed-off-by: Madhumitha Prabakaran
>
> 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_headlinks; /* 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
Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
On Wed, Apr 17, 2019 at 06:19:50AM -0500, Alex Elder wrote: > I'm not completely sure about the inline function, but on the no blank > lines thing (and many other minor issues) "checkpatch.pl" is to blame. > There are lots of examples of issues that checkpatch points out that are > matters of opinion and not hardened kernel style rules. > > We try to encourage people to get involved with kernel development by > fixing minor problems, and we tell them a good way to find them is > by running checkpatch and "fixing" what it reports. Unfortunately, > it is often things of this type, and reviewers balk and say "no, > please leave it," and the poor new person has a bad first experience. > > I *like* "checkpatch.pl". And the fact that it can point out some > of these sorts of things is great. But it would be nice if certain > types of problems (like multiple blank lines, or lines that are 81 > characters wide for example) would only be reported when a "--strict" > option or something were supplied. The problem is that --strict is enabled by default when running checkpatch on code in staging (and net). And it has all sorts of weird tests (prefixed as "CHECK" rather than "WARNING") to catch everyone and their mom's pet peeve. I don't think the intention ever was that all those should be "fixed", but this appears to be where this checkpatch mission creep comes from (and we're now seeing --strict being used on code outside of staging too). IMO we're setting a bad example for new contributers by accepting such changes by default. Blindly trusting a tool is not how kernel development works, but that seems to be the message currently sent. Johan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
On 4/17/19 1:25 AM, Greg KH wrote:
> On Tue, Apr 16, 2019 at 05:13:18PM -0500, 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.
>>
>> Blank line fixes are suggested by checkpatch.pl
>>
>> Signed-off-by: Madhumitha Prabakaran
>>
>> 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_headlinks; /* 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);
>> +}
>
> Why are we changing this to an inline function? The #define is fine,
> and how we "normally" do this type of container_of conversion.
I'm not completely sure about the inline function, but on the no blank
lines thing (and many other minor issues) "checkpatch.pl" is to blame.
There are lots of examples of issues that checkpatch points out that are
matters of opinion and not hardened kernel style rules.
We try to encourage people to get involved with kernel development by
fixing minor problems, and we tell them a good way to find them is
by running checkpatch and "fixing" what it reports. Unfortunately,
it is often things of this type, and reviewers balk and say "no,
please leave it," and the poor new person has a bad first experience.
I *like* "checkpatch.pl". And the fact that it can point out some
of these sorts of things is great. But it would be nice if certain
types of problems (like multiple blank lines, or lines that are 81
characters wide for example) would only be reported when a "--strict"
option or something were supplied.
-Alex
> I understand the objection of the "no blank line", but this was the
> "original" style that we used to create these #defines from the very
> beginning of the driver model work a decade ago. Changing that
> muscle-memory is going to be hard for some of us. Look at
> drivers/base/base.h for other examples of this.
>
> thanks,
>
> greg k-h
> ___
> greybus-dev mailing list
> greybus-...@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver
On 16-04-19, 17:13, 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. > > Blank line fixes are suggested by checkpatch.pl This was all intentional and we use such macros everywhere in kernel. No need for such a patch, sorry. -- viresh ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
