Date:   Tue, 21 Mar 2023 21:43:42 +0100 (CET)
From:   Julia Lawall <julia.lawall@inria.fr>
To:     Alex Elder <elder@ieee.org>
cc:     Menna Mahmoud <eng.mennamahmoud.mm@gmail.com>,
        gregkh@linuxfoundation.org, outreachy@lists.linux.dev,
        johan@kernel.org, elder@kernel.org, linux-kernel@vger.kernel.org,
        linux-staging@lists.linux.dev
Subject: Re: [PATCH v2] staging: greybus: use inline function for macros
In-Reply-To: <2e869677-2693-6419-ea25-f0cc2efcf3dd@ieee.org>
Message-ID: <alpine.DEB.2.22.394.2303212140480.2919@hadrien>
References: <20230321183456.10385-1-eng.mennamahmoud.mm@gmail.com> <2e869677-2693-6419-ea25-f0cc2efcf3dd@ieee.org>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk


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@inria.fr>
> > Signed-off-by: Menna Mahmoud <eng.mennamahmoud.mm@gmail.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);
>
>