* H. Nikolaus Schaller hns@goldelico.com [180619 04:54]:
I had seen the call sequence
create_pinctrl()-> pinctrl_dt_to_map() -> pcs_dt_node_to_map() -> pinctrl_generic_add_group()
w/o any lock inside.
So the sequence above has mutex added around adding the pin controller specific functions and groups by the patch series I posted for both pcs_parse_bits_in_pinctrl_entry() and pcs_parse_one_pinctrl_entry(). So I think the above should be fixed now. But please confirm to make sure I'm not mistaken.
There is a mutex_lock(&pinctrl_maps_mutex); in create_pinctrl(), but locked after that.
Or is there a lock outside of create_pinctrl()?
If I look into the stack dumps, call nesting is
driver_probe_device() -> pinctrl_bind_pins() -> devm_pinctrl_get() -> create_pinctrl()
They all do no locking.
Maybe I am missing something.
Can you please post a patch for that as you already have it debugged? That's easier to understand than reading a verbal patch :)
I have no patch for it and all tests were without, but I can suggest a change which IMHO could solve it:
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index eb2b217f5e1e..7d125f9a7804 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -1037,15 +1037,16 @@ static struct pinctrl *create_pinctrl(struct device *dev, INIT_LIST_HEAD(&p->states); INIT_LIST_HEAD(&p->dt_maps);
mutex_lock(&pinctrl_maps_mutex); ret = pinctrl_dt_to_map(p, pctldev); if (ret < 0) { kfree(p);mutex_unlock(&pinctrl_maps_mutex); return ERR_PTR(ret); } devname = dev_name(dev);
mutex_lock(&pinctrl_maps_mutex); /* Iterate over the pin control maps to locate the right ones */ for_each_maps(maps_node, i, map) { /* Map must be for this device */Description: we should also protect pinctrl_dt_to_map(), which calls pinctrl_generic_add_group() and the calls inside pinctrl_generic_add_group() to pinctrl_generic_group_name_to_selector() and radix_tree_insert() with a mutex.
But I haven't tested this case (because it is unlikely to happen and be testable).
Hmm not sure about this one. The groups and functions are specific to the pin controller driver and must be locked by the pin controller driver like this series does. So for pinctrl_generic_add_group() moving the pinctrl_maps_mutex earlier should not help. We already do the locking for generic functions and groups like I described above.
For pinctrl maps moving the mutex earlier might make sense though :) But aren't we doing that already where the list gets modified in pinctrl_register_map() and when searching through the list with for_each_maps? Linus got any comments on this one?
Regards,
Tony