Suspicious debounce handling code in pintctrl-baytrail

Suspicious debounce handling code in pintctrl-baytrail

[Jean Delvare]

Hi Cristina,

In this commit:

commit 658b476c742fe379e7020309fd590a27b457a4c1
Date:   Fri Apr 1 14:00:07 2016 +0300

    pinctrl: baytrail: Add debounce configuration

you added support for getting and setting debounce configuration for
the Baytrail pins. Now gcc complains about the following:

  CC [M]  drivers/pinctrl/intel/pinctrl-baytrail.o
drivers/pinctrl/intel/pinctrl-baytrail.c: In function ‘byt_pin_config_set’:
drivers/pinctrl/intel/pinctrl-baytrail.c:1181:17: warning: variable ‘debounce’ set but not used [-Wunused-but-set-variable]
  u32 conf, val, debounce;
                 ^

I looked at the code, and it indeed looks wrong. You are reading the
BYT_DEBOUNCE_REG register, clearing the debounce time bits from it, and
then writing the debounce time bits to the *BYT_CONF0_REG* register.
This is certainly corrupting the chip configuration, as the
configuration register bits 2-0 have a completely different meaning
(mux configuration.)

Did you ever test that code?

Additionally, I suspect that in the "get" path, the following is wrong:

	case PIN_CONFIG_INPUT_DEBOUNCE:
		if (!(conf & BYT_DEBOUNCE_EN))
			return -EINVAL;

I'm not familiar with the pinctrl interface but my understanding of
the PIN_CONFIG_INPUT_DEBOUNCE documentation is that the driver must
return 0 if debouncing is disabled. The "set" path would also need to
handle this case properly (setting BYT_DEBOUNCE_EN if debounce timing is
non-zero, clearing it if value is zero.)

-- 
Jean Delvare
SUSE L3 Support

Re: Suspicious debounce handling code in pintctrl-baytrail

[Mika Westerberg]

On Thu, Jan 26, 2017 at 02:20:20PM +0100, Jean Delvare wrote:
> Hi Cristina,
> 
> In this commit:
> 
> commit 658b476c742fe379e7020309fd590a27b457a4c1
> Date:   Fri Apr 1 14:00:07 2016 +0300
> 
>     pinctrl: baytrail: Add debounce configuration
> 
> you added support for getting and setting debounce configuration for
> the Baytrail pins. Now gcc complains about the following:
> 
>   CC [M]  drivers/pinctrl/intel/pinctrl-baytrail.o
> drivers/pinctrl/intel/pinctrl-baytrail.c: In function ‘byt_pin_config_set’:
> drivers/pinctrl/intel/pinctrl-baytrail.c:1181:17: warning: variable ‘debounce’ set but not used [-Wunused-but-set-variable]
>   u32 conf, val, debounce;
>                  ^
> 
> I looked at the code, and it indeed looks wrong. You are reading the
> BYT_DEBOUNCE_REG register, clearing the debounce time bits from it, and
> then writing the debounce time bits to the *BYT_CONF0_REG* register.
> This is certainly corrupting the chip configuration, as the
> configuration register bits 2-0 have a completely different meaning
> (mux configuration.)

Yes, you are right.

This is fixed by following commit from Andy:

https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-pinctrl.git/commit/?h=fixes&id=04ff5a095d662e0879f0eb04b9247e092210aeff

Re: Suspicious debounce handling code in pintctrl-baytrail

[Jean Delvare]

Hi Mika, hello Andy,

On Thu, 26 Jan 2017 15:43:41 +0200, Mika Westerberg wrote:
> On Thu, Jan 26, 2017 at 02:20:20PM +0100, Jean Delvare wrote:
> > Hi Cristina,
> > 
> > In this commit:
> > 
> > commit 658b476c742fe379e7020309fd590a27b457a4c1
> > Date:   Fri Apr 1 14:00:07 2016 +0300
> > 
> >     pinctrl: baytrail: Add debounce configuration
> > 
> > you added support for getting and setting debounce configuration for
> > the Baytrail pins. Now gcc complains about the following:
> > 
> >   CC [M]  drivers/pinctrl/intel/pinctrl-baytrail.o
> > drivers/pinctrl/intel/pinctrl-baytrail.c: In function ‘byt_pin_config_set’:
> > drivers/pinctrl/intel/pinctrl-baytrail.c:1181:17: warning: variable ‘debounce’ set but not used [-Wunused-but-set-variable]
> >   u32 conf, val, debounce;
> >                  ^
> > 
> > I looked at the code, and it indeed looks wrong. You are reading the
> > BYT_DEBOUNCE_REG register, clearing the debounce time bits from it, and
> > then writing the debounce time bits to the *BYT_CONF0_REG* register.
> > This is certainly corrupting the chip configuration, as the
> > configuration register bits 2-0 have a completely different meaning
> > (mux configuration.)
> 
> Yes, you are right.
> 
> This is fixed by following commit from Andy:
> 
> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-pinctrl.git/commit/?h=fixes&id=04ff5a095d662e0879f0eb04b9247e092210aeff

Thanks for the pointer, I wasn't aware of that patch. Looks overall
good, but:

+			case 0:
+				conf &= BYT_DEBOUNCE_EN;
+				break;

looks suspicious to me. I think ~BYT_DEBOUNCE_EN was intended?
Furthermore I would expect conf |= BYT_DEBOUNCE_EN in the other cases,
otherwise disabling debounce once will prevent you from enabling it
forever.

Also Andy's patch doesn't seem to address my second point:

> Additionally, I suspect that in the "get" path, the following is wrong:
> 
> 	case PIN_CONFIG_INPUT_DEBOUNCE:
> 		if (!(conf & BYT_DEBOUNCE_EN))
> 			return -EINVAL;
> 
> I'm not familiar with the pinctrl interface but my understanding of
> the PIN_CONFIG_INPUT_DEBOUNCE documentation is that the driver must
> return 0 if debouncing is disabled.

?

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: Suspicious debounce handling code in pintctrl-baytrail

[Mika Westerberg]

On Thu, Jan 26, 2017 at 03:00:46PM +0100, Jean Delvare wrote:
> Hi Mika, hello Andy,
> 
> On Thu, 26 Jan 2017 15:43:41 +0200, Mika Westerberg wrote:
> > On Thu, Jan 26, 2017 at 02:20:20PM +0100, Jean Delvare wrote:
> > > Hi Cristina,
> > > 
> > > In this commit:
> > > 
> > > commit 658b476c742fe379e7020309fd590a27b457a4c1
> > > Date:   Fri Apr 1 14:00:07 2016 +0300
> > > 
> > >     pinctrl: baytrail: Add debounce configuration
> > > 
> > > you added support for getting and setting debounce configuration for
> > > the Baytrail pins. Now gcc complains about the following:
> > > 
> > >   CC [M]  drivers/pinctrl/intel/pinctrl-baytrail.o
> > > drivers/pinctrl/intel/pinctrl-baytrail.c: In function ‘byt_pin_config_set’:
> > > drivers/pinctrl/intel/pinctrl-baytrail.c:1181:17: warning: variable ‘debounce’ set but not used [-Wunused-but-set-variable]
> > >   u32 conf, val, debounce;
> > >                  ^
> > > 
> > > I looked at the code, and it indeed looks wrong. You are reading the
> > > BYT_DEBOUNCE_REG register, clearing the debounce time bits from it, and
> > > then writing the debounce time bits to the *BYT_CONF0_REG* register.
> > > This is certainly corrupting the chip configuration, as the
> > > configuration register bits 2-0 have a completely different meaning
> > > (mux configuration.)
> > 
> > Yes, you are right.
> > 
> > This is fixed by following commit from Andy:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-pinctrl.git/commit/?h=fixes&id=04ff5a095d662e0879f0eb04b9247e092210aeff
> 
> Thanks for the pointer, I wasn't aware of that patch. Looks overall
> good, but:
> 
> +			case 0:
> +				conf &= BYT_DEBOUNCE_EN;
> +				break;
> 
> looks suspicious to me. I think ~BYT_DEBOUNCE_EN was intended?

Good catch! Yes, definitely it should be ~BYT_DEBOUNCE_EN.

> Furthermore I would expect conf |= BYT_DEBOUNCE_EN in the other cases,
> otherwise disabling debounce once will prevent you from enabling it
> forever.
> 
> Also Andy's patch doesn't seem to address my second point:
> 
> > Additionally, I suspect that in the "get" path, the following is wrong:
> > 
> > 	case PIN_CONFIG_INPUT_DEBOUNCE:
> > 		if (!(conf & BYT_DEBOUNCE_EN))
> > 			return -EINVAL;
> > 
> > I'm not familiar with the pinctrl interface but my understanding of
> > the PIN_CONFIG_INPUT_DEBOUNCE documentation is that the driver must
> > return 0 if debouncing is disabled.

Indeed, I also agree it should return 0 here.

We will fix this, thanks!

Re: Suspicious debounce handling code in pintctrl-baytrail

[Shevchenko, Andriy]

On Thu, 2017-01-26 at 15:00 +0100, Jean Delvare wrote:
> Hi Mika, hello Andy,
> 
> On Thu, 26 Jan 2017 15:43:41 +0200, Mika Westerberg wrote:
> > On Thu, Jan 26, 2017 at 02:20:20PM +0100, Jean Delvare wrote:

> Thanks for the pointer, I wasn't aware of that patch. Looks overall
> good, but:
> 
> +			case 0:
> +				conf &= BYT_DEBOUNCE_EN;
> +				break;
> 
> looks suspicious to me. I think ~BYT_DEBOUNCE_EN was intended?

Yes!

> Furthermore I would expect conf |= BYT_DEBOUNCE_EN in the other cases,
> otherwise disabling debounce once will prevent you from enabling it
> forever.

Yes.

> Also Andy's patch doesn't seem to address my second point:
> 
> > Additionally, I suspect that in the "get" path, the following is
> > wrong:
> > 
> > 	case PIN_CONFIG_INPUT_DEBOUNCE:
> > 		if (!(conf & BYT_DEBOUNCE_EN))
> > 			return -EINVAL;
> > 
> > I'm not familiar with the pinctrl interface but my understanding of
> > the PIN_CONFIG_INPUT_DEBOUNCE documentation is that the driver must
> > return 0 if debouncing is disabled.
> 
> ?

I need to check this.

I will address your comments. Thanks for report!

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

Re: Suspicious debounce handling code in pintctrl-baytrail

[Shevchenko, Andriy]

On Thu, 2017-01-26 at 16:13 +0200, Andy Shevchenko wrote:
> On Thu, 2017-01-26 at 15:00 +0100, Jean Delvare wrote:
> > Hi Mika, hello Andy,
> > 

> > Additionally, I suspect that in the "get" path, the following is
> > > wrong:
> > > 
> > > 	case PIN_CONFIG_INPUT_DEBOUNCE:
> > > 		if (!(conf & BYT_DEBOUNCE_EN))
> > > 			return -EINVAL;
> > > 
> > > I'm not familiar with the pinctrl interface but my understanding
> > > of
> > > the PIN_CONFIG_INPUT_DEBOUNCE documentation is that the driver
> > > must
> > > return 0 if debouncing is disabled.
> > 
> > ?

No, this is correct.
"...if it is available but disabled it should return -EINVAL"

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

Re: Suspicious debounce handling code in pintctrl-baytrail

[Jean Delvare]

On Thu, 26 Jan 2017 16:08:11 +0200, Mika Westerberg wrote:
> We will fix this, thanks!

Great, thanks.

BTW I think that Andy's patch which fixes a bug that "even might break
something physically" would deserve to be Cc'd to stable@ (once the fix
itself is fixed, that is.)

-- 
Jean Delvare
SUSE L3 Support

Re: Suspicious debounce handling code in pintctrl-baytrail

[mika.westerberg]

On Thu, Jan 26, 2017 at 02:15:21PM +0000, Shevchenko, Andriy wrote:
> On Thu, 2017-01-26 at 16:13 +0200, Andy Shevchenko wrote:
> > On Thu, 2017-01-26 at 15:00 +0100, Jean Delvare wrote:
> > > Hi Mika, hello Andy,
> > > 
> 
> > > Additionally, I suspect that in the "get" path, the following is
> > > > wrong:
> > > > 
> > > > 	case PIN_CONFIG_INPUT_DEBOUNCE:
> > > > 		if (!(conf & BYT_DEBOUNCE_EN))
> > > > 			return -EINVAL;
> > > > 
> > > > I'm not familiar with the pinctrl interface but my understanding
> > > > of
> > > > the PIN_CONFIG_INPUT_DEBOUNCE documentation is that the driver
> > > > must
> > > > return 0 if debouncing is disabled.
> > > 
> > > ?
> 
> No, this is correct.
> "...if it is available but disabled it should return -EINVAL"

OK, thanks Andy for checking it up.