linux-kernel - Re: [PATCH 1/2] spi: ar934x: fix transfer size

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, 22 Dec 2021 17:55:04 +0200
From:   Oskari Lemmelä <oskari@...mela.net>
To:     Mark Brown <broonie@...nel.org>
Cc:     linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] spi: ar934x: fix transfer size

On 22.12.2021 16.59, Mark Brown wrote:
> On Wed, Dec 22, 2021 at 04:27:36PM +0200, Oskari Lemmelä wrote:
>> On 22.12.2021 14.32, Mark Brown wrote:
>>> Does this actually materially affect what the hardware does?  How much
>>> data is transferred in an internal loop in the driver is completely
>>> immaterial, bits per word only matters for formatting of the transferred
>>> data.
>> I don't have logic analyzator to verify what hardware actual does.
>> I tested this with transferring 32bits to ATSAMD20J15 slave.
>> Running loop in 8bits or 16bits, transfer is done correctly without
>> any errors. When running loop in 24bits or 32bits directly I got
>> error from spi_sync_transfer.
> This doesn't inspire confidence TBH.  Given the lack of any change in
> the interaction with the hardware it doesn't seem likely that the word
> length is being changed at any point.  Possibly there's a bug somewhere
> that needs fixing but it's been misdiagnosed.
I did find datasheet for AR9344 and hardware supports shifting bits.

8.25.6 SPI Content to Shift Out or In (SPI_SHIFT_CNT_ADDR)
Address: 0x1FFF0014
Access: Read/Write
Reset: 0x0
-------------------------------------------
Bit  | Bit Name     | Desc
31   | SHIFT_EN     | Enables shifting data out
30   | SHIFT_CHNL   | If set to 1, enables chip select 2
29   |              | If set to 1, enables chip select 1
28   |              | If set to 1, enables chip select 0
27   | SHIFT_CLKOUT | Initial value of the clock signal
26   | TERMINATE    | When set to 1, deassert the chip select
25:7 | RES          | Reserved
6:0  | SHIFT_COUNT  | The number of bits to be shifted out or shifted in
on the data line

This is currently implemented in defines
#define AR934X_SPI_REG_SHIFT_CTRL       0x14
#define AR934X_SPI_SHIFT_EN             BIT(31)
#define AR934X_SPI_SHIFT_CS(n)          BIT(28 + (n))
#define AR934X_SPI_SHIFT_TERM           26
#define AR934X_SPI_SHIFT_VAL(cs, term, count)                   \
        (AR934X_SPI_SHIFT_EN | AR934X_SPI_SHIFT_CS(cs) |        \
        (term) << AR934X_SPI_SHIFT_TERM | (count))

In the transfer loop count value is set to number of bits per word.

reg = AR934X_SPI_SHIFT_VAL(spi->chip_select, term, trx_cur * 8);
iowrite32(reg, sp->base + AR934X_SPI_REG_SHIFT_CTRL);

So actually hardware support any word size between 1-32bits.
> Note also that the commit log is not good here, now I look at the code
> the driver only supports 8 bits per word at the minute and the change
> adds support for higher word lengths.  If you are seeing an issue that
> might point towards what it is.
Should I split this in two commits? First one fixing SPI_BPW_MASK(32) typo.
Then second commit which implements 8bits, 16bits and 24bits word sizes?
Or should driver implement support for any word size between 1-32bits?

Oskari