Hi Andy, > 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@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%2Flo > > > re > > > .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. Just give a look. But since most scmi firmware drivers are using devm_x APIs in protocol init. I would follow the style to use devm_x as of now. And for pinctrl protocol deinit phase, I will add a comment on why use kfree and what it is to free. For the __free macro, people drop all the scmi firmware drivers using devm_x APIs in init phase in a future patch. Is this ok? Thanks, Peng. > > Thanks, > Peng. > > > > > > I'll made further remarks on v7 that you just posted. > > > > -- > > With Best Regards, > > Andy Shevchenko
