edits in greybus driver

[PATCH 2/3] staging: greybus: use inline function for macros

Posted by Menna Mahmoud 2 years, 10 months ago

Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a
static inline function.

it is not great to have macro that use `container_of` macro,
because from looking at the definition one cannot tell what type
it applies to.

One can get the same benefit from an efficiency point of view
by making an inline function.

Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com>
---
 drivers/staging/greybus/gbphy.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h
index d4a225b76338..03a977056637 100644
--- a/drivers/staging/greybus/gbphy.h
+++ b/drivers/staging/greybus/gbphy.h
@@ -15,7 +15,10 @@ 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(const struct device *d)
+{
+	return container_of(d, struct gbphy_device, dev);
+}
 
 static inline void *gb_gbphy_get_data(struct gbphy_device *gdev)
 {
@@ -43,7 +46,10 @@ 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);
-- 
2.34.1

Re: [PATCH 2/3] staging: greybus: use inline function for macros

Posted by Uwe Kleine-König 2 years, 10 months ago

Hello,

just some nitpicks:

On Tue, Mar 21, 2023 at 01:04:33AM +0200, Menna Mahmoud wrote:
> Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a
> static inline function.
> 
> it is not great to have macro that use `container_of` macro,

s/it/It/; s/macro/macros/; s/use/use the/;

> because from looking at the definition one cannot tell what type
> it applies to.
> [...]
> -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> +static inline struct gbphy_device *to_gbphy_dev(const struct device *d)

drivers/staging/greybus/gbphy.c always passes a variable named
"dev" to this macro. So I'd call the parameter "dev", too, instead of
"d". This is also a more typical name for variables of that type.

> +{
> +	return container_of(d, struct gbphy_device, dev);
> +}
> [...]
>  };
> -#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);
> +}

With a similar reasoning (and also to not have "d"s that are either
device or device_driver) I'd recommend "drv" here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 2/3] staging: greybus: use inline function for macros

Posted by Menna Mahmoud 2 years, 10 months ago

On ٢١‏/٣‏/٢٠٢٣ ١٧:٤٧, Uwe Kleine-König wrote:
> Hello,
>
> just some nitpicks:
>
> On Tue, Mar 21, 2023 at 01:04:33AM +0200, Menna Mahmoud wrote:
>> Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a
>> static inline function.
>>
>> it is not great to have macro that use `container_of` macro,
> s/it/It/; s/macro/macros/; s/use/use the/;
Okay, I will fix it.
>
>> because from looking at the definition one cannot tell what type
>> it applies to.
>> [...]
>> -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
>> +static inline struct gbphy_device *to_gbphy_dev(const struct device *d)
> drivers/staging/greybus/gbphy.c always passes a variable named
> "dev" to this macro. So I'd call the parameter "dev", too, instead of
> "d". This is also a more typical name for variables of that type.
>
>> +{
>> +	return container_of(d, struct gbphy_device, dev);
>> +}
>> [...]
>>   };
>> -#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);
>> +}
> With a similar reasoning (and also to not have "d"s that are either
> device or device_driver) I'd recommend "drv" here.


please check this with Julia, because she said they should different.


Thanks,

Menna

>
> Best regards
> Uwe
>

Re: [PATCH 2/3] staging: greybus: use inline function for macros

Posted by Uwe Kleine-König 2 years, 10 months ago

On Tue, Mar 21, 2023 at 06:25:29PM +0200, Menna Mahmoud wrote:
> 
> On ٢١‏/٣‏/٢٠٢٣ ١٧:٤٧, Uwe Kleine-König wrote:
> > Hello,
> > 
> > just some nitpicks:
> > 
> > On Tue, Mar 21, 2023 at 01:04:33AM +0200, Menna Mahmoud wrote:
> > > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a
> > > static inline function.
> > > 
> > > it is not great to have macro that use `container_of` macro,
> > s/it/It/; s/macro/macros/; s/use/use the/;
> Okay, I will fix it.
> > 
> > > because from looking at the definition one cannot tell what type
> > > it applies to.
> > > [...]
> > > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> > > +static inline struct gbphy_device *to_gbphy_dev(const struct device *d)
> > drivers/staging/greybus/gbphy.c always passes a variable named
> > "dev" to this macro. So I'd call the parameter "dev", too, instead of
> > "d". This is also a more typical name for variables of that type.
> > 
> > > +{
> > > +	return container_of(d, struct gbphy_device, dev);
> > > +}
> > > [...]
> > >   };
> > > -#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);
> > > +}
> > With a similar reasoning (and also to not have "d"s that are either
> > device or device_driver) I'd recommend "drv" here.
> 
> 
> please check this with Julia, because she said they should different.

At least use "_dev" instead of "d" which seems to be a common idiom,
too:

	$ git grep -P 'container_of\(_(?<ident>[A-Za-z_0-9-]*)\s*,[^,]*,\s*\g{ident}\s*\)' | wc -l
	570

("drv" should be fine, because the third argument is "driver" there.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 2/3] staging: greybus: use inline function for macros

Posted by Menna Mahmoud 2 years, 10 months ago

On ٢١‏/٣‏/٢٠٢٣ ١٨:٤٢, Uwe Kleine-König wrote:
> On Tue, Mar 21, 2023 at 06:25:29PM +0200, Menna Mahmoud wrote:
>> On ٢١‏/٣‏/٢٠٢٣ ١٧:٤٧, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> just some nitpicks:
>>>
>>> On Tue, Mar 21, 2023 at 01:04:33AM +0200, Menna Mahmoud wrote:
>>>> Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a
>>>> static inline function.
>>>>
>>>> it is not great to have macro that use `container_of` macro,
>>> s/it/It/; s/macro/macros/; s/use/use the/;
>> Okay, I will fix it.
>>>> because from looking at the definition one cannot tell what type
>>>> it applies to.
>>>> [...]
>>>> -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
>>>> +static inline struct gbphy_device *to_gbphy_dev(const struct device *d)
>>> drivers/staging/greybus/gbphy.c always passes a variable named
>>> "dev" to this macro. So I'd call the parameter "dev", too, instead of
>>> "d". This is also a more typical name for variables of that type.
>>>
>>>> +{
>>>> +	return container_of(d, struct gbphy_device, dev);
>>>> +}
>>>> [...]
>>>>    };
>>>> -#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);
>>>> +}
>>> With a similar reasoning (and also to not have "d"s that are either
>>> device or device_driver) I'd recommend "drv" here.
>>
>> please check this with Julia, because she said they should different.
> At least use "_dev" instead of "d" which seems to be a common idiom,
> too:
>
> 	$ git grep -P 'container_of\(_(?<ident>[A-Za-z_0-9-]*)\s*,[^,]*,\s*\g{ident}\s*\)' | wc -l
> 	570
>
> ("drv" should be fine, because the third argument is "driver" there.)

Okay, I will do that.

Thanks,

Menna

>
> Best regards
> Uwe
>

Re: [PATCH 2/3] staging: greybus: use inline function for macros

Posted by Julia Lawall 2 years, 10 months ago


On Tue, 21 Mar 2023, Uwe Kleine-König wrote:

> Hello,
>
> just some nitpicks:
>
> On Tue, Mar 21, 2023 at 01:04:33AM +0200, Menna Mahmoud wrote:
> > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a
> > static inline function.
> >
> > it is not great to have macro that use `container_of` macro,
>
> s/it/It/; s/macro/macros/; s/use/use the/;
>
> > because from looking at the definition one cannot tell what type
> > it applies to.
> > [...]
> > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> > +static inline struct gbphy_device *to_gbphy_dev(const struct device *d)
>
> drivers/staging/greybus/gbphy.c always passes a variable named
> "dev" to this macro. So I'd call the parameter "dev", too, instead of
> "d". This is also a more typical name for variables of that type.

I argued against that.  Because then there are two uses of dev
in the argument of container_of, and they refer to completely different
things.  It's true that by the way container_of works, it's fine, but it
may be misleading.

julia

>
> > +{
> > +	return container_of(d, struct gbphy_device, dev);
> > +}
> > [...]
> >  };
> > -#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);
> > +}
>
> With a similar reasoning (and also to not have "d"s that are either
> device or device_driver) I'd recommend "drv" here.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |
>

Re: [PATCH 2/3] staging: greybus: use inline function for macros

Posted by Uwe Kleine-König 2 years, 10 months ago

On Tue, Mar 21, 2023 at 04:59:49PM +0100, Julia Lawall wrote:
> 
> 
> On Tue, 21 Mar 2023, Uwe Kleine-König wrote:
> 
> > Hello,
> >
> > just some nitpicks:
> >
> > On Tue, Mar 21, 2023 at 01:04:33AM +0200, Menna Mahmoud wrote:
> > > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a
> > > static inline function.
> > >
> > > it is not great to have macro that use `container_of` macro,
> >
> > s/it/It/; s/macro/macros/; s/use/use the/;
> >
> > > because from looking at the definition one cannot tell what type
> > > it applies to.
> > > [...]
> > > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> > > +static inline struct gbphy_device *to_gbphy_dev(const struct device *d)
> >
> > drivers/staging/greybus/gbphy.c always passes a variable named
> > "dev" to this macro. So I'd call the parameter "dev", too, instead of
> > "d". This is also a more typical name for variables of that type.
> 
> I argued against that.  Because then there are two uses of dev
> in the argument of container_of, and they refer to completely different
> things.  It's true that by the way container_of works, it's fine, but it
> may be misleading.

Hmm, that seems to be subjective, but I have less problems with that
than with using "d" for a struct device (or a struct device_driver).
I'd even go so far as to consider it nice if they are identical.

Maybe that's because having the first and third argument identical is
quite common:

	$ git grep -P 'container_of\((?<ident>[A-Za-z_0-9-]*)\s*,[^,]*,\s*\g{ident}\s*\)' | wc -l
	5940

which is >44% of all the usages

	$ git grep -P 'container_of\((?<ident>[A-Za-z_0-9-]*)\s*,[^,]*,\s*(?&ident)\s*\)' | wc -l
	13362

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 2/3] staging: greybus: use inline function for macros

Posted by Julia Lawall 2 years, 10 months ago


On Tue, 21 Mar 2023, Uwe Kleine-König wrote:

> On Tue, Mar 21, 2023 at 04:59:49PM +0100, Julia Lawall wrote:
> >
> >
> > On Tue, 21 Mar 2023, Uwe Kleine-König wrote:
> >
> > > Hello,
> > >
> > > just some nitpicks:
> > >
> > > On Tue, Mar 21, 2023 at 01:04:33AM +0200, Menna Mahmoud wrote:
> > > > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a
> > > > static inline function.
> > > >
> > > > it is not great to have macro that use `container_of` macro,
> > >
> > > s/it/It/; s/macro/macros/; s/use/use the/;
> > >
> > > > because from looking at the definition one cannot tell what type
> > > > it applies to.
> > > > [...]
> > > > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> > > > +static inline struct gbphy_device *to_gbphy_dev(const struct device *d)
> > >
> > > drivers/staging/greybus/gbphy.c always passes a variable named
> > > "dev" to this macro. So I'd call the parameter "dev", too, instead of
> > > "d". This is also a more typical name for variables of that type.
> >
> > I argued against that.  Because then there are two uses of dev
> > in the argument of container_of, and they refer to completely different
> > things.  It's true that by the way container_of works, it's fine, but it
> > may be misleading.
>
> Hmm, that seems to be subjective, but I have less problems with that
> than with using "d" for a struct device (or a struct device_driver).
> I'd even go so far as to consider it nice if they are identical.
>
> Maybe that's because having the first and third argument identical is
> quite common:
>
> 	$ git grep -P 'container_of\((?<ident>[A-Za-z_0-9-]*)\s*,[^,]*,\s*\g{ident}\s*\)' | wc -l
> 	5940
>
> which is >44% of all the usages
>
> 	$ git grep -P 'container_of\((?<ident>[A-Za-z_0-9-]*)\s*,[^,]*,\s*(?&ident)\s*\)' | wc -l
> 	13362

OK, if people like that, then why not.  But it's dangerous if the call to
container_of is in a macro, rather than in a function.

julia

Re: [PATCH 2/3] staging: greybus: use inline function for macros

Posted by Greg KH 2 years, 10 months ago

On Tue, Mar 21, 2023 at 05:35:54PM +0100, Julia Lawall wrote:
> 
> 
> On Tue, 21 Mar 2023, Uwe Kleine-König wrote:
> 
> > On Tue, Mar 21, 2023 at 04:59:49PM +0100, Julia Lawall wrote:
> > >
> > >
> > > On Tue, 21 Mar 2023, Uwe Kleine-König wrote:
> > >
> > > > Hello,
> > > >
> > > > just some nitpicks:
> > > >
> > > > On Tue, Mar 21, 2023 at 01:04:33AM +0200, Menna Mahmoud wrote:
> > > > > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a
> > > > > static inline function.
> > > > >
> > > > > it is not great to have macro that use `container_of` macro,
> > > >
> > > > s/it/It/; s/macro/macros/; s/use/use the/;
> > > >
> > > > > because from looking at the definition one cannot tell what type
> > > > > it applies to.
> > > > > [...]
> > > > > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> > > > > +static inline struct gbphy_device *to_gbphy_dev(const struct device *d)
> > > >
> > > > drivers/staging/greybus/gbphy.c always passes a variable named
> > > > "dev" to this macro. So I'd call the parameter "dev", too, instead of
> > > > "d". This is also a more typical name for variables of that type.
> > >
> > > I argued against that.  Because then there are two uses of dev
> > > in the argument of container_of, and they refer to completely different
> > > things.  It's true that by the way container_of works, it's fine, but it
> > > may be misleading.
> >
> > Hmm, that seems to be subjective, but I have less problems with that
> > than with using "d" for a struct device (or a struct device_driver).
> > I'd even go so far as to consider it nice if they are identical.
> >
> > Maybe that's because having the first and third argument identical is
> > quite common:
> >
> > 	$ git grep -P 'container_of\((?<ident>[A-Za-z_0-9-]*)\s*,[^,]*,\s*\g{ident}\s*\)' | wc -l
> > 	5940
> >
> > which is >44% of all the usages
> >
> > 	$ git grep -P 'container_of\((?<ident>[A-Za-z_0-9-]*)\s*,[^,]*,\s*(?&ident)\s*\)' | wc -l
> > 	13362
> 
> OK, if people like that, then why not.  But it's dangerous if the call to
> container_of is in a macro, rather than in a function.

It's not "dangerous" at all, as the macro will enforce type-safety, you
can't get it wrong when using it.

Ideally this is best as a macro as it's just doing pointer math, worst
case, the compiler turns a function like this into a real function and
you have a call/subtract/return for every time you make this call.

So this conversion to functions feels odd to me, as you potentially are
making all of this worse overall.

Wait until people realize that when we eventually turn these into
container_of_const() you HAVE to go back to using it as a macro instead
of in a function call wrapper like this...

thanks,

greg k-h