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]
Message-ID: <Zipe7u/9ajRXa81U@hu-bjorande-lv.qualcomm.com>
Date: Thu, 25 Apr 2024 06:47:26 -0700
From: Bjorn Andersson <quic_bjorande@...cinc.com>
To: Johan Hovold <johan@...nel.org>
CC: Bjorn Andersson <andersson@...nel.org>,
        Linus Walleij
	<linus.walleij@...aro.org>,
        Brian Norris <computersforpeace@...il.com>,
        Jaiganesh Narayanan <njaigane@...eaurora.org>,
        Doug Anderson
	<dianders@...omium.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain
 support

On Thu, Apr 25, 2024 at 02:02:14PM +0200, Johan Hovold wrote:
> On Wed, Apr 24, 2024 at 08:45:31PM -0700, Bjorn Andersson wrote:
> > When a GPIO is configured as OPEN_DRAIN gpiolib will in
> > gpiod_direction_output() attempt to configure the open-drain property of
> > the hardware and if this fails fall back to software emulation of this
> > state.
> > 
> > The TLMM block in most Qualcomm platform does not implement such
> > functionality, so this call would be expected to fail. But due to lack
> > of checks for this condition, the zero-initialized od_bit will cause
> > this request to silently corrupt the lowest bit in the config register
> > (which typically is part of the bias configuration) and happily continue
> > on.
> > 
> > Fix this by checking if the od_bit value is unspecified and if so fail
> > the request to avoid the unexpected state, and to make sure the software
> > fallback actually kicks in.
> 
> Fortunately, this is currently not a problem as the gpiochip driver does
> not implement the set_config() callback, which means that the attempt to
> change the pin configuration currently always fails with -ENOTSUP (see
> gpio_do_set_config()).
> 

You're right. I was convinced that I implemented set_config() and got
lost in the indirections.

> Specifically, this means that the software fallback kicks in, which I
> had already verified.
> 

I thought you did, and found this strange.

> Now, perhaps there is some other path which can allow you to end up
> here, but it's at least not via gpiod_direction_output().
> 
> The msm pinctrl binding does not allow 'drive-open-drain' so that path
> should also be ok unless you have a non-conformant devicetree.
> 

Looking at it again, I believe you're right and this is currently dead
code, waiting to screw us over once someone opens up the code path I
thought I fixed...

> > It is assumed for now that no implementation will come into existence
> > with BIT(0) being the open-drain bit, simply for convenience sake.
> > 
> > Fixes: 13355ca35cd1 ("pinctrl: qcom: ipq4019: add open drain support")
> 
> I guess hardware open-drain mode has never been properly tested on
> ipq4019.
> 

I see no other explanation. Perhaps there were additional changes in the
downstream tree where that change came from.

> > Signed-off-by: Bjorn Andersson <quic_bjorande@...cinc.com>
> > ---
> >  drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
> >  drivers/pinctrl/qcom/pinctrl-msm.h | 3 ++-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index aeaf0d1958f5..329474dc21c0 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -313,6 +313,8 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
> >  			*mask |= BIT(g->i2c_pull_bit) >> *bit;
> >  		break;
> >  	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > +		if (!g->od_bit)
> > +			return -EOPNOTSUPP;
> 
> I believe this should be -ENOTSUPP, which the rest of the driver and
> subsystem appear to use.
> 

Both error codes are used in across gpio/pinctrl subsystems. I first
went ENOTSUPP but folded, perhaps too easily, when checkpatch told me
EOPNOTSUPP was better.


I'm leaning towards us reverting the ipq4019 patch, rather than try
complete the patch, but will give this some more thought before spinning
v2.

Thank you,
Bjorn

> >  		*bit = g->od_bit;
> >  		*mask = 1;
> >  		break;
> 
> Johan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ