linux-kernel - 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]

Date: Wed, 24 Apr 2024 19:54:12 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: Gregor Herburger <gregor.herburger@...tq-group.com>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, Thomas Kopp <thomas.kopp@...rochip.com>, 
	"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 Wed. 17 Apr. 2024 at 22:45, Gregor Herburger
<gregor.herburger@...tq-group.com> 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.
>
> Never write byte 2 of IOCON register to avoid clearing of LAT0/LAT1.
>
> 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)
                                                                            ^^^^
count is never used.

> +{
> +       const size_t data_offset = sizeof(__be16) +
> +               mcp251xfd_regmap_crc.pad_bits / BITS_PER_BYTE;
> +       u16 reg = *(u16 *)data;

This line made me scratch my head a lot.

When I see a void * parameter named data, I expect this to be a memory
region. Here, if I got this correctly, data is just a pointer to a u16
which represents the low bit of a register.

So, if you are not passing an address to a memory region but just a
single scalar, why the void *? Wouldn't it be better to just do:

  mcp251xfd_regmap_crc_write_iocon(void *context, u16 reg)

> +       /* 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.
                                                                  ^^^^^^^^
clearing

> +        */
> +
> +       /* Write IOCON[15:0] */
> +       mcp251xfd_regmap_crc_gather_write(context, &reg, 1,
> +                                         data + data_offset, 2);
> +       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)

This also uses the const void* data, except that here, this is kind of
forced by the prototype of the write() callback function from struct
regmap_bus. Also, count is properly used.

>  {
>         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);

After changing the prototype of mcp251xfd_regmap_crc_write_iocon(),
this would then become:

                return mcp251xfd_regmap_crc_write_iocon(context, reg);

> +       else
> +               return mcp251xfd_regmap_crc_gather_write(context,
>                                                  data, data_offset,
>                                                  data + data_offset,
>                                                  count - data_offset);