Re: [PATCH] pinctrl: baytrail: Really serialize all register accesses

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 19-11-2019 09:19, Mika Westerberg wrote:
> On Mon, Nov 18, 2019 at 03:20:20PM +0100, Hans de Goede wrote:
>> Commit 39ce8150a079 ("pinctrl: baytrail: Serialize all register access")
>> added a spinlock around all register accesses because:
>>
>> "There is a hardware issue in Intel Baytrail where concurrent GPIO register
>>   access might result reads of 0xffffffff and writes might get dropped
>>   completely."
>>
>> Testing has shown that this does not catch all cases, there are still
>> 2 problems remaining
>>
>> 1) The original fix uses a spinlock per byt_gpio device / struct,
>> additional testing has shown that this is not sufficient concurent
>> accesses to 2 different GPIO banks also suffer from the same problem.
>>
>> This commit fixes this by moving to a single global lock.
>>
>> 2) The original fix did not add a lock around the register accesses in
>> the suspend/resume handling.
>>
>> Since pinctrl-baytrail.c is using normal suspend/resume handlers,
>> interrupts are still enabled during suspend/resume handling. Nothing
>> should be using the GPIOs when they are being taken down, _but_ the
>> GPIOs themselves may still cause interrupts, which are likely to
>> use (read) the triggering GPIO. So we need to protect against
>> concurrent GPIO register accesses in the suspend/resume handlers too.
>>
>> This commit fixes this by adding the missing spin_lock / unlock calls.
>>
>> The 2 fixes together fix the Acer Switch 10 SW5-012 getting completely
>> confused after a suspend resume. The DSDT for this device has a bug
>> in its _LID method which reprograms the home and power button trigger-
>> flags requesting both high and low _level_ interrupts so the IRQs for
>> these 2 GPIOs continuously fire. This combined with the saving of
>> registers during suspend, triggers concurrent GPIO register accesses
>> resulting in saving 0xffffffff as pconf0 value during suspend and then
>> when restoring this on resume the pinmux settings get all messed up,
>> resulting in various I2C busses being stuck, the wifi no longer working
>> and often the tablet simply not coming out of suspend at all.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 39ce8150a079 ("pinctrl: baytrail: Serialize all register access")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/pinctrl/intel/pinctrl-baytrail.c | 81 +++++++++++++-----------
>>   1 file changed, 44 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
>> index b18336d42252..1b289f64c3a2 100644
>> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
>> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
>> @@ -111,7 +111,6 @@ struct byt_gpio {
>>   	struct platform_device *pdev;
>>   	struct pinctrl_dev *pctl_dev;
>>   	struct pinctrl_desc pctl_desc;
>> -	raw_spinlock_t lock;
>>   	const struct intel_pinctrl_soc_data *soc_data;
>>   	struct intel_community *communities_copy;
>>   	struct byt_gpio_pin_context *saved_context;
>> @@ -550,6 +549,8 @@ static const struct intel_pinctrl_soc_data *byt_soc_data[] = {
>>   	NULL
>>   };
>>   
>> +static DEFINE_RAW_SPINLOCK(byt_gpio_lock);
> 
> Can we call it byt_lock instead? Following same convention we use in
> chv.

Ok, v2 with this changed coming up.

> Other than that looks good and definitely right thing to do. Thanks for
> doing this Hans!

You are welcome. I must say that this was an interesting adventure :)
The interrupt storm issue on the Acer SW5-012 really managed to hit this bug
very reliably, resulting in all sorts of fun due to the pinmux settings
getting messed up.

Regards,

Hans