Messages in this thread | | | Subject | Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation | From | Daniel Lezcano <> | Date | Wed, 5 Jan 2022 17:00:16 +0100 |
| |
On 31/12/2021 14:45, Ulf Hansson wrote:
[ ... ]
>> +static struct dtpm *dtpm_setup_dt(const struct dtpm_node *hierarchy, >> + struct dtpm *parent) >> +{ >> + struct dtpm_descr *dtpm_descr; >> + struct device_node *np; >> + int ret; >> + >> + np = of_find_node_by_path(hierarchy->name); >> + if (!np) { >> + pr_err("Failed to find '%s'\n", hierarchy->name); >> + return ERR_PTR(-ENXIO); >> + } >> + >> + for_each_dtpm_table(dtpm_descr) { >> + >> + ret = dtpm_descr->setup(parent, np); > > This will unconditionally call the ->setup callback() for each dtpm > desc in the dtpm table. At this point the ->setup() callback has not > been assigned by anyone that uses DTPM_DECLARE(), so if this would be > called, it would trigger a NULL pointer dereference error. > > On the other hand, we don't have someone calling > dtpm_create_hierarchy() yet, so this code doesn't get exercised, but
Yes, that is the reason why the test is not here.
> it still looks a bit odd to me. Maybe squashing patch2 and patch3 is > an option? Sure
>> + if (ret) { >> + pr_err("Failed to setup '%s': %d\n", hierarchy->name, ret); >> + of_node_put(np); >> + return ERR_PTR(ret); >> + } >> + >> + of_node_put(np); > > This will be called for every loop in the dtpm table. This is wrong, > you only want to call it once, outside the loop.
Right, good catch
>> + } >> + >> + /* >> + * By returning a NULL pointer, we let know the caller there >> + * is no child for us as we are a leaf of the tree >> + */ >> + return NULL; >> +} >> + >> +typedef struct dtpm * (*dtpm_node_callback_t)(const struct dtpm_node *, struct dtpm *); >> + >> +dtpm_node_callback_t dtpm_node_callback[] = { >> + [DTPM_NODE_VIRTUAL] = dtpm_setup_virtual, >> + [DTPM_NODE_DT] = dtpm_setup_dt, >> +}; >> + >> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy, >> + const struct dtpm_node *it, struct dtpm *parent) >> +{ >> + struct dtpm *dtpm; >> + int i, ret; >> + >> + for (i = 0; hierarchy[i].name; i++) { >> + >> + if (hierarchy[i].parent != it) >> + continue; >> + >> + dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent); >> + if (!dtpm || IS_ERR(dtpm)) >> + continue; >> + >> + ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm); > > Why do you need to recursively call dtpm_for_each_child() here? > > Is there a restriction on how the dtpm core code manages adding > children/parents?
[ ... ]
The recursive call is needed given the structure of the tree in an array in order to connect with the parent.
>> + * >> + * struct dtpm_node hierarchy[] = { >> + * [0] { .name = "topmost" }, > > For clarity, I think we should also specify DTPM_NODE_VIRTUAL here. > >> + * [1] { .name = "package", .parent = &hierarchy[0] }, > > Ditto.
Sure
[ ... ]
>> +static int __init init_dtpm(void) >> +{ >> pct = powercap_register_control_type(NULL, "dtpm", NULL); >> if (IS_ERR(pct)) { >> pr_err("Failed to register control type\n"); >> return PTR_ERR(pct); >> } > > It looks like powercap_register_control_type() should be able to be > called from dtpm_create_hierarchy(). In this way we can simply drop > the initcall below, altogether. > > Of course, that assumes that dtpm_create_hierachy() is being called > from a regular module_platform_driver() path - or at least from a > later initcall than fs_initcall(), which is when the "powercap_class" > is being registered. But that sounds like a reasonable assumption we > should be able to make, no?
Yes, agree. Good suggestion, I will do the change.
[ ... ]
>> int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent); >> >> +int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table); > > To start simple, I think dtpm_create_hiearchy() is the sufficient > interface to add at this point. > > However, it's quite likely that it's going to be called from a regular > module (SoC specific platform driver), which means it needs to manage > ->remove() operations too. Anyway, I am fine if we look into that as > improvements on top of the $subject series.
Yes, ATM, the modules can not be unloaded on purpose. The removal can be added later
-- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
| |