warning when converting `to_gbphy_dev` into inline function

warning when converting `to_gbphy_dev` into inline function

[Menna Mahmoud]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]

Hi Julia,

I tried to convert to_gbphy_dev to inline function but got these 
warnings while building:


```

drivers/staging/greybus/gbphy.c: In function ‘gbphy_dev_uevent’:
drivers/staging/greybus/gbphy.c:76:61: warning: passing argument 1 of 
‘to_gbphy_dev’ discards ‘const’ qualifier from pointer target type 
[-Wdiscarded-qualifiers]
    76 |         const struct gbphy_device *gbphy_dev = to_gbphy_dev(dev);
|                                                             ^~~
In file included from drivers/staging/greybus/gbphy.c:18:
drivers/staging/greybus/gbphy.h:22:64: note: expected ‘struct device *’ 
but argument is of type ‘const struct device *’
    22 | static inline struct gbphy_device *to_gbphy_dev(struct device *dev)
       | ~~~~~~~~~~~~~~~^~~
```

I tried to figure out the issue but don't know how to solve it, I 
attached the patch. Could you please check it?


Thanks in advance,

Menna

[-- Attachment #2: 0001-staging-greybus-use-inline-function-for-to_gbphy_dev.patch --]
[-- Type: text/x-patch, Size: 1286 bytes --]

From d3125a27fa60987d49610eddae4dd1ace0ec424b Mon Sep 17 00:00:00 2001
From: Menna Mahmoud <eng.mennamahmoud.mm@gmail.com>
Date: Sun, 19 Mar 2023 16:38:31 +0200
Subject: [PATCH] staging: greybus: use inline function for to_gbphy_dev

Convert to_gbphy_dev macro into 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 | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h
index 1de510499480..0a772b90cde3 100644
--- a/drivers/staging/greybus/gbphy.h
+++ b/drivers/staging/greybus/gbphy.h
@@ -15,8 +15,14 @@ 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 *dev)
+{
+	return container_of(dev, struct gbphy_device, dev);
+}
 
 static inline void *gb_gbphy_get_data(struct gbphy_device *gdev)
 {
@@ -44,7 +50,6 @@ struct gbphy_driver {
 
 	struct device_driver driver;
 };
-
 #define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver)
 
 int gb_gbphy_register_driver(struct gbphy_driver *driver,
-- 
2.34.1

Re: warning when converting `to_gbphy_dev` into inline function

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 2790 bytes --]



On Sun, 19 Mar 2023, Menna Mahmoud wrote:

> Hi Julia,
>
> I tried to convert to_gbphy_dev to inline function but got these warnings
> while building:
>
>
> ```
>
> drivers/staging/greybus/gbphy.c: In function ‘gbphy_dev_uevent’:
> drivers/staging/greybus/gbphy.c:76:61: warning: passing argument 1 of
> ‘to_gbphy_dev’ discards ‘const’ qualifier from pointer target type
> [-Wdiscarded-qualifiers]
>    76 |         const struct gbphy_device *gbphy_dev = to_gbphy_dev(dev);

If you look at the code it is complaining about, the context is:

static int gbphy_dev_uevent(const struct device *dev, struct
kobj_uevent_env *env)
{
        const struct gbphy_device *gbphy_dev = to_gbphy_dev(dev);


So dev has been declared to be const, meaning that the current function
should not modify its contents.  But dev is passed to your function
to_gbphy_dev that does not declare that its parameter is const.  So the
compiler is concerned that your function may modify its contents.

There are other calls to to_gbphy_dev where the argument is not declared
as const.  So one could be concerned that a function is not possible,
because some calls have an argument with a const type and other calls have
an argument that does not have a const type.  But this is not a problem.
You cannot pass a const value to a non-const parameter, but you can pass a
non-const value to a const parameter.  And to_gbphy_dev doesn't actually
modify its argument in any way, so changing the parameter type to const
struct device * makes the code compile.

Also, in your patch, you have the parameter named dev and the third
argument of container_of named dev.  This will work, but it is very
confusing, because the two occurrences of dev are not the same thing.  The
first one is the argument value.  The second one is a name that will
be used to construct a structure field reference when the macro is
expanded.  You can use the parameter name that was originally used in the
macro.

When you have a question about a patch, it is better to directly include
it in the message, in the normal way with a --- etc, because then it is
possible to comment on it.  Having it in an attachment is not so
convenient, and it seems that my mailreader has decided to drop the
attachment in the reply...

julia


> |                                                             ^~~
> In file included from drivers/staging/greybus/gbphy.c:18:
> drivers/staging/greybus/gbphy.h:22:64: note: expected ‘struct device *’ but
> argument is of type ‘const struct device *’
>    22 | static inline struct gbphy_device *to_gbphy_dev(struct device *dev)
>       | ~~~~~~~~~~~~~~~^~~
> ```
>
> I tried to figure out the issue but don't know how to solve it, I attached the
> patch. Could you please check it?
>
>
> Thanks in advance,
>
> Menna
>

Re: warning when converting `to_gbphy_dev` into inline function

[Menna Mahmoud]

On ١٩‏/٣‏/٢٠٢٣ ١٧:٣٣, Julia Lawall wrote:
>
> On Sun, 19 Mar 2023, Menna Mahmoud wrote:
>
>> Hi Julia,
>>
>> I tried to convert to_gbphy_dev to inline function but got these warnings
>> while building:
>>
>>
>> ```
>>
>> drivers/staging/greybus/gbphy.c: In function ‘gbphy_dev_uevent’:
>> drivers/staging/greybus/gbphy.c:76:61: warning: passing argument 1 of
>> ‘to_gbphy_dev’ discards ‘const’ qualifier from pointer target type
>> [-Wdiscarded-qualifiers]
>>     76 |         const struct gbphy_device *gbphy_dev = to_gbphy_dev(dev);
> If you look at the code it is complaining about, the context is:
>
> static int gbphy_dev_uevent(const struct device *dev, struct
> kobj_uevent_env *env)
> {
>          const struct gbphy_device *gbphy_dev = to_gbphy_dev(dev);
>
>
> So dev has been declared to be const, meaning that the current function
> should not modify its contents.  But dev is passed to your function
> to_gbphy_dev that does not declare that its parameter is const.  So the
> compiler is concerned that your function may modify its contents.
>
> There are other calls to to_gbphy_dev where the argument is not declared
> as const.  So one could be concerned that a function is not possible,
> because some calls have an argument with a const type and other calls have
> an argument that does not have a const type.  But this is not a problem.
> You cannot pass a const value to a non-const parameter, but you can pass a
> non-const value to a const parameter.  And to_gbphy_dev doesn't actually
> modify its argument in any way, so changing the parameter type to const
> struct device * makes the code compile.


understood, so, no problem to keep the inline function as it is, right?


>
> Also, in your patch, you have the parameter named dev and the third
> argument of container_of named dev.  This will work, but it is very
> confusing, because the two occurrences of dev are not the same thing.  The
> first one is the argument value.  The second one is a name that will
> be used to construct a structure field reference when the macro is
> expanded.  You can use the parameter name that was originally used in the
> macro.


I see, I will fix it.

>
> When you have a question about a patch, it is better to directly include
> it in the message, in the normal way with a --- etc, because then it is
> possible to comment on it.  Having it in an attachment is not so
> convenient, and it seems that my mailreader has decided to drop the
> attachment in the reply...
>
> julia

Okay, I will.


Thanks,

Menna


>
>
>> |                                                             ^~~
>> In file included from drivers/staging/greybus/gbphy.c:18:
>> drivers/staging/greybus/gbphy.h:22:64: note: expected ‘struct device *’ but
>> argument is of type ‘const struct device *’
>>     22 | static inline struct gbphy_device *to_gbphy_dev(struct device *dev)
>>        | ~~~~~~~~~~~~~~~^~~
>> ```
>>
>> I tried to figure out the issue but don't know how to solve it, I attached the
>> patch. Could you please check it?
>>
>>
>> Thanks in advance,
>>
>> Menna
> >

Re: warning when converting `to_gbphy_dev` into inline function

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]



On Sun, 19 Mar 2023, Menna Mahmoud wrote:

>
> On ١٩/٣/٢٠٢٣ ١٧:٣٣, Julia Lawall wrote:
> >
> > On Sun, 19 Mar 2023, Menna Mahmoud wrote:
> >
> > > Hi Julia,
> > >
> > > I tried to convert to_gbphy_dev to inline function but got these warnings
> > > while building:
> > >
> > >
> > > ```
> > >
> > > drivers/staging/greybus/gbphy.c: In function ‘gbphy_dev_uevent’:
> > > drivers/staging/greybus/gbphy.c:76:61: warning: passing argument 1 of
> > > ‘to_gbphy_dev’ discards ‘const’ qualifier from pointer target type
> > > [-Wdiscarded-qualifiers]
> > >     76 |         const struct gbphy_device *gbphy_dev = to_gbphy_dev(dev);
> > If you look at the code it is complaining about, the context is:
> >
> > static int gbphy_dev_uevent(const struct device *dev, struct
> > kobj_uevent_env *env)
> > {
> >          const struct gbphy_device *gbphy_dev = to_gbphy_dev(dev);
> >
> >
> > So dev has been declared to be const, meaning that the current function
> > should not modify its contents.  But dev is passed to your function
> > to_gbphy_dev that does not declare that its parameter is const.  So the
> > compiler is concerned that your function may modify its contents.
> >
> > There are other calls to to_gbphy_dev where the argument is not declared
> > as const.  So one could be concerned that a function is not possible,
> > because some calls have an argument with a const type and other calls have
> > an argument that does not have a const type.  But this is not a problem.
> > You cannot pass a const value to a non-const parameter, but you can pass a
> > non-const value to a const parameter.  And to_gbphy_dev doesn't actually
> > modify its argument in any way, so changing the parameter type to const
> > struct device * makes the code compile.
>
>
> understood, so, no problem to keep the inline function as it is, right?

No, the function has to be changed so that it compiles.  Read the above
again and look at the code to understand why.

julia

Re: warning when converting `to_gbphy_dev` into inline function

[Menna Mahmoud]

On ١٩‏/٣‏/٢٠٢٣ ١٨:٢٠, Julia Lawall wrote:
>
> On Sun, 19 Mar 2023, Menna Mahmoud wrote:
>
>> On ١٩/٣/٢٠٢٣ ١٧:٣٣, Julia Lawall wrote:
>>> On Sun, 19 Mar 2023, Menna Mahmoud wrote:
>>>
>>>> Hi Julia,
>>>>
>>>> I tried to convert to_gbphy_dev to inline function but got these warnings
>>>> while building:
>>>>
>>>>
>>>> ```
>>>>
>>>> drivers/staging/greybus/gbphy.c: In function ‘gbphy_dev_uevent’:
>>>> drivers/staging/greybus/gbphy.c:76:61: warning: passing argument 1 of
>>>> ‘to_gbphy_dev’ discards ‘const’ qualifier from pointer target type
>>>> [-Wdiscarded-qualifiers]
>>>>      76 |         const struct gbphy_device *gbphy_dev = to_gbphy_dev(dev);
>>> If you look at the code it is complaining about, the context is:
>>>
>>> static int gbphy_dev_uevent(const struct device *dev, struct
>>> kobj_uevent_env *env)
>>> {
>>>           const struct gbphy_device *gbphy_dev = to_gbphy_dev(dev);
>>>
>>>
>>> So dev has been declared to be const, meaning that the current function
>>> should not modify its contents.  But dev is passed to your function
>>> to_gbphy_dev that does not declare that its parameter is const.  So the
>>> compiler is concerned that your function may modify its contents.
>>>
>>> There are other calls to to_gbphy_dev where the argument is not declared
>>> as const.  So one could be concerned that a function is not possible,
>>> because some calls have an argument with a const type and other calls have
>>> an argument that does not have a const type.  But this is not a problem.
>>> You cannot pass a const value to a non-const parameter, but you can pass a
>>> non-const value to a const parameter.  And to_gbphy_dev doesn't actually
>>> modify its argument in any way, so changing the parameter type to const
>>> struct device * makes the code compile.
>>
>> understood, so, no problem to keep the inline function as it is, right?
> No, the function has to be changed so that it compiles.  Read the above
> again and look at the code to understand why.


yes, I misunderstood, but fixed now.


Menna

> julia