linux-kernel - Re: [PATCH v2] staging: greybus: use inline function for macros

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.22.394.2303212140480.2919@hadrien>
Date:   Tue, 21 Mar 2023 21:43:42 +0100 (CET)
From:   Julia Lawall <julia.lawall@...ia.fr>
To:     Alex Elder <elder@...e.org>
cc:     Menna Mahmoud <eng.mennamahmoud.mm@...il.com>,
        gregkh@...uxfoundation.org, outreachy@...ts.linux.dev,
        johan@...nel.org, elder@...nel.org, linux-kernel@...r.kernel.org,
        linux-staging@...ts.linux.dev
Subject: Re: [PATCH v2] staging: greybus: use inline function for macros



On Tue, 21 Mar 2023, Alex Elder wrote:

> On 3/21/23 1:34 PM, Menna Mahmoud wrote:
> > Convert `to_gbphy_dev` and `to_gbphy_driver` macros into a
> > static inline function.
> >
> > It is not great to have macros that use the `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@...ia.fr>
> > Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@...il.com>
>
> I'm sorry if this conflicts with what others have said.
>
> But the use of a macro (with a container_of() right-hand
> side) to get at the structure containing a field pointer
> is a widely-used idiom throughout the kernel.
>
> What you propose achieves the same result but I would
> lean toward keeping it as a macro, mainly because it
> is so common.

Common is not necessarily good.  Macros are less safe and less
informative.

julia


>
> 					-Alex
> > ---
> > changes in v2:
> > 	-send patch as a single patch.
> > 	-edit the name of struct object.
> > 	-edit commit message.
> > ---
> >   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..e7ba232bada1 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 *_dev)
> > +{
> > +	return container_of(_dev, 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
> > *drv)
> > +{
> > +	return container_of(drv, struct gbphy_driver, driver);
> > +}
> >     int gb_gbphy_register_driver(struct gbphy_driver *driver,
> >   			     struct module *owner, const char *mod_name);
>
>