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

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx
> Fixes: 39ce8150a079 ("pinctrl: baytrail: Serialize all register access")
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  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.

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