1. Patchew
  2. linux
  3. View series

[PATCH] Input: ads7846 - don't use scratch for tx_buf when clearing register

Kris Bahnsen posted 1 patch 1 month, 3 weeks ago
There is a newer version of this series
drivers/input/touchscreen/ads7846.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
[PATCH] Input: ads7846 - don't use scratch for tx_buf when clearing register
Posted by Kris Bahnsen 1 month, 3 weeks ago 
The workaround for XPT2046 clears the command register, giving the
touchscreen controller a NOP. The change incorrectly re-uses the
req->scratch variable which is used as rx_buf for xfer[5], so by
the time xfer[6] occurs, the contents of req->scratch may not be
0. It was found that the touchscreen controller can end up in
a completely unresponsive state due to it being given a command
the driver does not expect.

Instead, rely on the spi_transfer behavior of tx_buf being NULL to
transmit all 0 bits, moving the 3 bytes to a single message.

This change was tested on real TSC2046 and ADS7843 controllers,
but not the XPT2046 the workaround was originally created for.
Confirming that the original modification to clear the command
register does not impact either real controller.

Fixes: 781a07da9bb94 ("Input: ads7846 - add dummy command register clearing cycle")
Cc: stable@vger.kernel.org
Co-developed-by: Mark Featherston <mark@embeddedTS.com>
Signed-off-by: Mark Featherston <mark@embeddedTS.com>
Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
---
 drivers/input/touchscreen/ads7846.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 4b39f7212d35c..599793d27129e 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -327,7 +327,7 @@ struct ser_req {
 	u8			ref_off;
 	u16			scratch;
 	struct spi_message	msg;
-	struct spi_transfer	xfer[8];
+	struct spi_transfer	xfer[7];
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
@@ -403,16 +403,11 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
 	spi_message_add_tail(&req->xfer[5], &req->msg);
 
 	/* clear the command register */
-	req->scratch = 0;
-	req->xfer[6].tx_buf = &req->scratch;
-	req->xfer[6].len = 1;
+	req->xfer[6].rx_buf = &req->scratch;
+	req->xfer[6].len = 3;
+	CS_CHANGE(req->xfer[6]);
 	spi_message_add_tail(&req->xfer[6], &req->msg);
 
-	req->xfer[7].rx_buf = &req->scratch;
-	req->xfer[7].len = 2;
-	CS_CHANGE(req->xfer[7]);
-	spi_message_add_tail(&req->xfer[7], &req->msg);
-
 	scoped_guard(mutex, &ts->lock) {
 		ads7846_stop(ts);
 		status = spi_sync(spi, &req->msg);

base-commit: dd6c438c3e64a5ff0b5d7e78f7f9be547803ef1b
-- 
2.34.1
Re: [PATCH] Input: ads7846 - don't use scratch for tx_buf when clearing register
Posted by Dmitry Torokhov 1 month, 3 weeks ago 
Hi Kris,

On Fri, Apr 24, 2026 at 07:25:34PM +0000, Kris Bahnsen wrote:
> The workaround for XPT2046 clears the command register, giving the
> touchscreen controller a NOP. The change incorrectly re-uses the
> req->scratch variable which is used as rx_buf for xfer[5], so by
> the time xfer[6] occurs, the contents of req->scratch may not be
> 0. It was found that the touchscreen controller can end up in
> a completely unresponsive state due to it being given a command
> the driver does not expect.
> 
> Instead, rely on the spi_transfer behavior of tx_buf being NULL to
> transmit all 0 bits, moving the 3 bytes to a single message.
> 
> This change was tested on real TSC2046 and ADS7843 controllers,
> but not the XPT2046 the workaround was originally created for.
> Confirming that the original modification to clear the command
> register does not impact either real controller.
> 
> Fixes: 781a07da9bb94 ("Input: ads7846 - add dummy command register clearing cycle")
> Cc: stable@vger.kernel.org
> Co-developed-by: Mark Featherston <mark@embeddedTS.com>
> Signed-off-by: Mark Featherston <mark@embeddedTS.com>
> Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
> ---
>  drivers/input/touchscreen/ads7846.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 4b39f7212d35c..599793d27129e 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -327,7 +327,7 @@ struct ser_req {
>  	u8			ref_off;
>  	u16			scratch;
>  	struct spi_message	msg;
> -	struct spi_transfer	xfer[8];
> +	struct spi_transfer	xfer[7];
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -403,16 +403,11 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
>  	spi_message_add_tail(&req->xfer[5], &req->msg);
>  
>  	/* clear the command register */
> -	req->scratch = 0;
> -	req->xfer[6].tx_buf = &req->scratch;
> -	req->xfer[6].len = 1;
> +	req->xfer[6].rx_buf = &req->scratch;
> +	req->xfer[6].len = 3;

Doesn't this overflow "scratch" which is only 2 bytes? I guess there is
a hole in ser_req between "scratch" and "msg" but I do not think we
should rely on this.

Can we also set rx_buf to NULL to discard incoming data?

[credit to sashiko].

Thanks.

-- 
Dmitry
Re: [PATCH] Input: ads7846 - don't use scratch for tx_buf when clearing register
Posted by Kris Bahnsen 1 month, 3 weeks ago 

On 4/25/26 9:51 PM, Dmitry Torokhov wrote:
> Hi Kris,
> 
> On Fri, Apr 24, 2026 at 07:25:34PM +0000, Kris Bahnsen wrote:
>> The workaround for XPT2046 clears the command register, giving the
>> touchscreen controller a NOP. The change incorrectly re-uses the
>> req->scratch variable which is used as rx_buf for xfer[5], so by
>> the time xfer[6] occurs, the contents of req->scratch may not be
>> 0. It was found that the touchscreen controller can end up in
>> a completely unresponsive state due to it being given a command
>> the driver does not expect.
>>
>> Instead, rely on the spi_transfer behavior of tx_buf being NULL to
>> transmit all 0 bits, moving the 3 bytes to a single message.
>>
>> This change was tested on real TSC2046 and ADS7843 controllers,
>> but not the XPT2046 the workaround was originally created for.
>> Confirming that the original modification to clear the command
>> register does not impact either real controller.
>>
>> Fixes: 781a07da9bb94 ("Input: ads7846 - add dummy command register clearing cycle")
>> Cc: stable@vger.kernel.org
>> Co-developed-by: Mark Featherston <mark@embeddedTS.com>
>> Signed-off-by: Mark Featherston <mark@embeddedTS.com>
>> Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
>> ---
>>  drivers/input/touchscreen/ads7846.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
>> index 4b39f7212d35c..599793d27129e 100644
>> --- a/drivers/input/touchscreen/ads7846.c
>> +++ b/drivers/input/touchscreen/ads7846.c
>> @@ -327,7 +327,7 @@ struct ser_req {
>>  	u8			ref_off;
>>  	u16			scratch;
>>  	struct spi_message	msg;
>> -	struct spi_transfer	xfer[8];
>> +	struct spi_transfer	xfer[7];
>>  	/*
>>  	 * DMA (thus cache coherency maintenance) requires the
>>  	 * transfer buffers to live in their own cache lines.
>> @@ -403,16 +403,11 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
>>  	spi_message_add_tail(&req->xfer[5], &req->msg);
>>  
>>  	/* clear the command register */
>> -	req->scratch = 0;
>> -	req->xfer[6].tx_buf = &req->scratch;
>> -	req->xfer[6].len = 1;
>> +	req->xfer[6].rx_buf = &req->scratch;
>> +	req->xfer[6].len = 3;
> 
> Doesn't this overflow "scratch" which is only 2 bytes? I guess there is
> a hole in ser_req between "scratch" and "msg" but I do not think we
> should rely on this.
> 
> Can we also set rx_buf to NULL to discard incoming data?

Well spotted! I'm quite annoyed with myself that I fixed one pointer
use bug to introduce a buffer overflow.

Will send a v2 patch later today.
 
> [credit to sashiko].
> 
> Thanks.
> 

-- 
Kris Bahnsen
Software Engineer
embeddedTS
Re: [PATCH] Input: ads7846 - don't use scratch for tx_buf when clearing register
Posted by Marek Vasut 1 month, 3 weeks ago 
On 4/24/26 9:25 PM, Kris Bahnsen wrote:
> The workaround for XPT2046 clears the command register, giving the
> touchscreen controller a NOP. The change incorrectly re-uses the
> req->scratch variable which is used as rx_buf for xfer[5], so by
> the time xfer[6] occurs, the contents of req->scratch may not be
> 0. It was found that the touchscreen controller can end up in
> a completely unresponsive state due to it being given a command
> the driver does not expect.
> 
> Instead, rely on the spi_transfer behavior of tx_buf being NULL to
> transmit all 0 bits, moving the 3 bytes to a single message.
> 
> This change was tested on real TSC2046 and ADS7843 controllers,
> but not the XPT2046 the workaround was originally created for.
> Confirming that the original modification to clear the command
> register does not impact either real controller.
> 
> Fixes: 781a07da9bb94 ("Input: ads7846 - add dummy command register clearing cycle")
> Cc: stable@vger.kernel.org
> Co-developed-by: Mark Featherston <mark@embeddedTS.com>
> Signed-off-by: Mark Featherston <mark@embeddedTS.com>
> Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
Tested-by: Marek Vasut <marex@nabladev.com> # XPT2046

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.