linux-kernel - Re: [PATCH v1 1/2] spi: npcm-pspi: add full duplex support

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: <YtlYt/5VKIblUHBP@sirena.org.uk>
Date:   Thu, 21 Jul 2022 14:46:31 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Tomer Maimon <tmaimon77@...il.com>
Cc:     avifishman70@...il.com, tali.perry1@...il.com, joel@....id.au,
        venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        openbmc@...ts.ozlabs.org, linux-spi@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v1 1/2] spi: npcm-pspi: add full duplex support

On Thu, Jul 21, 2022 at 01:15:55PM +0300, Tomer Maimon wrote:

> The NPCM PSPI handler, on TX-buffer not null, would perform a dummy read
> but did not save the rx-data, this was valid only for half duplex.

> This patch adds full duplex support for NPCM PSPI driver by storing all
> rx-data when the Rx-buffer is defined also for TX-buffer handling.

This doesn't seem to entirely correspond to what the patch does, nor to
what the driver currently does?  I can't see any dummy read code in the
current driver.

>  static void npcm_pspi_send(struct npcm_pspi *priv)
>  {
>  	int wsize;
> -	u16 val;
> +	u16 val = 0;
>  
>  	wsize = min(bytes_per_word(priv->bits_per_word), priv->tx_bytes);
>  	priv->tx_bytes -= wsize;
>  
> -	if (!priv->tx_buf)
> -		return;
> -
>  	switch (wsize) {
>  	case 1:
> -		val = *priv->tx_buf++;
> +		if (priv->tx_buf)
> +			val = *priv->tx_buf++;
>  		iowrite8(val, NPCM_PSPI_DATA + priv->base);
>  		break;

These changes appaear to be trying to ensure that when _send() is called
we now always write something out, even if there was no transmit buffer.
Since the device has been supporting half duplex transfers it is not
clear why we'd want to do that, it's adding overhead to the PIO which
isn't great.  This also isn't what the changelog said, the changelog
said we were adding reading of data when there's a transmit buffer.
Similar issues apply on the read side.

AFAICT the bulk of what the change is doing is trying make the driver
unconditionally do both read and writes to the hardware when it would
previously have only read or written data if there was a buffer
provided.  That's basically open coding SPI_CONTROLLER_MUST_TX and
SPI_CONTROLLER_MUST_RX, if that's what the hardware needs then you
should just set those flags and let the core fix things up.

> +       /*
> +        * first we do the read since if we do the write we previous read might
> +        * be lost (indeed low chances)
> +        */

This reordering sounds like it might be needed but should have been
mentioned in the changelog and is a separate patch.

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