linux-kernel - Re: [RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200419125058.ldueh7fdswgxocgf@ROU-LT-M43218B.mchp-main.com>
Date:   Sun, 19 Apr 2020 14:50:58 +0200
From:   ludovic.desroches@...rochip.com
To:     Codrin Ciubotariu <codrin.ciubotariu@...rochip.com>
Cc:     linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        nicolas.ferre@...rochip.com, alexandre.belloni@...tlin.com,
        kamel.bouhara@...tlin.com, linux@...linux.org.uk,
        linus.walleij@...aro.org, alan@...tiron.com, wsa@...-dreams.de
Subject: Re: [RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod_get() for bus
 recovery

On Wed, Apr 15, 2020 at 10:06:43AM +0300, Codrin Ciubotariu wrote:
> devm_gpiod_get() usually calls gpio_request_enable() for non-strict pinmux
> drivers. These puts the pins in GPIO mode, whithout notifying the pinctrl
> driver. At this point, the I2C bus no longer owns the pins. To mux the
> pins back to the I2C bus, we use the pinctrl driver to change the state
> of the pins to GPIO, before using devm_gpiod_get(). After the pins are
> received as GPIOs, we switch theer pinctrl state back to the default
> one,
> 
> Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery")
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@...rochip.com>

At the moment, I don't see another way to deal with this issue.

Link to the discussion:
https://lore.kernel.org/linux-arm-kernel/20191206173343.GX25745@shell.armlinux.org.uk/

Acked-by: Ludovic Desroches <ludovic.desroches@...rochip.com>

> ---
>  drivers/i2c/busses/i2c-at91-master.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index 0aba51a7df32..43d85845c897 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -845,6 +845,18 @@ static int at91_init_twi_recovery_info(struct platform_device *pdev,
>  							 PINCTRL_STATE_DEFAULT);
>  	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
>  						      "gpio");
> +	if (IS_ERR(dev->pinctrl_pins_default) ||
> +	    IS_ERR(dev->pinctrl_pins_gpio)) {
> +		dev_info(&pdev->dev, "pinctrl states incomplete for recovery\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * pins will be taken as GPIO, so we might as well inform pinctrl about
> +	 * this and move the state to GPIO
> +	 */
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
> +
>  	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
>  	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
>  		return -EPROBE_DEFER;
> @@ -855,9 +867,7 @@ static int at91_init_twi_recovery_info(struct platform_device *pdev,
>  		return -EPROBE_DEFER;
>  
>  	if (IS_ERR(rinfo->sda_gpiod) ||
> -	    IS_ERR(rinfo->scl_gpiod) ||
> -	    IS_ERR(dev->pinctrl_pins_default) ||
> -	    IS_ERR(dev->pinctrl_pins_gpio)) {
> +	    IS_ERR(rinfo->scl_gpiod)) {
>  		dev_info(&pdev->dev, "recovery information incomplete\n");
>  		if (!IS_ERR(rinfo->sda_gpiod)) {
>  			gpiod_put(rinfo->sda_gpiod);
> @@ -870,6 +880,9 @@ static int at91_init_twi_recovery_info(struct platform_device *pdev,
>  		return -EINVAL;
>  	}
>  
> +	/* change the state of the pins back to their default state */
> +	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
> +
>  	dev_info(&pdev->dev, "using scl, sda for recovery\n");
>  
>  	rinfo->prepare_recovery = at91_prepare_twi_recovery;
> -- 
> 2.20.1
>