Hi Andy, andy.shevchenko@xxxxxxxxx writes: > Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev kirjoitti: >> scmi-pinctrl driver implements pinctrl driver interface and using >> SCMI protocol to redirect messages from pinctrl subsystem SDK to >> SCP firmware, which does the changes in HW. >> >> This setup expects SCP firmware (or similar system, such as ATF) >> to be installed on the platform, which implements pinctrl driver >> for the specific platform. >> >> SCMI-Pinctrl driver should be configured from the device-tree and uses >> generic device-tree mappings for the configuration. [snip] > ... > >> +error: > > Labels shoud be self-explanatory, i.e. they should tell what _will_ be when goto. > >> + devm_kfree(pmx->dev, pmx->functions[selector].groups); > > Red Flag. Please, elaborate. > Thank you for the review. I did some research regarding this and now I'm confused. Could you please explain to me why it's a red flag? IIUC devm_alloc/free functions are the calls to the resource-managed alloc/free command, which is bound to the device. pinctrl-scmi driver does devm_pinctrl_register_and_init which does devres_alloc and doesn't open devres_group like scmi_alloc_init_protocol_instance (thanks to Cristian detailed explanation). As was mentioned in Documentation/driver-api/driver-model/devres.rst: ``` No matter what, all devres entries are released on driver detach. On release, the associated release function is invoked and then the devres entry is freed. ``` Also there is devm_pinctrl_get call listed in the managed interfaces. My understanding is that all resources, bound to the particular device will be freed on driver detach. Also I found some examples of using devm_alloc/free like from dt_node_to_map call in pinctrl-simple.c driver. I agree that I need to implement .remove callback with proper cleanup, but why can't I use devm_* here? Maybe I've misunderstood your point. -- Thanks, Oleksii
