linux-kernel - Re: [PATCH] spi: bitbang: Fix lsb-first Rx

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Aug 2022 16:22:13 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Mark Brown <broonie@...nel.org>,
        Andreas Färber <afaerber@...e.de>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Christian Hewitt <christianshewitt@...il.com>
Cc:     linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spi: bitbang: Fix lsb-first Rx

On 2022-08-03 16:02, Mark Brown wrote:
> On Wed, Aug 03, 2022 at 03:58:57PM +0100, Robin Murphy wrote:
>> Shifting the recieved bit by "bits" inserts it at the top of the
>> *currently remaining* Tx data, so we end up accumulating the whole
>> transfer into bit 0 of the output word. Oops.
>>
>> For the algorithm to work as intended, we need to remember where the
>> top of the *original* word was, and shift Rx to there.
> 
> So if this never worked we presumably have some systems out there which
> somehow rely on the old behaviour that we need to fix somehow - copying
> in everyone from the original change...

Hmm, I can't imagine anyone's relying too critically on data transfer 
corruption :/

Apparently these TM16x8 chips are the only thing so far to even want 
lsb-first bitbang support at all, so I strongly suspect I'm the first 
person to try the receive side of it (since Rockchip's SPI hardware 
doesn't seem to support 3-wire half-duplex). It actually took me quite a 
while to figure out that this wasn't my bug in trying to hook up the 
matrix-keymap stuff, since half the bytes I was expecting to get back do 
only have bit 0 set anyway.

Cheers,
Robin.

>> Fixes: 1847e3046c52 ("spi: gpio: Implement LSB First bitbang support")
>> Signed-off-by: Robin Murphy <robin.murphy@....com>
>> ---
>>   drivers/spi/spi-bitbang-txrx.h | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
>> index 267342dfa738..2dcbe166df63 100644
>> --- a/drivers/spi/spi-bitbang-txrx.h
>> +++ b/drivers/spi/spi-bitbang-txrx.h
>> @@ -116,6 +116,7 @@ bitbang_txrx_le_cpha0(struct spi_device *spi,
>>   {
>>   	/* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
>>   
>> +	u8 rxbit = bits - 1;
>>   	u32 oldbit = !(word & 1);
>>   	/* clock starts at inactive polarity */
>>   	for (; likely(bits); bits--) {
>> @@ -135,7 +136,7 @@ bitbang_txrx_le_cpha0(struct spi_device *spi,
>>   		/* sample LSB (from slave) on leading edge */
>>   		word >>= 1;
>>   		if ((flags & SPI_MASTER_NO_RX) == 0)
>> -			word |= getmiso(spi) << (bits - 1);
>> +			word |= getmiso(spi) << rxbit;
>>   		setsck(spi, cpol);
>>   	}
>>   	return word;
>> @@ -148,6 +149,7 @@ bitbang_txrx_le_cpha1(struct spi_device *spi,
>>   {
>>   	/* if (cpol == 0) this is SPI_MODE_1; else this is SPI_MODE_3 */
>>   
>> +	u8 rxbit = bits - 1;
>>   	u32 oldbit = !(word & 1);
>>   	/* clock starts at inactive polarity */
>>   	for (; likely(bits); bits--) {
>> @@ -168,7 +170,7 @@ bitbang_txrx_le_cpha1(struct spi_device *spi,
>>   		/* sample LSB (from slave) on trailing edge */
>>   		word >>= 1;
>>   		if ((flags & SPI_MASTER_NO_RX) == 0)
>> -			word |= getmiso(spi) << (bits - 1);
>> +			word |= getmiso(spi) << rxbit;
>>   	}
>>   	return word;
>>   }
>> -- 
>> 2.36.1.dirty
>>