[PATCH] pinctrl: move subsystem mutex to pinctrl_dev struct

[Patrice CHOTARD]

On 03/28/2013 12:33 AM, Stephen Warren wrote:

> On 03/27/2013 03:45 PM, Linus Walleij wrote:
>> On Thu, Mar 14, 2013 at 5:59 PM, Patrice CHOTARD <patrice.chotard at st.com> wrote:
>>> On 03/13/2013 07:22 PM, Stephen Warren wrote:
>>>
>>>> On 03/13/2013 09:44 AM, Linus Walleij wrote:
>>>>> From: Patrice Chotard <patrice.chotard at st.com>
>>>>>
>>>>> This mutex avoids deadlock in case of use of multiple pin
>>>>> controllers. Before this modification, by using a global
>>>>> mutex, deadlock appeared when, for example, a call to
>>>>> pinctrl_pins_show() locked the pinctrl_mutex, called the
>>>>> ops->pin_dbg_show of a particular pin controller. If this
>>>>> pin controller needs I2C access to retrieve configuration
>>>>> information and I2C driver is using pinctrl to drive its
>>>>> pins, a call to pinctrl_select_state() try to lock again
>>>>> pinctrl_mutex which leads to a deadlock.
>>>>>
>>>>> Notice that the mutex grab from the two direction functions
>>>>> was moved into pinctrl_gpio_direction().
>>>>>
>>>>> For two cases, we can't replace pinctrl_mutex by
>>>>> pctldev->mutex, because at this stage, pctldev is
>>>>> not accessible :
>>>>>      - pinctrl_get()/pinctrl_put()
>>>>>      - pinctrl_register_maps()
>>>>>
>>>>> So add respectively pinctrl_list_mutex and
>>>>> pinctrl_maps_mutex in order to protect
>>>>> pinctrl_list and pinctrl_maps list instead.
>>>>
>>>> I can't see how this would be safe, or even how it would solve the
>>>> problem (and still be safe).
>>>>
>>>> In the scenario described above, pinctrl_pins_show() would need to lock
>>>> the list mutex since it's interacting with the list of pinctrl devices.
>>>> Then, the I2C drivers'c pinctrl_select() also needs to acquire that same
>>>> lock, since it's interacting with another entry in that same list in
>>>> order to re-program the other pinctrl device to route I2C to the correct
>>>> location.
>>>>
>>>
>>> Hi Stephen,
>>>
>>> Thanks for your review.
>>>
>>> I don't understand why, in pinctrl_pins_show(), pinctrl_list_mutex
>>> should be locked whereas pinctrl_list is not updated or parsed ?
> 
> Sorry for the slow response.
> 
> If pinctrl_pins_show() calls into a pin controller, then the list of pin
> controllers must be locked to prevent that pin controller from being
> destroyed while the "show" code is still using it.
> 


Hi all,

Correct me if i am wrong, but i can't see any issue.

A pin controller can't be destroyed while a pinctrl_pins_show() call. In
both pinctrl_pins_show() and pinctrl_unregister(),
mutex_lock(&pctldev->mutex) is perfomed. So during pinctrl_pins_show()
execution, a pinctrl can't be unregistered/removed.


> I suppose an alternative would be module_get() the relevant driver's
> module to prevent it from being unloaded. I'm not sure if that would
> remove the need to scan through the list of pin controllers (which would
> then require taking the lock), or whether something else knows which
> driver is related to each debugfs file so that no list lookup is
> required? Perhaps debugfs already does that internally somehow?
> 
>>>> So, I can't see how separating out the map lock would make any difference.
>>>>
>>>> Also, why is the map lock relevant here at all anyway? The I2C mux's
>>>> probe() should have executed pinctrl_get(), and isn't the map parsed at
>>>> that time, and converted to a struct pinctrl, and hence any later call
>>>> to pinctrl_select() doesn't touch the map?
>>>
>>> Sorry, but i don't follow what you mean ....
>>> In create_pinctrl(), maps is protected by pinctrl_maps_mutex.
> 
>>> I don't understand the link between maps and pinctrl_select(),
>>> pinctrl_select_state_locked() doesn't touch the map.
> 
> Yes, pinctrl_select() shouldn't touch the map since it's already been
> parsed.
> 
> But if there's a per-pinctrl-driver lock, then pinctrl_select() needs to
> lock all those locks for each driver referenced by a struct
> pinctrl_state entry.
> 
> Perhaps it doesn't need to hold more than one of those at a time though;
> that might help remove any possibility of deadlock.


Ok, regarding pinctrl_select(), i will propose a new patch version which
hold the per-pincontrol-driver lock referenced by each struct
pinctrl_state entry.

> 
>>>> Is there a recursive lock type that could be used instead? I'm not sure
>>>> if that'd still be safe though.
>>>>
>>>> Finally, a long while ago when I removed these separate locks and
>>>> created the single lock, I raised a slew of complex points re: why it
>>>> was extremely hard to split up the locking. I talked about a number of
>>>> AB/BA deadlock cases IIRC mostly w.r.t pinctrl device registration. Were
>>>> those considered when writing this patch? What's the solution?
>>>
>>> I suppose you make reference to the comment you put on this commit ?
>>> 57b676f9c1b7cd84397fe5a86c9bd2788ac4bd32 : pinctrl: fix and simplify
>>> locking
> 
> Yes, that commit and the email thread surrounding it.
> 
>>> Added in CC Seraphin Bonnafe as he has reported the deadlock issue using
>>> several pin ctrollers.
>>
>> Any reaction to this?
> 
> I was hoping that Patrice would go through the various issues I raised
> in that commit log and the surrounding email discussion and point out
> exactly why the proposed locking scheme would not cause the problems I
> mentioned there.
> 
>> I can intuitively agree with the idea that the locking should not
>> be for the entire subsystem but preferably per-controller.
> 
> Yes, that would be great if it doesn't introduce any issues.
> 
>> Indeed that commit says:
>>
>>     Solving this requires the introduction of a higher-level lock, at least
>>     a lock per pin controller, which both gpio range registration and
>>     pinctrl_get()/put() will acquire.
>>
>> So this is the "atleast".
> 
> Yup.
> 
>> Then it says:
>>
>>     Related, a future change will add a
>>     "complete" op to the pin controller drivers, the idea being that each
>>     state's programming will be programmed into the pinctrl driver followed
>>     by the "complete" call,
>>
>> Hm that is actually quite useful but we haven't got around to
>> doing that, and it should be able to use the same per-controller mutex.
> 
> Indeed. I guess nobody ended up caring about that optimization yet.
> Still, it's something that should be easy to add any time it's useful.
> 
>> And that of course brings into the question of locking when accessing
>> the list of such controllers, or the maps, neither of which are part
>> of any controller. So these needs to be a separate mutexes.
>>
>> The old locking would be per-descriptor but this patch preserves
>> part of the reform in Stephen's patch, it keeps one big lock for
>> everything happening in a certain pin controller, but brings back
>> the two locks for lists and maps.
> 
> OK.
> 
>> The commit message also states:
>>
>>     However, each pinctrl mapping table entry may affect a different pin
>>     controller if necessary. Hence, with a per-pin-controller lock, almost
>>     any pinctrl API may need to acquire multiple locks, one per controller.
>>     To avoid deadlock, these would need to be acquired in the same order in
>>     all cases. This is extremely difficult to implement in the case of
>>     pinctrl_get(), which doesn't know which pin controllers to lock until it
>>     has parsed the entire mapping table, since it contains somewhat arbitrary
>>     data.
>>
>> This is the real problem, right?
> 
> That's one issue.
> 
> Perhaps if we acquire and release the locks for each controller as we
> process each individual entry in struct pinctrl_state, we'll never hold
> more than one per-pinctrl-driver lock at once, so there won't be any
> possibility of ABBA locking problems across multiple pinctrl drivers.
> 
> But there's at least one more issue that I vaguely recall now: When
> registering a pinctrl driver, the pinctrl core can automatically select
> that device's own default state (a/k/a hogging). pinctrl_register()
> would need to hold the list lock since it's manipulating the list.
> However, if we re-use the pinctrl_select() code, then that requires
> recursive locking; the lock is held once by pinctrl_register, and would
> need to be taken during map->pinctrl_state conversion in order to look
> up the pinctrl driver for each map entry. There was some reason it
> didn't make sense to first register the new pinctrl driver, then drop
> the list lock, then apply the hogs outside the lock, but I forget what
> that was:-( Perhaps I was mistaken here. Or perhaps, I was just trying
> to avoid many lock/unlock operations during register, and I should have
> just not tried to avoid that.
> 
>> This patch will just take the pinctrl_list_mutex in the pinctrl_get()
>> path. If it then ends up in create_pinctrl() from there
>> it is iterating the map, and should thus also take the
>> pinctrl_maps_mutex, which it does.
>>
>> I don't quite see how taking these two orthogonal mutexes would
>> be so complicated to get right, so please help me out here.
> 
> The main issue I was trying to avoid was deadlock due to ABBA lock
> acquisition.