netdev - Re: [PATCH 2/4] can: mcp251xfd: mcp251xfd_regmap_crc

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: <20240424-worm-of-massive-triumph-2eaf27-mkl@pengutronix.de>
Date: Wed, 24 Apr 2024 13:51:37 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Gregor Herburger <gregor.herburger@...tq-group.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	Thomas Kopp <thomas.kopp@...rochip.com>, Vincent Mailhol <mailhol.vincent@...adoo.fr>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, linux-can@...r.kernel.org, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux@...tq-group.com, alexander.stein@...tq-group.com
Subject: Re: [PATCH 2/4] can: mcp251xfd: mcp251xfd_regmap_crc_write():
 workaround for errata 5

On 17.04.2024 15:43:55, Gregor Herburger wrote:
> According to Errata DS80000789E 5 writing IOCON register using one SPI
> write command clears LAT0/LAT1.
> 
> Errata Fix/Work Around suggests to write registers with single byte write
> instructions. However, it seems that every write to the second byte
> causes the overrite of LAT0/LAT1.

This change doesn't use single byte write instructions.

> Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.

I discovered that erratum, it's described in
mcp251xfd_chip_rx_int_enable():

	/* Configure GPIOs:
	 * - PIN0: GPIO Input
	 * - PIN1: GPIO Input/RX Interrupt
	 *
	 * PIN1 must be Input, otherwise there is a glitch on the
	 * rx-INT line. It happens between setting the PIN as output
	 * (in the first byte of the SPI transfer) and configuring the
	 * PIN as interrupt (in the last byte of the SPI transfer).
	 */

The problem is that the SPI writes 1 byte at a time, starting at the
lower address. The chip updates the GPIO pin's status after each written
byte.

This may leads to a glitch if you have an external pull up. The power on
default auf the chip is GPIO/input, the GPIO line is not driven by the
chip and with the external pull up this will result in a high level.

If you configure the GPIO as an output/high, the driver first writes
bits 0...7, which results in the GPIO line being configured as an
output; the subsequent bits 8...15 configure the level of the GPIO
line.

This change doesn't take care of this.

I'm not sure, if it's better to have 2 dedicated writes to IOCON in the
driver or try to hide it here in the regmap.

> Signed-off-by: Gregor Herburger <gregor.herburger@...tq-group.com>
> ---
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c | 35 +++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> index 92b7bc7f14b9..ab4e372baffb 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> @@ -229,14 +229,47 @@ mcp251xfd_regmap_crc_gather_write(void *context,
>  	return spi_sync_transfer(spi, xfer, ARRAY_SIZE(xfer));
>  }
>  
> +static int
> +mcp251xfd_regmap_crc_write_iocon(void *context, const void *data, size_t count)
> +{
> +	const size_t data_offset = sizeof(__be16) +
> +		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +	u16 reg = *(u16 *)data;
> +
> +	/* Never write to bits 16..23 of IOCON register to avoid clearing of LAT0/LAT1
> +	 *
> +	 * According to Errata DS80000789E 5 writing IOCON register using one
> +	 * SPI write command clears LAT0/LAT1.
> +	 *
> +	 * Errata Fix/Work Around suggests to write registers with single byte
> +	 * write instructions. However, it seems that the byte at 0xe06(IOCON[23:16])
> +	 * is for read-only access and writing to it causes the cleraing of LAT0/LAT1.
> +	 */
> +
> +	/* Write IOCON[15:0] */
> +	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +					  data + data_offset, 2);

You write 15:0 in 1 go here.

> +	reg += 3;
> +	/* Write IOCON[31:24] */
> +	mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +					  data + data_offset + 3, 1);
> +
> +	return 0;
> +}
> +
>  static int
>  mcp251xfd_regmap_crc_write(void *context,
>  			   const void *data, size_t count)
>  {
>  	const size_t data_offset = sizeof(__be16) +
>  		mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +	u16 reg = *(u16 *)data;
>  
> -	return mcp251xfd_regmap_crc_gather_write(context,
> +	if (reg == MCP251XFD_REG_IOCON)
> +		return mcp251xfd_regmap_crc_write_iocon(context,
> +						 data, count);
> +	else
> +		return mcp251xfd_regmap_crc_gather_write(context,
>  						 data, data_offset,
>  						 data + data_offset,
>  						 count - data_offset);

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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