Hi Tony,
> Am 19.06.2018 um 06:34 schrieb Tony Lindgren <t...@atomide.com>:
>
> * H. Nikolaus Schaller <h...@goldelico.com> [180618 18:33]:
>>>> So code just needs group cleanup on failed probing and fixing the mutex
>>>> around pinctrl_generic_add_group().
>>>>
>>>> I think we need the mutex because a race still can happen when
>>>> create_pinctrl() is calling pcs_dt_node_to_map()
>>>> and pinctrl_generic_add_group() w/o being locked on pinctrl_maps_mutex.
>>>>
>>>> The race I suspect is that two drivers are trying to insert the same name
>>>> and may come
>>>> both to the conclusion that it does not yet exist. And both insert into
>>>> the radix tree.
>>>>
>>>> The window of risk is small though... It is in pinctrl_generic_add_group()
>>>> between calling
>>>> pinctrl_generic_group_name_to_selector() and radix_tree_insert() so we
>>>> probably won't
>>>> see it in real hardware tests.
>>>
>>> Hmm but that race should be already fixed with mutex held
>>> by the pin controller drivers with these fixes? Or am I
>>> missing something still?
>>
>> Hm. Maybe we refer to a different mutex?
>
> Yes I think that's the case, you're talking about a different
> mutex here :)
>
>> 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.
>>
>> 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).
BR,
Nikolaus
