[PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

[PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

[Bjorn Andersson]

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.

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")
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.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;
 		*bit = g->od_bit;
 		*mask = 1;
 		break;
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 63852ed70295..7b8cd1832112 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -51,7 +51,8 @@ struct pinctrl_pin_desc;
  * @mux_bit:              Offset in @ctl_reg for the pinmux function selection.
  * @pull_bit:             Offset in @ctl_reg for the bias configuration.
  * @drv_bit:              Offset in @ctl_reg for the drive strength configuration.
- * @od_bit:               Offset in @ctl_reg for controlling open drain.
+ * @od_bit:               Offset in @ctl_reg for controlling open drain. 0 if
+ *                        not supported by target.
  * @oe_bit:               Offset in @ctl_reg for controlling output enable.
  * @in_bit:               Offset in @io_reg for the input bit value.
  * @out_bit:              Offset in @io_reg for the output bit value.

---
base-commit: 5e4f84f18c4ee9b0ccdc19e39b7de41df21699dd
change-id: 20240424-tlmm-open-drain-8b014c1cfa1a

Best regards,
-- 
Bjorn Andersson <quic_bjorande@quicinc.com>

Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

[Johan Hovold]

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()).

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

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.

> 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.

> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.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.

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

Johan

Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

[Bjorn Andersson]

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@quicinc.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

Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

[Brian Norris]

Hi Johan, Bjorn,

On Thu, Apr 25, 2024 at 5:02 AM Johan Hovold <johan@kernel.org> 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.

Apologies if I broke something here. Both the pinctrl subsystem and
the wide world of diverse QCOM chips can be complicated beasts. I
definitely could have missed things along the way. (And on first
glance, it seems like you may have found one. I definitely did not
consider the gpiod_direction_output() "emulation" behavior here when
submitting this.)

But I can't tell based on subsequent conversation: are you observing a
real problem, or is this a theoretical one that only exists if the
gpiochip driver adds set_config() support?

> > 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()).
>
> Specifically, this means that the software fallback kicks in, which I
> had already verified.
>
> 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.

The ipq4019 binding does:
https://git.kernel.org/linus/99d19f5a48ee6fbc647935de458505e9308078e3

This is used in OpenWrt device trees.

> > 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.

It was quite some time ago that I wrote and tested this, and per the
above, I easily could have missed things. (Plus, the open drain
configuration may not have much practical effect on the systems in
question, so certain errors may not even be observable.)

But I do recall seeing the code in question activate. And inspection
shows that the pinconf_apply_setting() -> ... msm_config_group_set()
path is non-dead code here, for appropriate device trees.

I can try to fire up my development devices again and see what's up if
that helps, but I won't have time to do that in the next few days.

Brian

Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

[Bjorn Andersson]

On Fri, Apr 26, 2024 at 03:08:06PM -0700, Brian Norris wrote:
> Hi Johan, Bjorn,
> 
> On Thu, Apr 25, 2024 at 5:02 AM Johan Hovold <johan@kernel.org> 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.
> 
> Apologies if I broke something here.

False alarm on the breakage part, I got lost in the software layers.

> Both the pinctrl subsystem and
> the wide world of diverse QCOM chips can be complicated beasts. I
> definitely could have missed things along the way. (And on first
> glance, it seems like you may have found one. I definitely did not
> consider the gpiod_direction_output() "emulation" behavior here when
> submitting this.)
> 
> But I can't tell based on subsequent conversation: are you observing a
> real problem, or is this a theoretical one that only exists if the
> gpiochip driver adds set_config() support?
> 

There is a problem that if a non-ipq4019 device where to be pinconf'ed
for open-drain, the outcome would be unexpected and I have a concern
that someone one day would implement set_config().

So, I'd like to fix this, but my argumentation is at least wrong.

> > > 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()).
> >
> > Specifically, this means that the software fallback kicks in, which I
> > had already verified.
> >
> > 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.
> 
> The ipq4019 binding does:
> https://git.kernel.org/linus/99d19f5a48ee6fbc647935de458505e9308078e3
> 

Perhaps we could convert that to yaml?

> This is used in OpenWrt device trees.
> 

Thanks, I couldn't find a user, so this was helpful input for deciding
the path forward.

> > > 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.
> 
> It was quite some time ago that I wrote and tested this, and per the
> above, I easily could have missed things. (Plus, the open drain
> configuration may not have much practical effect on the systems in
> question, so certain errors may not even be observable.)
> 
> But I do recall seeing the code in question activate. And inspection
> shows that the pinconf_apply_setting() -> ... msm_config_group_set()
> path is non-dead code here, for appropriate device trees.
> 

Thank you for taking a look, Brian. This was valuable input. I will
rework this to have a valid motivation - at least.

> I can try to fire up my development devices again and see what's up if
> that helps, but I won't have time to do that in the next few days.
> 

As my observation was incorrect, I don't think that is urgent.

Regards,
Bjorn

Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

[Brian Norris]

Hi Bjorn,

On Fri, Apr 26, 2024 at 4:44 PM Bjorn Andersson
<quic_bjorande@quicinc.com> wrote:
> On Fri, Apr 26, 2024 at 03:08:06PM -0700, Brian Norris wrote:
> > Apologies if I broke something here.
>
> False alarm on the breakage part, I got lost in the software layers.

OK! Glad it's not causing a visible problem.

> > But I can't tell based on subsequent conversation: are you observing a
> > real problem, or is this a theoretical one that only exists if the
> > gpiochip driver adds set_config() support?
> >
>
> There is a problem that if a non-ipq4019 device where to be pinconf'ed
> for open-drain, the outcome would be unexpected

Well as observed elsewhere, that's not permitted in most MSM bindings
;) But still might be nice to remove the landmine.

> and I have a concern
> that someone one day would implement set_config().
>
> So, I'd like to fix this, but my argumentation is at least wrong.

Sure.

I haven't surveyed the other pinconf types well, but how does this
driver handle all that? Are all other types supported uniformly by all
qcom pin blocks? It seems a little weird to be treating bit 0 as a
NULL choice, but clearly it works for now, with only a single non-zero
bit user.

> > https://git.kernel.org/linus/99d19f5a48ee6fbc647935de458505e9308078e3
>
> Perhaps we could convert that to yaml?

Ha, sure, perhaps. I don't have time for that soon, but there's a
chance such a patch could materialize in the future.

> Thank you for taking a look, Brian. This was valuable input. I will
> rework this to have a valid motivation - at least.

You're welcome! Glad it's cleared up enough to help move forward.

Regards,
Brian

Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

[Linus Walleij]

On Thu, Apr 25, 2024 at 5:46 AM Bjorn Andersson
<quic_bjorande@quicinc.com> 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.
>
> 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")
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

I tried to follow the discussion but couldn't get to a verdict on this patch,
should it be applied or not, and if it should be applied, should the Fixes:
tag be dropped or left and considered a nonurgent fix as it does not
affect current behaviour?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: qcom: Fix behavior in abscense of open-drain support

[Johan Hovold]

On Fri, May 03, 2024 at 09:28:41AM +0200, Linus Walleij wrote:
> On Thu, Apr 25, 2024 at 5:46 AM Bjorn Andersson
> <quic_bjorande@quicinc.com> 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.
> >
> > 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")
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> 
> I tried to follow the discussion but couldn't get to a verdict on this patch,
> should it be applied or not, and if it should be applied, should the Fixes:
> tag be dropped or left and considered a nonurgent fix as it does not
> affect current behaviour?

It should not be applied in its current form (e.g. as the commit message
is incorrect). Bjorn will be sending a v2.

Johan