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: <ZUjuxfKdwRQgbfdb@finisterre.sirena.org.uk>
Date:   Mon, 6 Nov 2023 13:48:53 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Harald Mommer <Harald.Mommer@...nsynergy.com>
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 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.

> +	// 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.

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

> +	/* Fill struct spi_transfer_head */
> +	th->slave_id = spi->chip_select;

If the spec just copied the Linux terminology it'd have few issues :(

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

> +	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()


Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)