> 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@xxxxxxx> 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
