On Thu, 7 Dec 2023 20:24:45 +0200 Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Thu, Dec 7, 2023 at 7:52 PM Hugo Villeneuve hugo@hugovil.com wrote:
On Wed, 6 Dec 2023 14:29:39 +0800 kernel test robot lkp@intel.com wrote:
...
drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_i2c_probe':
drivers/tty/serial/sc16is7xx.c:1703:41: warning: '%u' directive output may be truncated writing between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=]
1703 | snprintf(buf, sizeof(buf), "port%u", port_id); | ^~In function 'sc16is7xx_regmap_name', inlined from 'sc16is7xx_i2c_probe' at drivers/tty/serial/sc16is7xx.c:1805:17: drivers/tty/serial/sc16is7xx.c:1703:36: note: directive argument in the range [0, 4294967294] 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); | ^~~~~~~~ drivers/tty/serial/sc16is7xx.c:1703:9: note: 'snprintf' output between 6 and 15 bytes into a destination of size 6 1703 | snprintf(buf, sizeof(buf), "port%u", port_id); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Hi, the only solution I could find is to add this line just before snprintf:
BUG_ON(port_id > MAX310X_MAX_PORTS);it allows us to have the smallest buffer size possible.
One other solution would be to change port_id from "unsigned int" to "u8", and increase the buffer by an additional 2 bytes to silence the warning, but then wasting 2 bytes for each channel, like so:
I didn't get this. It's a buffer that is rewritten on each port (why is it even static?). Just make sure it's enough for any given number and drop the static.
...
While at it, can you look at the following items to improve?
- sc16is7xx_alloc_line() can be updated to use IDA framework
- move return xxx; to the default cases in a few functions
- if (div > 0xffff) { --> if (div >= BIT(16)) { as it better shows why
the limit is that (we have only 16 bits for the divider)
- do {} while (0) in the sc16is7xx_port_irq, WTH?!
- while (1) { -- do { } while (keep_polling); in sc16is7xx_irq()
- use in_range() in sc16is7xx_setup_mctrl_ports() ? (maybe not, dunno)
- for (i--; i >= 0; i--) { --> while (i--) {
- use spi_get_device_match_data() and i2c_get_match_data()
- 15000000 --> 15 * HZ_PER_MHZ ?
- dropping MODULE_ALIAS (and fix the ID tables, _if_ needed)
- split the code to the core / main + SPI + I2C glue drivers
- These just come on the first glance at the code, perhaps there is
more room to improve.
Hi Andy, just to let you know that I have implemented almost all of the fixes / improvements. I will submit them once V2 of this current series lands in Greg's next tree.
However, for sc16is7xx_alloc_line(), I looked at using the IDA framework but it doesn't seem possible because there is no IDA function to search if a bit is set, which is a needed functionality.
Hugo Villeneuve