linux-kernel - Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 28 Sep 2016 15:07:46 +0300
From:   Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
To:     Stefan Agner <stefan@...er.ch>,
        Viresh Kumar <viresh.kumar@...aro.org>
CC:     <linus.walleij@...aro.org>, <shawnguo@...nel.org>,
        <aalonso@...escale.com>, <b38343@...escale.com>,
        <ldewangan@...dia.com>, <van.freenix@...il.com>,
        <p.zabel@...gutronix.de>, <linux-gpio@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when
 freeing GPIO



On 09/28/2016 06:38 AM, Stefan Agner wrote:
> On 2016-09-27 19:00, Viresh Kumar wrote:
>> On 27-09-16, 12:34, Stefan Agner wrote:
>>> Added Viresh Kumar to the discussion, he implemented the I2C recovery
>>> functions.
>>>
>>> Yes, reordering the pinctrl/gpio_free calls would fix the problem too.
>>>
>>> However, I guess there is no explicit rule to that ("request/free GPIOs
>>> only when they are muxed as GPIO"), so I think of it that the issue is
>>> actually in the pinctrl driver.
>>>
>>> On top of that it is not entirely trivial to reorder the calls the way
>>> i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now.
>>
>> AFAICT, these routines don't touch the muxing part at all. Perhaps it is done
>> internally by the GPIO calls. Can you please elaborate the exact change you are
>> hinting towards here ?
>
> The i.MX I2C driver touches the pinctrl in its prepare/unprepare
> callbacks.
>
> So, on a i.MX or Vybrid, the call chain looks like this:
>
> i2c_generic_gpio_recovery
>     -> i2c_get_gpios_for_recovery
>        -> gpio_request_one
>     -> i2c_generic_recovery
>        -> prepare_recovery (i2c_imx_prepare_recovery)
>           -> pinctrl_select_state [gpio]
>        -> unprepare_recovery (i2c_imx_unprepare_recovery)
>           -> pinctrl_select_state [default]
>     -> i2c_put_gpios_for_recovery
>        -> gpio_free

I would expect that the change below improves the situation, but I didn't
perform any tests and here the core change is governed by the accepted
i.MX i2c bus driver specific changes, thus conceptually it may be incorrect:

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index da3a02ef4a31..3a4f59c3c3e6 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -697,9 +697,6 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
  	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
  	int i = 0, val = 1, ret = 0;
  
-	if (bri->prepare_recovery)
-		bri->prepare_recovery(adap);
-
  	bri->set_scl(adap, val);
  	ndelay(RECOVERY_NDELAY);
  
@@ -725,22 +722,34 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
  		ndelay(RECOVERY_NDELAY);
  	}
  
-	if (bri->unprepare_recovery)
-		bri->unprepare_recovery(adap);
-
  	return ret;
  }
  
  int i2c_generic_scl_recovery(struct i2c_adapter *adap)
  {
-	return i2c_generic_recovery(adap);
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	int ret;
+
+	if (bri->prepare_recovery)
+		bri->prepare_recovery(adap);
+
+	ret = i2c_generic_recovery(adap);
+
+	if (bri->unprepare_recovery)
+		bri->unprepare_recovery(adap);
+
+	return ret;
  }
  EXPORT_SYMBOL_GPL(i2c_generic_scl_recovery);
  
  int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
  {
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
  	int ret;
  
+	if (bri->prepare_recovery)
+		bri->prepare_recovery(adap);
+
  	ret = i2c_get_gpios_for_recovery(adap);
  	if (ret)
  		return ret;
@@ -748,6 +757,9 @@ int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
  	ret = i2c_generic_recovery(adap);
  	i2c_put_gpios_for_recovery(adap);
  
+	if (bri->unprepare_recovery)
+		bri->unprepare_recovery(adap);
+
  	return ret;
  }
  EXPORT_SYMBOL_GPL(i2c_generic_gpio_recovery);


Alternatively you may consider to add contents of i2c_get_gpios_for_recovery()
into i2c_imx_prepare_recovery(), contents of i2c_put_gpios_for_recovery() into
i2c_imx_unprepare_recovery() in the i.MX I2C bus driver and change the recovery
callback .recover_bus to i2c_generic_scl_recovery().

>
> And for the pinctrl/GPIO driver of Vybrid this is actually a problem
> because gpio_free disables the output driver of the pad, and when that
> happens after the (I2C) default pinctrl state gets selected the pad is
> no longer active.
>

--
With best wishes,
Vladimir