linux-kernel - RE: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

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:
 <DU0PR04MB94172B29B33AC0002CF6991B883E2@DU0PR04MB9417.eurprd04.prod.outlook.com>
Date: Tue, 2 Apr 2024 14:01:30 +0000
From: Peng Fan <peng.fan@....com>
To: Andy Shevchenko <andy.shevchenko@...il.com>, Cristian Marussi
	<cristian.marussi@....com>
CC: "Peng Fan (OSS)" <peng.fan@....nxp.com>, Sudeep Holla
	<sudeep.holla@....com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>, Dan Carpenter
	<dan.carpenter@...aro.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-gpio@...r.kernel.org"
	<linux-gpio@...r.kernel.org>, Oleksii Moisieiev <oleksii_moisieiev@...m.com>
Subject: RE: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support

> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
> 
> On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> <cristian.marussi@....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
> > > > (include what you use). There are a lot of inclusions I see
> > > > missing (just in the 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Flinux-arm-kernel%2FZJ78hBcjAhiU%2BZBO%40e120937-
> lin%2F%2
> >
> 3t&data=05%7C02%7Cpeng.fan%40nxp.com%7C3f8c12062db048608e2a08d
> c5315bed
> >
> 0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6384766000583
> 40430%7CUn
> >
> known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I
> k1haW
> >
> wiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Whn3ehZjXy%2BcKG4irlWjQ6
> K3HF%2FofD
> > Yu7j0Lrm8dN5k%3D&reserved=0
> >
> > 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 am new to __free() honestly. I'll let Cristian and Sudeep to comment on
what should I do for v8.

Thanks,
Peng.

> 
> > I'll made further remarks on v7 that you just posted.
> 
> --
> With Best Regards,
> Andy Shevchenko