Hi Andy, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> writes: > On Thu, Jul 20, 2023 at 4:40 PM Oleksii Moisieiev > <Oleksii_Moisieiev@xxxxxxxx> wrote: >> andy.shevchenko@xxxxxxxxx writes: >> > Tue, Jun 06, 2023 at 04:22:28PM +0000, Oleksii Moisieiev kirjoitti: > > ... > >> >> + 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. >> ``` > > Precisely. So, why do you intervene in this? > >> 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? > > You can use devm_*(), but what's the point if you call release > yourself? That's quite a red flag usually shows a bigger issue > (misunderstanding of the objects lifetimes and their interaction). > The idea was to follow the way of how pinctrl subsystem manages resources. It assumes that functions, groups and pins should be registered using helper functions pinmux_generic_add_function, pinmux_generic_remove_function, pinconf_generic_add_group, pinconf_generic_remove_group, etc. Which has data as the input parameter and should be freed on pinctrl_unregister call. So pins, groups and functions should live until pinctrl_unregister is called (from remove callback or from devm_pinctrl_dev_release) Unfortunately, I can't use this helpers because pins, funcs and groups should have selector which is understandable by SCMI. pinctrl_scmi_get_function_groups returns pointer to the allocated resources to the caller, so I'm allocating managed resources to be sure that they should be freed on detach. devm_kfree is called only if scmi_get_group_name call was failed while converting group_ids to group_names. I count that as a lack of memory, so I clean allocated groups to give caller a chance to free additional memory and repeat the call. So IMHO devm_* fits here good. What do you think? Sorry for being annoying but I'm trying to understand... -- Thanks, Oleksii
