Hi Tony,
Am 18.06.2018 um 20:17 schrieb Tony Lindgren tony@atomide.com:
- H. Nikolaus Schaller hns@goldelico.com [180618 16:46]:
I can also demonstrate that the duplication has gone:
OK good to hear.
And I was no longer able to reproduce the strcmp(NULL) issue. So it is either better hidden or gone.
It should not be possible with checks preventing registering a group or function with no name. I'll try to repost the whole series tomorrow with that added.
Fine.
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?
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.
BR, Nikolaus