| Message ID | 20210107094900.173046-16-mkl@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,01/19] can: tcan4x5x: replace DEVICE_NAME by KBUILD_MODNAME | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Pull request |
| netdev/fixes_present | success | Link |
| netdev/patch_count | warning | Series longer than 15 patches |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: wg@grandegger.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 174 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, 7 Jan 2021 10:48:56 +0100 Marc Kleine-Budde wrote: > +struct __packed tcan4x5x_map_buf { > + struct tcan4x5x_buf_cmd cmd; > + u8 data[256 * sizeof(u32)]; > +} ____cacheline_aligned; Interesting attribute combo, I must say.
On Thu, 7 Jan 2021 11:00:35 -0800 Jakub Kicinski wrote: > On Thu, 7 Jan 2021 10:48:56 +0100 Marc Kleine-Budde wrote: > > +struct __packed tcan4x5x_map_buf { > > + struct tcan4x5x_buf_cmd cmd; > > + u8 data[256 * sizeof(u32)]; > > +} ____cacheline_aligned; > > Interesting attribute combo, I must say. Looking at the rest of the patch I don't really see a reason for __packed. Perhaps it can be dropped?
On 1/7/21 8:06 PM, Jakub Kicinski wrote: > On Thu, 7 Jan 2021 11:00:35 -0800 Jakub Kicinski wrote: >> On Thu, 7 Jan 2021 10:48:56 +0100 Marc Kleine-Budde wrote: >>> +struct __packed tcan4x5x_map_buf { >>> + struct tcan4x5x_buf_cmd cmd; >>> + u8 data[256 * sizeof(u32)]; >>> +} ____cacheline_aligned; >> >> Interesting attribute combo, I must say. > > Looking at the rest of the patch I don't really see a reason for > __packed. Perhaps it can be dropped? It's the stream of bytes send via SPI to the chip. Here are both structs for reference: > +struct __packed tcan4x5x_buf_cmd { > + u8 cmd; > + __be16 addr; > + u8 len; > +}; This has to be packed, as I assume the compiler would add some space after the "u8 cmd" to align the __be16 naturally. > +struct __packed tcan4x5x_map_buf { > + struct tcan4x5x_buf_cmd cmd; > + u8 data[256 * sizeof(u32)]; > +} ____cacheline_aligned; Due to the packing of the struct tcan4x5x_buf_cmd it should have a length of 4 bytes. Without __packed, will the "u8 data" come directly after the cmd? Marc
On 1/7/21 8:00 PM, Jakub Kicinski wrote: > On Thu, 7 Jan 2021 10:48:56 +0100 Marc Kleine-Budde wrote: >> +struct __packed tcan4x5x_map_buf { >> + struct tcan4x5x_buf_cmd cmd; >> + u8 data[256 * sizeof(u32)]; >> +} ____cacheline_aligned; > > Interesting attribute combo, I must say. __packed as it's the byte stream send to the chip via SPI. ____cacheline_aligned, as it might be subject to DMA mapping in the SPI host driver. An alternative would be to allocate these with separate kmalloc(). regards, Marc
On Thu, 7 Jan 2021 22:17:15 +0100 Marc Kleine-Budde wrote: > > +struct __packed tcan4x5x_buf_cmd { > > + u8 cmd; > > + __be16 addr; > > + u8 len; > > +}; > > This has to be packed, as I assume the compiler would add some space after the > "u8 cmd" to align the __be16 naturally. Ack, packing this one makes sense. > > +struct __packed tcan4x5x_map_buf { > > + struct tcan4x5x_buf_cmd cmd; > > + u8 data[256 * sizeof(u32)]; > > +} ____cacheline_aligned; > > Due to the packing of the struct tcan4x5x_buf_cmd it should have a length of 4 > bytes. Without __packed, will the "u8 data" come directly after the cmd? Yup, u8 with no alignment attribute will follow the previous field with no holes.
On 07.01.21 23:38, Jakub Kicinski wrote: > On Thu, 7 Jan 2021 22:17:15 +0100 Marc Kleine-Budde wrote: >>> +struct __packed tcan4x5x_buf_cmd { >>> + u8 cmd; >>> + __be16 addr; >>> + u8 len; >>> +}; >> >> This has to be packed, as I assume the compiler would add some space after the >> "u8 cmd" to align the __be16 naturally. > > Ack, packing this one makes sense. > >>> +struct __packed tcan4x5x_map_buf { >>> + struct tcan4x5x_buf_cmd cmd; >>> + u8 data[256 * sizeof(u32)]; >>> +} ____cacheline_aligned; >> >> Due to the packing of the struct tcan4x5x_buf_cmd it should have a length of 4 >> bytes. Without __packed, will the "u8 data" come directly after the cmd? > > Yup, u8 with no alignment attribute will follow the previous > field with no holes. __packed has a documentation benefit though. It documents that the author considers the current layout to be the only correct one. (and thus extra care should be taken when modifying it). > >
On Fri, 8 Jan 2021 11:07:26 +0100 Ahmad Fatoum wrote: > >>> +struct __packed tcan4x5x_map_buf { > >>> + struct tcan4x5x_buf_cmd cmd; > >>> + u8 data[256 * sizeof(u32)]; > >>> +} ____cacheline_aligned; > >> > >> Due to the packing of the struct tcan4x5x_buf_cmd it should have a length of 4 > >> bytes. Without __packed, will the "u8 data" come directly after the cmd? > > > > Yup, u8 with no alignment attribute will follow the previous > > field with no holes. > > __packed has a documentation benefit though. It documents that the author > considers the current layout to be the only correct one. (and thus extra > care should be taken when modifying it). ____cacheline_aligned adds a big architecture dependent padding at the end of this struct, so the size of this structure is architecture dependent. Besides using packed forced the compiler to use byte by byte loads on architectures without unaligned access, so __packed is not free.
From: Marc Kleine-Budde > Sent: 07 January 2021 21:17 > > On 1/7/21 8:06 PM, Jakub Kicinski wrote: > > On Thu, 7 Jan 2021 11:00:35 -0800 Jakub Kicinski wrote: > >> On Thu, 7 Jan 2021 10:48:56 +0100 Marc Kleine-Budde wrote: > >>> +struct __packed tcan4x5x_map_buf { > >>> + struct tcan4x5x_buf_cmd cmd; > >>> + u8 data[256 * sizeof(u32)]; > >>> +} ____cacheline_aligned; > >> > >> Interesting attribute combo, I must say. > > > > Looking at the rest of the patch I don't really see a reason for > > __packed. Perhaps it can be dropped? > > It's the stream of bytes send via SPI to the chip. Here are both structs for > reference: > > > +struct __packed tcan4x5x_buf_cmd { > > + u8 cmd; > > + __be16 addr; > > + u8 len; > > +}; > > This has to be packed, as I assume the compiler would add some space after the > "u8 cmd" to align the __be16 naturally. Why not generate a series of 32bit words to be sent over the SPI bus. Slightly less faffing in the code. Then have a #define (or inline function) to merge the cmd+addr+len into a single 32bit word. Also if the length is in 32bit units, then the data[] field ought to be u32[]. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hello Jakub, On 08.01.21 17:32, Jakub Kicinski wrote: > On Fri, 8 Jan 2021 11:07:26 +0100 Ahmad Fatoum wrote: >>>>> +struct __packed tcan4x5x_map_buf { >>>>> + struct tcan4x5x_buf_cmd cmd; >>>>> + u8 data[256 * sizeof(u32)]; >>>>> +} ____cacheline_aligned; >>>> >>>> Due to the packing of the struct tcan4x5x_buf_cmd it should have a length of 4 >>>> bytes. Without __packed, will the "u8 data" come directly after the cmd? >>> >>> Yup, u8 with no alignment attribute will follow the previous >>> field with no holes. >> >> __packed has a documentation benefit though. It documents that the author >> considers the current layout to be the only correct one. (and thus extra >> care should be taken when modifying it). > > ____cacheline_aligned adds a big architecture dependent padding at the > end of this struct, so the size of this structure is architecture > dependent. Besides using packed forced the compiler to use byte by byte > loads on architectures without unaligned access, so __packed is not > free. https://godbolt.org/z/j68x8n seems to indicate that explicit alignment "overrules" packed's implicit alignment of 1 as there isn't any byte-by-byte access generated for a struct that is both packed and cacheline aligned. packed only structs are accessed byte-by-byte however. Did I get something wrong in my testcase? I compiled with ARM gcc 8.2 -mno-unaligned-access -fno-strict-aliasing -O2 Cheers, Ahmad
On Mon, 11 Jan 2021 16:35:30 +0100 Ahmad Fatoum wrote: > Hello Jakub, > > On 08.01.21 17:32, Jakub Kicinski wrote: > > On Fri, 8 Jan 2021 11:07:26 +0100 Ahmad Fatoum wrote: > >>>>> +struct __packed tcan4x5x_map_buf { > >>>>> + struct tcan4x5x_buf_cmd cmd; > >>>>> + u8 data[256 * sizeof(u32)]; > >>>>> +} ____cacheline_aligned; > >>>> > >>>> Due to the packing of the struct tcan4x5x_buf_cmd it should have a length of 4 > >>>> bytes. Without __packed, will the "u8 data" come directly after the cmd? > >>> > >>> Yup, u8 with no alignment attribute will follow the previous > >>> field with no holes. > >> > >> __packed has a documentation benefit though. It documents that the author > >> considers the current layout to be the only correct one. (and thus extra > >> care should be taken when modifying it). > > > > ____cacheline_aligned adds a big architecture dependent padding at the > > end of this struct, so the size of this structure is architecture > > dependent. Besides using packed forced the compiler to use byte by byte > > loads on architectures without unaligned access, so __packed is not > > free. > > https://godbolt.org/z/j68x8n > > seems to indicate that explicit alignment "overrules" packed's implicit > alignment of 1 as > there isn't any byte-by-byte access generated for a struct > that is both packed and cacheline aligned. packed only structs are accessed > byte-by-byte however. > > Did I get something wrong in my testcase? > > I compiled with ARM gcc 8.2 -mno-unaligned-access -fno-strict-aliasing -O2 I see, that's why I said combining ____cacheline_aligned with __packed looks very confusing. Good to know which one takes precedence. That doesn't change my recommendation to remove __packed, though, let's not leave readers of this code scratching their heads.
On 1/7/21 11:38 PM, Jakub Kicinski wrote: > On Thu, 7 Jan 2021 22:17:15 +0100 Marc Kleine-Budde wrote: >>> +struct __packed tcan4x5x_buf_cmd { >>> + u8 cmd; >>> + __be16 addr; >>> + u8 len; >>> +}; >> >> This has to be packed, as I assume the compiler would add some space after the >> "u8 cmd" to align the __be16 naturally. > > Ack, packing this one makes sense. > >>> +struct __packed tcan4x5x_map_buf { >>> + struct tcan4x5x_buf_cmd cmd; >>> + u8 data[256 * sizeof(u32)]; >>> +} ____cacheline_aligned; >> >> Due to the packing of the struct tcan4x5x_buf_cmd it should have a length of 4 >> bytes. Without __packed, will the "u8 data" come directly after the cmd? > > Yup, u8 with no alignment attribute will follow the previous > field with no holes. Ack, without __packed, there's no diff in the objdump on arm. Marc
On 1/8/21 10:53 PM, David Laight wrote: >> On 1/7/21 8:06 PM, Jakub Kicinski wrote: >>> On Thu, 7 Jan 2021 11:00:35 -0800 Jakub Kicinski wrote: >>>> On Thu, 7 Jan 2021 10:48:56 +0100 Marc Kleine-Budde wrote: >>>>> +struct __packed tcan4x5x_map_buf { >>>>> + struct tcan4x5x_buf_cmd cmd; >>>>> + u8 data[256 * sizeof(u32)]; >>>>> +} ____cacheline_aligned; >>>> >>>> Interesting attribute combo, I must say. >>> >>> Looking at the rest of the patch I don't really see a reason for >>> __packed. Perhaps it can be dropped? >> >> It's the stream of bytes send via SPI to the chip. Here are both structs for >> reference: >> >>> +struct __packed tcan4x5x_buf_cmd { >>> + u8 cmd; >>> + __be16 addr; >>> + u8 len; >>> +}; >> >> This has to be packed, as I assume the compiler would add some space after the >> "u8 cmd" to align the __be16 naturally. > > Why not generate a series of 32bit words to be sent over the SPI bus. > Slightly less faffing in the code. > Then have a #define (or inline function) to merge the cmd+addr+len > into a single 32bit word. The driver uses regmap. With proper configuration regmap already formats the "cmd" and the "addr" in correct byte order into the first 3 bytes of the "reg_buf". As regmap doesn't support generating a length parameter, I use the above struct tcan4x5x_buf_cmd to set the length. > Also if the length is in 32bit units, then the data[] field > ought to be u32[]. In the regmap callback the data is passed with a void pointer and a length (in bytes), so copying this to a "u8 data[]" felt more natural to me. regards, Marc
diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c index 739b8f89a335..d37843a74663 100644 --- a/drivers/net/can/m_can/tcan4x5x-core.c +++ b/drivers/net/can/m_can/tcan4x5x-core.c @@ -380,7 +380,7 @@ static int tcan4x5x_can_probe(struct spi_device *spi) spi_set_drvdata(spi, priv); /* Configure the SPI bus */ - spi->bits_per_word = 32; + spi->bits_per_word = 8; ret = spi_setup(spi); if (ret) goto out_m_can_class_free_dev; diff --git a/drivers/net/can/m_can/tcan4x5x-regmap.c b/drivers/net/can/m_can/tcan4x5x-regmap.c index 5ea162578619..660e9d87dffb 100644 --- a/drivers/net/can/m_can/tcan4x5x-regmap.c +++ b/drivers/net/can/m_can/tcan4x5x-regmap.c @@ -9,48 +9,76 @@ #include "tcan4x5x.h" -#define TCAN4X5X_WRITE_CMD (0x61 << 24) -#define TCAN4X5X_READ_CMD (0x41 << 24) +#define TCAN4X5X_SPI_INSTRUCTION_WRITE (0x61 << 24) +#define TCAN4X5X_SPI_INSTRUCTION_READ (0x41 << 24) #define TCAN4X5X_MAX_REGISTER 0x8ffc -static int tcan4x5x_regmap_gather_write(void *context, const void *reg, - size_t reg_len, const void *val, - size_t val_len) +static int tcan4x5x_regmap_gather_write(void *context, + const void *reg, size_t reg_len, + const void *val, size_t val_len) { struct spi_device *spi = context; - struct spi_message m; - u32 addr; - struct spi_transfer t[2] = { - { .tx_buf = &addr, .len = reg_len, .cs_change = 0,}, - { .tx_buf = val, .len = val_len, }, + struct tcan4x5x_priv *priv = spi_get_drvdata(spi); + struct tcan4x5x_map_buf *buf_tx = &priv->map_buf_tx; + struct spi_transfer xfer[] = { + { + .tx_buf = buf_tx, + .len = sizeof(buf_tx->cmd) + val_len, + }, }; - addr = TCAN4X5X_WRITE_CMD | (*((u16 *)reg) << 8) | val_len >> 2; + memcpy(&buf_tx->cmd, reg, sizeof(buf_tx->cmd.cmd) + + sizeof(buf_tx->cmd.addr)); + tcan4x5x_spi_cmd_set_len(&buf_tx->cmd, val_len); + memcpy(buf_tx->data, val, val_len); - spi_message_init(&m); - spi_message_add_tail(&t[0], &m); - spi_message_add_tail(&t[1], &m); - - return spi_sync(spi, &m); + return spi_sync_transfer(spi, xfer, ARRAY_SIZE(xfer)); } static int tcan4x5x_regmap_write(void *context, const void *data, size_t count) { - return tcan4x5x_regmap_gather_write(context, data, sizeof(u32), - data + sizeof(u32), - count - sizeof(u32)); + return tcan4x5x_regmap_gather_write(context, data, sizeof(__be32), + data + sizeof(__be32), + count - sizeof(__be32)); } static int tcan4x5x_regmap_read(void *context, - const void *reg, size_t reg_size, - void *val, size_t val_size) + const void *reg_buf, size_t reg_len, + void *val_buf, size_t val_len) { struct spi_device *spi = context; + struct tcan4x5x_priv *priv = spi_get_drvdata(spi); + struct tcan4x5x_map_buf *buf_rx = &priv->map_buf_rx; + struct tcan4x5x_map_buf *buf_tx = &priv->map_buf_tx; + struct spi_transfer xfer[] = { + { + .tx_buf = buf_tx, + } + }; + struct spi_message msg; + int err; + + spi_message_init(&msg); + spi_message_add_tail(&xfer[0], &msg); + + memcpy(&buf_tx->cmd, reg_buf, sizeof(buf_tx->cmd.cmd) + + sizeof(buf_tx->cmd.addr)); + tcan4x5x_spi_cmd_set_len(&buf_tx->cmd, val_len); + + xfer[0].rx_buf = buf_rx; + xfer[0].len = sizeof(buf_tx->cmd) + val_len; - u32 addr = TCAN4X5X_READ_CMD | (*((u16 *)reg) << 8) | val_size >> 2; + if (TCAN4X5X_SANITIZE_SPI) + memset(buf_tx->data, 0x0, val_len); - return spi_write_then_read(spi, &addr, reg_size, (u32 *)val, val_size); + err = spi_sync(spi, &msg); + if (err) + return err; + + memcpy(val_buf, buf_rx->data, val_len); + + return 0; } static const struct regmap_range tcan4x5x_reg_table_yes_range[] = { @@ -66,21 +94,26 @@ static const struct regmap_access_table tcan4x5x_reg_table = { }; static const struct regmap_config tcan4x5x_regmap = { - .reg_bits = 32, + .reg_bits = 24, .reg_stride = 4, + .pad_bits = 8, .val_bits = 32, .wr_table = &tcan4x5x_reg_table, .rd_table = &tcan4x5x_reg_table, - .cache_type = REGCACHE_NONE, .max_register = TCAN4X5X_MAX_REGISTER, + .cache_type = REGCACHE_NONE, + .read_flag_mask = (__force unsigned long) + cpu_to_be32(TCAN4X5X_SPI_INSTRUCTION_READ), + .write_flag_mask = (__force unsigned long) + cpu_to_be32(TCAN4X5X_SPI_INSTRUCTION_WRITE), }; static const struct regmap_bus tcan4x5x_bus = { .write = tcan4x5x_regmap_write, .gather_write = tcan4x5x_regmap_gather_write, .read = tcan4x5x_regmap_read, - .reg_format_endian_default = REGMAP_ENDIAN_NATIVE, - .val_format_endian_default = REGMAP_ENDIAN_NATIVE, + .reg_format_endian_default = REGMAP_ENDIAN_BIG, + .val_format_endian_default = REGMAP_ENDIAN_BIG, .max_raw_read = 256, .max_raw_write = 256, }; diff --git a/drivers/net/can/m_can/tcan4x5x.h b/drivers/net/can/m_can/tcan4x5x.h index e5bdd91b8005..7bf264f8e81f 100644 --- a/drivers/net/can/m_can/tcan4x5x.h +++ b/drivers/net/can/m_can/tcan4x5x.h @@ -17,6 +17,19 @@ #include "m_can.h" +#define TCAN4X5X_SANITIZE_SPI 1 + +struct __packed tcan4x5x_buf_cmd { + u8 cmd; + __be16 addr; + u8 len; +}; + +struct __packed tcan4x5x_map_buf { + struct tcan4x5x_buf_cmd cmd; + u8 data[256 * sizeof(u32)]; +} ____cacheline_aligned; + struct tcan4x5x_priv { struct m_can_classdev cdev; @@ -27,8 +40,18 @@ struct tcan4x5x_priv { struct gpio_desc *device_wake_gpio; struct gpio_desc *device_state_gpio; struct regulator *power; + + struct tcan4x5x_map_buf map_buf_rx; + struct tcan4x5x_map_buf map_buf_tx; }; +static inline void +tcan4x5x_spi_cmd_set_len(struct tcan4x5x_buf_cmd *cmd, u8 len) +{ + /* number of u32 */ + cmd->len = len >> 2; +} + int tcan4x5x_regmap_init(struct tcan4x5x_priv *priv); #endif