Received-SPF: pass (google.com: domain of linux-kernel+bounces-127995-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240323-pinctrl-scmi-v6-0-a895243257c0@nxp.com>
 <20240323-pinctrl-scmi-v6-3-a895243257c0@nxp.com> <ZgcP4IkTQGks9ehH@surfacebook.localdomain>
 <DU0PR04MB9417E797F4E0F7BB6154B3BE88382@DU0PR04MB9417.eurprd04.prod.outlook.com>
 <Zgu4Tok43W5t8KM0@pluto>
In-Reply-To: <Zgu4Tok43W5t8KM0@pluto>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Tue, 2 Apr 2024 16:06:06 +0300
Message-ID: <CAHp75VdAaTeQ_Ag3gd0s9UfT=kAT2hwibeJ9-YFXJx4z=R3e+g@mail.gmail.com>
Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support
To: Cristian Marussi <cristian.marussi@arm.com>
Cc: Peng Fan <peng.fan@nxp.com>, "Peng Fan (OSS)" <peng.fan@oss.nxp.com>, 
	Sudeep Holla <sudeep.holla@arm.com>, Rob Herring <robh@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Linus Walleij <linus.walleij@linaro.org>, Dan Carpenter <dan.carpenter@linaro.org>, 
	"linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, 
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, Oleksii Moisieiev <oleksii_moisieiev@epam.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 2, 2024 at 10:48=E2=80=AFAM Cristian Marussi
<cristian.marussi@arm.com> wrote:
> On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:

..

> > > > +#include <linux/module.h>
> > > > +#include <linux/scmi_protocol.h>
> > > > +#include <linux/slab.h>
> > >
> > > This is semi-random list of headers. Please, follow IWYU principle (i=
nclude
> > > what you use). There are a lot of inclusions I see missing (just in t=
he context of
> > > this page I see bits.h, types.h, and  asm/byteorder.h).
> >
> > Is there any documentation about this requirement?
> > Some headers are already included by others.

The documentation here is called "a common sense".
The C language is built like this and we expect that nobody will
invest into the dependency hell that we have already, that's why IWYU
principle, please follow it.

> Andy made (mostly) the same remarks on this same patch ~1-year ago on
> this same patch while it was posted by Oleksii.
>
> And I told that time that most of the remarks around devm_ usage were
> wrong due to how the SCMI core handles protocol initialization (using a
> devres group transparently).
>
> This is what I answered that time.
>
> https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
>
> I wont repeat myself, but, in a nutshell the memory allocation like it
> is now is fine: a bit happens via devm_ at protocol initialization, the
> other is doe via explicit kmalloc at runtime and freed via kfree at
> remove time (if needed...i.e. checking the present flag of some structs)

This sounds like a mess. devm_ is expected to be used only for the
->probe() stage, otherwise you may consider cleanup.h (__free() macro)
to have automatic free at the paths where memory is not needed.

And the function naming doesn't suggest that you have a probe-remove
pair. Moreover, if the init-deinit part is called in the probe-remove,
the devm_ must not be mixed with non-devm ones, as it breaks the order
and leads to subtle mistakes.

> I'll made further remarks on v7 that you just posted.

--=20
With Best Regards,
Andy Shevchenko