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

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

[Kris Bahnsen]

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 and use the scratch variable for the rx_buf for
both the 1 byte command to and 2 byte response from the controller.

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

V1 -> V2: Don't use rx_buf when clearing command reg
V2 -> V3: Modify original 2 xfer command to eliminate dev_err()
          output on xfer with len and NULL buffers

 drivers/input/touchscreen/ads7846.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 4b39f7212d35c..488bcc8393293 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -403,8 +403,7 @@ 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].rx_buf = &req->scratch;
 	req->xfer[6].len = 1;
 	spi_message_add_tail(&req->xfer[6], &req->msg);
 

base-commit: dd6c438c3e64a5ff0b5d7e78f7f9be547803ef1b
-- 
2.34.1

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

[Dmitry Torokhov]

Hi Kris,

On Thu, Apr 30, 2026 at 05:37:38PM +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 and use the scratch variable for the rx_buf for
> both the 1 byte command to and 2 byte response from the controller.
> 
> 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>
> ---
> 
> V1 -> V2: Don't use rx_buf when clearing command reg
> V2 -> V3: Modify original 2 xfer command to eliminate dev_err()
>           output on xfer with len and NULL buffers
> 
>  drivers/input/touchscreen/ads7846.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 4b39f7212d35c..488bcc8393293 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -403,8 +403,7 @@ 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].rx_buf = &req->scratch;

Sashiko (I believe correctly) pointed out that by doing this "scratch"
is now write only and this may cause DMA from the device stomp on
message status and other unrelated data that shares the same cacheline
with scracth. While it was already a problem before now it is even more
likely.

Since scratch is now write-only I believe moving it below "sample"
forces it into separate cacheline and fixes this problem. Could you
please try making this change?

Thanks.

-- 
Dmitry

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

[Kris Bahnsen]

Dmitry,

On 5/4/26 8:01 PM, Dmitry Torokhov wrote:
> Hi Kris,
> 
> On Thu, Apr 30, 2026 at 05:37:38PM +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 and use the scratch variable for the rx_buf for
>> both the 1 byte command to and 2 byte response from the controller.
>>
>> 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>
>> ---
>>
>> V1 -> V2: Don't use rx_buf when clearing command reg
>> V2 -> V3: Modify original 2 xfer command to eliminate dev_err()
>>           output on xfer with len and NULL buffers
>>
>>  drivers/input/touchscreen/ads7846.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
>> index 4b39f7212d35c..488bcc8393293 100644
>> --- a/drivers/input/touchscreen/ads7846.c
>> +++ b/drivers/input/touchscreen/ads7846.c
>> @@ -403,8 +403,7 @@ 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].rx_buf = &req->scratch;
> 
> Sashiko (I believe correctly) pointed out that by doing this "scratch"
> is now write only and this may cause DMA from the device stomp on
> message status and other unrelated data that shares the same cacheline
> with scracth. While it was already a problem before now it is even more
> likely.
> 
> Since scratch is now write-only I believe moving it below "sample"
> forces it into separate cacheline and fixes this problem. Could you
> please try making this change?

Apologies, I'm not quite certain I understand what you mean by
"moving it below sample." Do you mean relocating the xfer[6] block
immediately below the xfer[3] block like so? If yes, I can get this
tested and a v4 patch together. If not, can you please clarify?


diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 4b39f7212d35..6d57865ff505 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -390,6 +390,11 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
        req->xfer[3].len = 2;
        spi_message_add_tail(&req->xfer[3], &req->msg);
 
+       /* clear the command register */
+       req->xfer[6].rx_buf = &req->scratch;
+       req->xfer[6].len = 1;
+       spi_message_add_tail(&req->xfer[6], &req->msg);
+
        /* REVISIT:  take a few more samples, and compare ... */
 
        /* converter in low power mode & enable PENIRQ */
@@ -402,12 +407,6 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
        req->xfer[5].len = 2;
        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;
-       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]);

> 
> Thanks.
> 

-- 
Kris Bahnsen
Software Engineer
embeddedTS

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

[Dmitry Torokhov]

On Tue, May 05, 2026 at 09:21:50AM -0700, Kris Bahnsen wrote:
> Dmitry,
> 
> On 5/4/26 8:01 PM, Dmitry Torokhov wrote:
> > Hi Kris,
> > 
> > On Thu, Apr 30, 2026 at 05:37:38PM +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 and use the scratch variable for the rx_buf for
> >> both the 1 byte command to and 2 byte response from the controller.
> >>
> >> 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>
> >> ---
> >>
> >> V1 -> V2: Don't use rx_buf when clearing command reg
> >> V2 -> V3: Modify original 2 xfer command to eliminate dev_err()
> >>           output on xfer with len and NULL buffers
> >>
> >>  drivers/input/touchscreen/ads7846.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> >> index 4b39f7212d35c..488bcc8393293 100644
> >> --- a/drivers/input/touchscreen/ads7846.c
> >> +++ b/drivers/input/touchscreen/ads7846.c
> >> @@ -403,8 +403,7 @@ 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].rx_buf = &req->scratch;
> > 
> > Sashiko (I believe correctly) pointed out that by doing this "scratch"
> > is now write only and this may cause DMA from the device stomp on
> > message status and other unrelated data that shares the same cacheline
> > with scracth. While it was already a problem before now it is even more
> > likely.
> > 
> > Since scratch is now write-only I believe moving it below "sample"
> > forces it into separate cacheline and fixes this problem. Could you
> > please try making this change?
> 
> Apologies, I'm not quite certain I understand what you mean by
> "moving it below sample." Do you mean relocating the xfer[6] block
> immediately below the xfer[3] block like so? If yes, I can get this
> tested and a v4 patch together. If not, can you please clarify?

I meant doing this:

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 093f4b56cc18..04ba98b62f70 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -328,7 +328,6 @@ struct ser_req {
 	u8			ref_on;
 	u8			command;
 	u8			ref_off;
-	u16			scratch;
 	struct spi_message	msg;
 	struct spi_transfer	xfer[8];
 	/*
@@ -336,6 +335,7 @@ struct ser_req {
 	 * transfer buffers to live in their own cache lines.
 	 */
 	__be16 sample ____cacheline_aligned;
+	u16 scratch;
 };
 
 struct ads7845_ser_req {

Thanks.

-- 
Dmitry