linux-kernel - Re: [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft specification).

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]

Message-ID: <75e473b2-f4d2-476c-8d1c-1085446d8b41@opensynergy.com>
Date:   Wed, 6 Dec 2023 18:23:08 +0100
From:   Harald Mommer <harald.mommer@...nsynergy.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     virtio-dev@...ts.oasis-open.org,
        Haixu Cui <quic_haixcui@...cinc.com>,
        linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Harald.Mommer@...il.com, quic_ztu@...cinc.com,
        Matti Moell <Matti.Moell@...nsynergy.com>,
        Mikhail Golubev <Mikhail.Golubev@...nsynergy.com>
Subject: Re: [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4 draft
 specification).

On 06.11.23 14:48, Mark Brown wrote:
> On Fri, Oct 27, 2023 at 06:10:16PM +0200, Harald Mommer wrote:
>
>> +config SPI_VIRTIO
>> +	tristate "Virtio SPI SPI Controller"
>> +	depends on VIRTIO
>> +	help
>> +	  This enables the Virtio SPI driver.
>> +
>> +	  Virtio SPI is an SPI driver for virtual machines using Virtio.
>> +
>> +	  If your Linux is a virtual machine using Virtio, say Y here.
>> +
> This advice is going to be inappropriate for the majortiy of guests.
Reminder for me: Need still to address this, but this is not code I'm 
currently working on, so comes later.
>> +	// clang-format off
>> +	struct spi_transfer_head transfer_head	____cacheline_aligned;
>> +	const uint8_t *tx_buf			____cacheline_aligned;
>> +	uint8_t *rx_buf				____cacheline_aligned;
>> +	struct spi_transfer_result result	____cacheline_aligned;
>> +	// clang-format on
> Remove this clang-format stuff.
Not needed any more, will be removed. Maybe I should remove this 
____cacheline_aligned. It's only there because I looked too deeply into 
struct virtio_i2c_req, not because I think this ____cacheline_aligned is 
decisive here.
>> +static struct spi_board_info board_info = {
>> +	.modalias = "spi-virtio",
>> +	.max_speed_hz = 125000000, /* Arbitrary very high limit */
>> +	.bus_num = 0, /* Patched later during initialization */
>> +	.chip_select = 0, /* Patched later during initialization */
>> +	.mode = SPI_MODE_0,
>> +};
>> +/* Compare with file i2c_virtio.c structure virtio_i2c_msg_done */
> In what way is one supposed to compare with the i2c driver?  What
> happens if the I2C driver changes?  It would be better to ensure that
> the code can be read and understood as a standalone thing.
It was a reminder for me from where I got some inspiration and to reveal 
that not everything was invented by me. Served it's purpose, all such 
comment references to foreign code is being removed.
>> +	/* Fill struct spi_transfer_head */
>> +	th->slave_id = spi->chip_select;
> If the spec just copied the Linux terminology it'd have few issues :(
Spec changed this in the meantime so I can do also.
>> +	th->bits_per_word = spi->bits_per_word;
>> +	th->cs_change = xfer->cs_change;
> The virtio spec for cs_change is *not* what the Linux cs_change field
> does, this will not do the right thing.

This is a hard one. Linux says (spi.h):

"@cs_change: affects chipselect after this transfer completes

...

All SPI transfers start with the relevant chipselect active. Normally
it stays selected until after the last transfer in a message. Drivers
can affect the chipselect signal using cs_change."

V8 of the draft spec says:

"cs_change indicates whether to deselect device before starting the next 
SPI transfer, 0 means chipselect
keep asserted and 1 means chipselect deasserted then asserted again."

What I understand here (unfortunately in both texts) is

- at the start of a transfer CS is made active

- it is kept active during the transfer

- when cs_change is 0 after the transfer CS is kept active (not changed)

- when cs_change is 1 after the transfer CS is de-asserted

So if cs_change is 0 keep CS asserted after transfer, otherwise 
de-assert after transfer.

I fear there is some subtle thing I haven't gotten yet but I don't 
understand / see it.What is it?

>> +	th->tx_nbits = xfer->tx_nbits;
>> +	th->rx_nbits = xfer->rx_nbits;
>> +	th->reserved[0] = 0;
>> +	th->reserved[1] = 0;
>> +	th->reserved[2] = 0;
>> +
>> +#if (VIRTIO_SPI_CPHA != SPI_CPHA)
>> +#error VIRTIO_SPI_CPHA != SPI_CPHA
>> +#endif
> BUILD_BUG_ON()

Thanks for this one, didn't know yet.