* [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-11-30 19:10 [PATCH 0/7] serial: sc16is7xx and max310x: regmap fixes and improvements Hugo Villeneuve
@ 2023-11-30 19:10 ` Hugo Villeneuve
2023-12-06 6:29 ` kernel test robot
` (2 more replies)
2023-11-30 19:10 ` [PATCH 2/7] serial: sc16is7xx: remove global regmap from struct sc16is7xx_port Hugo Villeneuve
` (5 subsequent siblings)
6 siblings, 3 replies; 22+ messages in thread
From: Hugo Villeneuve @ 2023-11-30 19:10 UTC (permalink / raw)
To: gregkh, jirislaby, hvilleneuve
Cc: linux-kernel, linux-serial, hugo, stable, Andy Shevchenko
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Change snprint format specifier from %d to %u since port_id is unsigned.
Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
Cc: stable@vger.kernel.org # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
I did not originally add a "Cc: stable" tag for commit 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
as it was intended only to improve debugging using debugfs. But
since then, I have been able to confirm that it also fixes a long standing
bug in our system where the Tx interrupt are no longer enabled at some
point when transmitting large RS-485 paquets (> 64 bytes, which is the size
of the FIFO). I have been investigating why, but so far I haven't found the
exact cause, altough I suspect it has something to do with regmap caching.
Therefore, I have added it as a prerequisite for this patch so that it is
automatically added to the stable kernels.
---
drivers/tty/serial/sc16is7xx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 10e90a7774f0..8e5baf2f6ec6 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1700,7 +1700,7 @@ static const char *sc16is7xx_regmap_name(unsigned int port_id)
{
static char buf[6];
- snprintf(buf, sizeof(buf), "port%d", port_id);
+ snprintf(buf, sizeof(buf), "port%u", port_id);
return buf;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-11-30 19:10 ` [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name() Hugo Villeneuve
@ 2023-12-06 6:29 ` kernel test robot
2023-12-07 17:52 ` Hugo Villeneuve
2023-12-07 1:44 ` Greg KH
2023-12-07 1:45 ` Greg KH
2 siblings, 1 reply; 22+ messages in thread
From: kernel test robot @ 2023-12-06 6:29 UTC (permalink / raw)
To: Hugo Villeneuve, gregkh, jirislaby, hvilleneuve
Cc: oe-kbuild-all, linux-kernel, linux-serial, hugo, stable,
Andy Shevchenko
Hi Hugo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on d804987153e7bedf503f8e4ba649afe52cfd7f6d]
url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-fix-snprintf-format-specifier-in-sc16is7xx_regmap_name/20231201-031413
base: d804987153e7bedf503f8e4ba649afe52cfd7f6d
patch link: https://lore.kernel.org/r/20231130191050.3165862-2-hugo%40hugovil.com
patch subject: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
config: x86_64-buildonly-randconfig-001-20231201 (https://download.01.org/0day-ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312061443.Cknef7Uq-lkp@intel.com/
All warnings (new ones prefixed by >>):
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);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +1703 drivers/tty/serial/sc16is7xx.c
1698
1699 static const char *sc16is7xx_regmap_name(unsigned int port_id)
1700 {
1701 static char buf[6];
1702
> 1703 snprintf(buf, sizeof(buf), "port%u", port_id);
1704
1705 return buf;
1706 }
1707
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-12-06 6:29 ` kernel test robot
@ 2023-12-07 17:52 ` Hugo Villeneuve
2023-12-07 18:24 ` Andy Shevchenko
2023-12-07 19:45 ` David Laight
0 siblings, 2 replies; 22+ messages in thread
From: Hugo Villeneuve @ 2023-12-07 17:52 UTC (permalink / raw)
To: kernel test robot
Cc: gregkh, jirislaby, hvilleneuve, oe-kbuild-all, linux-kernel,
linux-serial, stable, Andy Shevchenko
On Wed, 6 Dec 2023 14:29:39 +0800
kernel test robot <lkp@intel.com> wrote:
> Hi Hugo,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on d804987153e7bedf503f8e4ba649afe52cfd7f6d]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-fix-snprintf-format-specifier-in-sc16is7xx_regmap_name/20231201-031413
> base: d804987153e7bedf503f8e4ba649afe52cfd7f6d
> patch link: https://lore.kernel.org/r/20231130191050.3165862-2-hugo%40hugovil.com
> patch subject: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
> config: x86_64-buildonly-randconfig-001-20231201 (https://download.01.org/0day-ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202312061443.Cknef7Uq-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> 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:
static const char *max310x_regmap_name(u8 port_id)
{
static char buf[
sizeof(MAX310X_PORT_NAME_SUFFIX __stringify(UCHAR_MAX))];
I prefer solution 1, unless there is another solution that I am
unaware of.
Hugo.
> vim +1703 drivers/tty/serial/sc16is7xx.c
>
> 1698
> 1699 static const char *sc16is7xx_regmap_name(unsigned int port_id)
> 1700 {
> 1701 static char buf[6];
> 1702
> > 1703 snprintf(buf, sizeof(buf), "port%u", port_id);
> 1704
> 1705 return buf;
> 1706 }
> 1707
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
--
Hugo Villeneuve
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-12-07 17:52 ` Hugo Villeneuve
@ 2023-12-07 18:24 ` Andy Shevchenko
2023-12-07 19:02 ` Hugo Villeneuve
2023-12-12 20:03 ` Hugo Villeneuve
2023-12-07 19:45 ` David Laight
1 sibling, 2 replies; 22+ messages in thread
From: Andy Shevchenko @ 2023-12-07 18:24 UTC (permalink / raw)
To: Hugo Villeneuve
Cc: kernel test robot, gregkh, jirislaby, hvilleneuve, oe-kbuild-all,
linux-kernel, linux-serial, stable
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-12-07 18:24 ` Andy Shevchenko
@ 2023-12-07 19:02 ` Hugo Villeneuve
2023-12-12 20:03 ` Hugo Villeneuve
1 sibling, 0 replies; 22+ messages in thread
From: Hugo Villeneuve @ 2023-12-07 19:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: kernel test robot, gregkh, jirislaby, hvilleneuve, oe-kbuild-all,
linux-kernel, linux-serial, stable
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.
Yes, using static is not appropriate, as regmap will copy each name
into its internal buffer.
I will drop the static and refactor the code accordingly.
> 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.
Ok, no problem, I will have a look at it.
Thank you,
Hugo Villeneuve
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-12-07 18:24 ` Andy Shevchenko
2023-12-07 19:02 ` Hugo Villeneuve
@ 2023-12-12 20:03 ` Hugo Villeneuve
2023-12-13 14:43 ` Andy Shevchenko
1 sibling, 1 reply; 22+ messages in thread
From: Hugo Villeneuve @ 2023-12-12 20:03 UTC (permalink / raw)
To: Andy Shevchenko
Cc: kernel test robot, gregkh, jirislaby, hvilleneuve, oe-kbuild-all,
linux-kernel, linux-serial, stable
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-12-12 20:03 ` Hugo Villeneuve
@ 2023-12-13 14:43 ` Andy Shevchenko
0 siblings, 0 replies; 22+ messages in thread
From: Andy Shevchenko @ 2023-12-13 14:43 UTC (permalink / raw)
To: Hugo Villeneuve
Cc: kernel test robot, gregkh, jirislaby, hvilleneuve, oe-kbuild-all,
linux-kernel, linux-serial, stable
On Tue, Dec 12, 2023 at 10:03 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> 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:
...
> > 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.
Hooray!
> 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.
It can be done via trying to get it, but probably it's uglier than
current behaviour. Okay, let's leave it as is for now.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-12-07 17:52 ` Hugo Villeneuve
2023-12-07 18:24 ` Andy Shevchenko
@ 2023-12-07 19:45 ` David Laight
1 sibling, 0 replies; 22+ messages in thread
From: David Laight @ 2023-12-07 19:45 UTC (permalink / raw)
To: 'Hugo Villeneuve', kernel test robot
Cc: gregkh@linuxfoundation.org, jirislaby@kernel.org,
hvilleneuve@dimonoff.com, oe-kbuild-all@lists.linux.dev,
linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
stable@vger.kernel.org, Andy Shevchenko
From: Hugo Villeneuve
> Sent: 07 December 2023 17:53
...
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on d804987153e7bedf503f8e4ba649afe52cfd7f6d]
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-fix-
> snprintf-format-specifier-in-sc16is7xx_regmap_name/20231201-031413
> > base: d804987153e7bedf503f8e4ba649afe52cfd7f6d
> > patch link: https://lore.kernel.org/r/20231130191050.3165862-2-hugo%40hugovil.com
> > patch subject: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in
> sc16is7xx_regmap_name()
> > config: x86_64-buildonly-randconfig-001-20231201 (https://download.01.org/0day-
> ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/config)
> > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-
> ci/archive/20231206/202312061443.Cknef7Uq-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202312061443.Cknef7Uq-lkp@intel.com/
> >
> > All warnings (new ones prefixed by >>):
> >
> > 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.
Or "port%c", '0' + port_id);
Or maybe:
size_t buflen = sizeof (buf);
OPTIMIZER_HIDE_VAR(buflen);
snprintf(buf, buflen, fmt, args);
See https://godbolt.org/z/Wjz3xG5c4
Maybe there should be snprintf_may_truncate() (etc) in one of the headers.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-11-30 19:10 ` [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name() Hugo Villeneuve
2023-12-06 6:29 ` kernel test robot
@ 2023-12-07 1:44 ` Greg KH
2023-12-07 16:02 ` Hugo Villeneuve
2023-12-07 1:45 ` Greg KH
2 siblings, 1 reply; 22+ messages in thread
From: Greg KH @ 2023-12-07 1:44 UTC (permalink / raw)
To: Hugo Villeneuve
Cc: jirislaby, hvilleneuve, linux-kernel, linux-serial, stable,
Andy Shevchenko
On Thu, Nov 30, 2023 at 02:10:43PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> Change snprint format specifier from %d to %u since port_id is unsigned.
>
> Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
> Cc: stable@vger.kernel.org # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
> I did not originally add a "Cc: stable" tag for commit 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
> as it was intended only to improve debugging using debugfs. But
> since then, I have been able to confirm that it also fixes a long standing
> bug in our system where the Tx interrupt are no longer enabled at some
> point when transmitting large RS-485 paquets (> 64 bytes, which is the size
> of the FIFO). I have been investigating why, but so far I haven't found the
> exact cause, altough I suspect it has something to do with regmap caching.
> Therefore, I have added it as a prerequisite for this patch so that it is
> automatically added to the stable kernels.
Looks like the 0-day test bot found problems with this, so I'll hold off
on taking this patch and the rest of the series until that's fixed up
with a new version of this series.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-12-07 1:44 ` Greg KH
@ 2023-12-07 16:02 ` Hugo Villeneuve
0 siblings, 0 replies; 22+ messages in thread
From: Hugo Villeneuve @ 2023-12-07 16:02 UTC (permalink / raw)
To: Greg KH
Cc: jirislaby, hvilleneuve, linux-kernel, linux-serial, stable,
Andy Shevchenko
On Thu, 7 Dec 2023 10:44:33 +0900
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 30, 2023 at 02:10:43PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >
> > Change snprint format specifier from %d to %u since port_id is unsigned.
> >
> > Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
> > Cc: stable@vger.kernel.org # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port
> > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> > I did not originally add a "Cc: stable" tag for commit 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
> > as it was intended only to improve debugging using debugfs. But
> > since then, I have been able to confirm that it also fixes a long standing
> > bug in our system where the Tx interrupt are no longer enabled at some
> > point when transmitting large RS-485 paquets (> 64 bytes, which is the size
> > of the FIFO). I have been investigating why, but so far I haven't found the
> > exact cause, altough I suspect it has something to do with regmap caching.
> > Therefore, I have added it as a prerequisite for this patch so that it is
> > automatically added to the stable kernels.
>
> Looks like the 0-day test bot found problems with this, so I'll hold off
> on taking this patch and the rest of the series until that's fixed up
> with a new version of this series.
No problem, I am on it.
Hugo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-11-30 19:10 ` [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name() Hugo Villeneuve
2023-12-06 6:29 ` kernel test robot
2023-12-07 1:44 ` Greg KH
@ 2023-12-07 1:45 ` Greg KH
2023-12-07 16:05 ` Hugo Villeneuve
2 siblings, 1 reply; 22+ messages in thread
From: Greg KH @ 2023-12-07 1:45 UTC (permalink / raw)
To: Hugo Villeneuve
Cc: jirislaby, hvilleneuve, linux-kernel, linux-serial, stable,
Andy Shevchenko
On Thu, Nov 30, 2023 at 02:10:43PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> Change snprint format specifier from %d to %u since port_id is unsigned.
>
> Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
> Cc: stable@vger.kernel.org # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
> I did not originally add a "Cc: stable" tag for commit 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
> as it was intended only to improve debugging using debugfs. But
> since then, I have been able to confirm that it also fixes a long standing
> bug in our system where the Tx interrupt are no longer enabled at some
> point when transmitting large RS-485 paquets (> 64 bytes, which is the size
> of the FIFO). I have been investigating why, but so far I haven't found the
> exact cause, altough I suspect it has something to do with regmap caching.
> Therefore, I have added it as a prerequisite for this patch so that it is
> automatically added to the stable kernels.
As you are splitting fixes from non-fixes in this series, please resend
this as 2 different series, one that I can apply now to my tty-linus
branch to get merged for 6.7-final, and one that can go into tty-next
for 6.8-rc1. Mixing them up here just ensures that they all would get
applied to tty-next.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name()
2023-12-07 1:45 ` Greg KH
@ 2023-12-07 16:05 ` Hugo Villeneuve
0 siblings, 0 replies; 22+ messages in thread
From: Hugo Villeneuve @ 2023-12-07 16:05 UTC (permalink / raw)
To: Greg KH
Cc: jirislaby, hvilleneuve, linux-kernel, linux-serial, stable,
Andy Shevchenko
On Thu, 7 Dec 2023 10:45:48 +0900
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 30, 2023 at 02:10:43PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> >
> > Change snprint format specifier from %d to %u since port_id is unsigned.
> >
> > Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
> > Cc: stable@vger.kernel.org # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port
> > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> > I did not originally add a "Cc: stable" tag for commit 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
> > as it was intended only to improve debugging using debugfs. But
> > since then, I have been able to confirm that it also fixes a long standing
> > bug in our system where the Tx interrupt are no longer enabled at some
> > point when transmitting large RS-485 paquets (> 64 bytes, which is the size
> > of the FIFO). I have been investigating why, but so far I haven't found the
> > exact cause, altough I suspect it has something to do with regmap caching.
> > Therefore, I have added it as a prerequisite for this patch so that it is
> > automatically added to the stable kernels.
>
> As you are splitting fixes from non-fixes in this series, please resend
> this as 2 different series, one that I can apply now to my tty-linus
> branch to get merged for 6.7-final, and one that can go into tty-next
> for 6.8-rc1. Mixing them up here just ensures that they all would get
> applied to tty-next.
Ok, makes sense. Will do after I fix the 0-day issue.
Thank you,
Hugo.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/7] serial: sc16is7xx: remove global regmap from struct sc16is7xx_port
2023-11-30 19:10 [PATCH 0/7] serial: sc16is7xx and max310x: regmap fixes and improvements Hugo Villeneuve
2023-11-30 19:10 ` [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name() Hugo Villeneuve
@ 2023-11-30 19:10 ` Hugo Villeneuve
2023-11-30 19:10 ` [PATCH 3/7] serial: sc16is7xx: remove unused line structure member Hugo Villeneuve
` (4 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Hugo Villeneuve @ 2023-11-30 19:10 UTC (permalink / raw)
To: gregkh, jirislaby, hvilleneuve
Cc: linux-kernel, linux-serial, hugo, stable, Andy Shevchenko
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Remove global struct regmap so that it is more obvious that this
regmap is to be used only in the probe function.
Also add a comment to that effect in probe function.
Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
Cc: stable@vger.kernel.org
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
drivers/tty/serial/sc16is7xx.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 8e5baf2f6ec6..23dbf77633aa 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -334,7 +334,6 @@ struct sc16is7xx_one {
struct sc16is7xx_port {
const struct sc16is7xx_devtype *devtype;
- struct regmap *regmap;
struct clk *clk;
#ifdef CONFIG_GPIOLIB
struct gpio_chip gpio;
@@ -1422,7 +1421,8 @@ static void sc16is7xx_setup_irda_ports(struct sc16is7xx_port *s)
/*
* Configure ports designated to operate as modem control lines.
*/
-static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
+static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s,
+ struct regmap *regmap)
{
int i;
int ret;
@@ -1451,7 +1451,7 @@ static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
if (s->mctrl_mask)
regmap_update_bits(
- s->regmap,
+ regmap,
SC16IS7XX_IOCONTROL_REG,
SC16IS7XX_IOCONTROL_MODEM_A_BIT |
SC16IS7XX_IOCONTROL_MODEM_B_BIT, s->mctrl_mask);
@@ -1483,6 +1483,10 @@ static int sc16is7xx_probe(struct device *dev,
* This device does not have an identification register that would
* tell us if we are really connected to the correct device.
* The best we can do is to check if communication is at all possible.
+ *
+ * Note: regmap[0] is used in the probe function to access registers
+ * common to all channels/ports, as it is guaranteed to be present on
+ * all variants.
*/
ret = regmap_read(regmaps[0], SC16IS7XX_LSR_REG, &val);
if (ret < 0)
@@ -1518,7 +1522,6 @@ static int sc16is7xx_probe(struct device *dev,
return -EINVAL;
}
- s->regmap = regmaps[0];
s->devtype = devtype;
dev_set_drvdata(dev, s);
mutex_init(&s->efr_lock);
@@ -1533,7 +1536,7 @@ static int sc16is7xx_probe(struct device *dev,
sched_set_fifo(s->kworker_task);
/* reset device, purging any pending irq / data */
- regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG,
+ regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
SC16IS7XX_IOCONTROL_SRESET_BIT);
for (i = 0; i < devtype->nr_uart; ++i) {
@@ -1604,7 +1607,7 @@ static int sc16is7xx_probe(struct device *dev,
sc16is7xx_setup_irda_ports(s);
- ret = sc16is7xx_setup_mctrl_ports(s);
+ ret = sc16is7xx_setup_mctrl_ports(s, regmaps[0]);
if (ret)
goto out_ports;
--
2.39.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 3/7] serial: sc16is7xx: remove unused line structure member
2023-11-30 19:10 [PATCH 0/7] serial: sc16is7xx and max310x: regmap fixes and improvements Hugo Villeneuve
2023-11-30 19:10 ` [PATCH 1/7] serial: sc16is7xx: fix snprintf format specifier in sc16is7xx_regmap_name() Hugo Villeneuve
2023-11-30 19:10 ` [PATCH 2/7] serial: sc16is7xx: remove global regmap from struct sc16is7xx_port Hugo Villeneuve
@ 2023-11-30 19:10 ` Hugo Villeneuve
2023-11-30 19:10 ` [PATCH 4/7] serial: sc16is7xx: add macro for max number of UART ports Hugo Villeneuve
` (3 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Hugo Villeneuve @ 2023-11-30 19:10 UTC (permalink / raw)
To: gregkh, jirislaby, hvilleneuve; +Cc: linux-kernel, linux-serial, hugo, stable
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Now that the driver has been converted to use one regmap per port, the line
structure member is no longer used, so remove it.
Fixes: 3837a0379533 ("serial: sc16is7xx: improve regmap debugfs by using one regmap per port")
Cc: stable@vger.kernel.org
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
drivers/tty/serial/sc16is7xx.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 23dbf77633aa..eb2c0dcd3775 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -322,7 +322,6 @@ struct sc16is7xx_one_config {
struct sc16is7xx_one {
struct uart_port port;
- u8 line;
struct regmap *regmap;
struct kthread_work tx_work;
struct kthread_work reg_work;
@@ -1540,7 +1539,6 @@ static int sc16is7xx_probe(struct device *dev,
SC16IS7XX_IOCONTROL_SRESET_BIT);
for (i = 0; i < devtype->nr_uart; ++i) {
- s->p[i].line = i;
/* Initialize port data */
s->p[i].port.dev = dev;
s->p[i].port.irq = irq;
--
2.39.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 4/7] serial: sc16is7xx: add macro for max number of UART ports
2023-11-30 19:10 [PATCH 0/7] serial: sc16is7xx and max310x: regmap fixes and improvements Hugo Villeneuve
` (2 preceding siblings ...)
2023-11-30 19:10 ` [PATCH 3/7] serial: sc16is7xx: remove unused line structure member Hugo Villeneuve
@ 2023-11-30 19:10 ` Hugo Villeneuve
2023-11-30 19:10 ` [PATCH 5/7] serial: sc16is7xx: improve sc16is7xx_regmap_name() buffer size computation Hugo Villeneuve
` (2 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Hugo Villeneuve @ 2023-11-30 19:10 UTC (permalink / raw)
To: gregkh, jirislaby, hvilleneuve; +Cc: linux-kernel, linux-serial, hugo
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Add macro to hold the maximum number of UART ports per IC/device.
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
drivers/tty/serial/sc16is7xx.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index eb2c0dcd3775..750c55b93f5e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -28,6 +28,7 @@
#define SC16IS7XX_NAME "sc16is7xx"
#define SC16IS7XX_MAX_DEVS 8
+#define SC16IS7XX_MAX_PORTS 2 /* Maximum number of UART ports per IC. */
/* SC16IS7XX register definitions */
#define SC16IS7XX_RHR_REG (0x00) /* RX FIFO */
@@ -1399,7 +1400,7 @@ static void sc16is7xx_setup_irda_ports(struct sc16is7xx_port *s)
int i;
int ret;
int count;
- u32 irda_port[2];
+ u32 irda_port[SC16IS7XX_MAX_PORTS];
struct device *dev = s->p[0].port.dev;
count = device_property_count_u32(dev, "irda-mode-ports");
@@ -1426,7 +1427,7 @@ static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s,
int i;
int ret;
int count;
- u32 mctrl_port[2];
+ u32 mctrl_port[SC16IS7XX_MAX_PORTS];
struct device *dev = s->p[0].port.dev;
count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
@@ -1716,7 +1717,7 @@ static unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id)
static int sc16is7xx_spi_probe(struct spi_device *spi)
{
const struct sc16is7xx_devtype *devtype;
- struct regmap *regmaps[2];
+ struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
unsigned int i;
int ret;
@@ -1791,7 +1792,7 @@ static int sc16is7xx_i2c_probe(struct i2c_client *i2c)
{
const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
const struct sc16is7xx_devtype *devtype;
- struct regmap *regmaps[2];
+ struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
unsigned int i;
if (i2c->dev.of_node) {
--
2.39.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 5/7] serial: sc16is7xx: improve sc16is7xx_regmap_name() buffer size computation
2023-11-30 19:10 [PATCH 0/7] serial: sc16is7xx and max310x: regmap fixes and improvements Hugo Villeneuve
` (3 preceding siblings ...)
2023-11-30 19:10 ` [PATCH 4/7] serial: sc16is7xx: add macro for max number of UART ports Hugo Villeneuve
@ 2023-11-30 19:10 ` Hugo Villeneuve
2023-11-30 19:10 ` [PATCH 6/7] serial: max310x: add macro for max number of ports Hugo Villeneuve
2023-11-30 19:10 ` [PATCH 7/7] serial: max310x: use separate regmap name for each port Hugo Villeneuve
6 siblings, 0 replies; 22+ messages in thread
From: Hugo Villeneuve @ 2023-11-30 19:10 UTC (permalink / raw)
To: gregkh, jirislaby, hvilleneuve; +Cc: linux-kernel, linux-serial, hugo
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Define macro for regmap port name suffix and use it in addition to
SC16IS7XX_MAX_PORTS to automatically compute the required buffer size to
hold the name.
This helps with code readability by making it more obvious what is the
required size of the buffer.
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
drivers/tty/serial/sc16is7xx.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 750c55b93f5e..b02e6c79da67 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -20,6 +20,7 @@
#include <linux/regmap.h>
#include <linux/serial_core.h>
#include <linux/serial.h>
+#include <linux/stringify.h>
#include <linux/tty.h>
#include <linux/tty_flip.h>
#include <linux/spi/spi.h>
@@ -27,6 +28,7 @@
#include <uapi/linux/sched/types.h>
#define SC16IS7XX_NAME "sc16is7xx"
+#define SC16IS7XX_PORT_NAME_SUFFIX "port" /* Used for regmap name. */
#define SC16IS7XX_MAX_DEVS 8
#define SC16IS7XX_MAX_PORTS 2 /* Maximum number of UART ports per IC. */
@@ -1700,9 +1702,9 @@ static struct regmap_config regcfg = {
static const char *sc16is7xx_regmap_name(unsigned int port_id)
{
- static char buf[6];
+ static char buf[sizeof(SC16IS7XX_PORT_NAME_SUFFIX __stringify(SC16IS7XX_MAX_PORTS))];
- snprintf(buf, sizeof(buf), "port%u", port_id);
+ snprintf(buf, sizeof(buf), SC16IS7XX_PORT_NAME_SUFFIX "%u", port_id);
return buf;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 6/7] serial: max310x: add macro for max number of ports
2023-11-30 19:10 [PATCH 0/7] serial: sc16is7xx and max310x: regmap fixes and improvements Hugo Villeneuve
` (4 preceding siblings ...)
2023-11-30 19:10 ` [PATCH 5/7] serial: sc16is7xx: improve sc16is7xx_regmap_name() buffer size computation Hugo Villeneuve
@ 2023-11-30 19:10 ` Hugo Villeneuve
2023-12-01 15:59 ` Jan Kundrát
2023-11-30 19:10 ` [PATCH 7/7] serial: max310x: use separate regmap name for each port Hugo Villeneuve
6 siblings, 1 reply; 22+ messages in thread
From: Hugo Villeneuve @ 2023-11-30 19:10 UTC (permalink / raw)
To: gregkh, jirislaby, hvilleneuve; +Cc: linux-kernel, linux-serial, hugo
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Add macro to hold the maximum number of UART ports per IC/device.
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
drivers/tty/serial/max310x.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index f3a99daebdaa..58dd5cc62014 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -30,6 +30,7 @@
#define MAX310X_MAJOR 204
#define MAX310X_MINOR 209
#define MAX310X_UART_NRMAX 16
+#define MAX310X_MAX_PORTS 4 /* Maximum number of UART ports per IC. */
/* MAX310X register definitions */
#define MAX310X_RHR_REG (0x00) /* RX FIFO */
@@ -1502,7 +1503,7 @@ static const struct max310x_if_cfg __maybe_unused max310x_spi_if_cfg = {
static int max310x_spi_probe(struct spi_device *spi)
{
const struct max310x_devtype *devtype;
- struct regmap *regmaps[4];
+ struct regmap *regmaps[MAX310X_MAX_PORTS];
unsigned int i;
int ret;
@@ -1605,7 +1606,7 @@ static int max310x_i2c_probe(struct i2c_client *client)
const struct max310x_devtype *devtype =
device_get_match_data(&client->dev);
struct i2c_client *port_client;
- struct regmap *regmaps[4];
+ struct regmap *regmaps[MAX310X_MAX_PORTS];
unsigned int i;
u8 port_addr;
--
2.39.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 6/7] serial: max310x: add macro for max number of ports
2023-11-30 19:10 ` [PATCH 6/7] serial: max310x: add macro for max number of ports Hugo Villeneuve
@ 2023-12-01 15:59 ` Jan Kundrát
0 siblings, 0 replies; 22+ messages in thread
From: Jan Kundrát @ 2023-12-01 15:59 UTC (permalink / raw)
To: Hugo Villeneuve
Cc: gregkh, jirislaby, hvilleneuve, linux-kernel, linux-serial
On čtvrtek 30. listopadu 2023 20:10:48 CET, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> Add macro to hold the maximum number of UART ports per IC/device.
>
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Jan Kundrát <jan.kundrat@cesnet.cz>
Tested-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> ---
> drivers/tty/serial/max310x.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index f3a99daebdaa..58dd5cc62014 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -30,6 +30,7 @@
> #define MAX310X_MAJOR 204
> #define MAX310X_MINOR 209
> #define MAX310X_UART_NRMAX 16
> +#define MAX310X_MAX_PORTS 4 /* Maximum number of UART ports per IC. */
>
> /* MAX310X register definitions */
> #define MAX310X_RHR_REG (0x00) /* RX FIFO */
> @@ -1502,7 +1503,7 @@ static const struct max310x_if_cfg
> __maybe_unused max310x_spi_if_cfg = {
> static int max310x_spi_probe(struct spi_device *spi)
> {
> const struct max310x_devtype *devtype;
> - struct regmap *regmaps[4];
> + struct regmap *regmaps[MAX310X_MAX_PORTS];
> unsigned int i;
> int ret;
>
> @@ -1605,7 +1606,7 @@ static int max310x_i2c_probe(struct
> i2c_client *client)
> const struct max310x_devtype *devtype =
> device_get_match_data(&client->dev);
> struct i2c_client *port_client;
> - struct regmap *regmaps[4];
> + struct regmap *regmaps[MAX310X_MAX_PORTS];
> unsigned int i;
> u8 port_addr;
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 7/7] serial: max310x: use separate regmap name for each port
2023-11-30 19:10 [PATCH 0/7] serial: sc16is7xx and max310x: regmap fixes and improvements Hugo Villeneuve
` (5 preceding siblings ...)
2023-11-30 19:10 ` [PATCH 6/7] serial: max310x: add macro for max number of ports Hugo Villeneuve
@ 2023-11-30 19:10 ` Hugo Villeneuve
2023-12-01 16:00 ` Jan Kundrát
2023-12-06 14:22 ` kernel test robot
6 siblings, 2 replies; 22+ messages in thread
From: Hugo Villeneuve @ 2023-11-30 19:10 UTC (permalink / raw)
To: gregkh, jirislaby, hvilleneuve; +Cc: linux-kernel, linux-serial, hugo
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Use a separate regmap name for each port so that each port can have its own
debugfs entry, allowing to access each port registers independently.
For example, a four channels/ports device like the MAX14830 will have four
entries in its regmap debugfs:
$ find /sys/kernel/debug/regmap -type d | grep spi0.0
/sys/kernel/debug/regmap/spi0.0-port0
/sys/kernel/debug/regmap/spi0.0-port1
/sys/kernel/debug/regmap/spi0.0-port2
/sys/kernel/debug/regmap/spi0.0-port3
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
drivers/tty/serial/max310x.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 58dd5cc62014..d2eca05a6966 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -27,6 +27,7 @@
#include <linux/uaccess.h>
#define MAX310X_NAME "max310x"
+#define MAX310X_PORT_NAME_SUFFIX "port"
#define MAX310X_MAJOR 204
#define MAX310X_MINOR 209
#define MAX310X_UART_NRMAX 16
@@ -1486,6 +1487,15 @@ static struct regmap_config regcfg = {
.max_raw_write = MAX310X_FIFO_SIZE,
};
+static const char *max310x_regmap_name(unsigned int port_id)
+{
+ static char buf[sizeof(MAX310X_PORT_NAME_SUFFIX __stringify(MAX310X_MAX_PORTS))];
+
+ snprintf(buf, sizeof(buf), MAX310X_PORT_NAME_SUFFIX "%u", port_id);
+
+ return buf;
+}
+
#ifdef CONFIG_SPI_MASTER
static int max310x_spi_extended_reg_enable(struct device *dev, bool enable)
{
@@ -1521,6 +1531,8 @@ static int max310x_spi_probe(struct spi_device *spi)
for (i = 0; i < devtype->nr; i++) {
u8 port_mask = i * 0x20;
+
+ regcfg.name = max310x_regmap_name(i);
regcfg.read_flag_mask = port_mask;
regcfg.write_flag_mask = port_mask | MAX310X_WRITE_BIT;
regmaps[i] = devm_regmap_init_spi(spi, ®cfg);
@@ -1617,6 +1629,7 @@ static int max310x_i2c_probe(struct i2c_client *client)
client->addr, devtype->slave_addr.min,
devtype->slave_addr.max);
+ regcfg_i2c.name = max310x_regmap_name(0);
regmaps[0] = devm_regmap_init_i2c(client, ®cfg_i2c);
for (i = 1; i < devtype->nr; i++) {
@@ -1625,6 +1638,7 @@ static int max310x_i2c_probe(struct i2c_client *client)
client->adapter,
port_addr);
+ regcfg_i2c.name = max310x_regmap_name(i);
regmaps[i] = devm_regmap_init_i2c(port_client, ®cfg_i2c);
}
--
2.39.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 7/7] serial: max310x: use separate regmap name for each port
2023-11-30 19:10 ` [PATCH 7/7] serial: max310x: use separate regmap name for each port Hugo Villeneuve
@ 2023-12-01 16:00 ` Jan Kundrát
2023-12-06 14:22 ` kernel test robot
1 sibling, 0 replies; 22+ messages in thread
From: Jan Kundrát @ 2023-12-01 16:00 UTC (permalink / raw)
To: Hugo Villeneuve
Cc: gregkh, jirislaby, hvilleneuve, linux-kernel, linux-serial
On čtvrtek 30. listopadu 2023 20:10:49 CET, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> Use a separate regmap name for each port so that each port can have its own
> debugfs entry, allowing to access each port registers independently.
>
> For example, a four channels/ports device like the MAX14830 will have four
> entries in its regmap debugfs:
>
> $ find /sys/kernel/debug/regmap -type d | grep spi0.0
> /sys/kernel/debug/regmap/spi0.0-port0
> /sys/kernel/debug/regmap/spi0.0-port1
> /sys/kernel/debug/regmap/spi0.0-port2
> /sys/kernel/debug/regmap/spi0.0-port3
>
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
I was carrying a similar patch locally, and this one works for me as well.
Reviewed-by: Jan Kundrát <jan.kundrat@cesnet.cz>
Tested-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> ---
> drivers/tty/serial/max310x.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 58dd5cc62014..d2eca05a6966 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -27,6 +27,7 @@
> #include <linux/uaccess.h>
>
> #define MAX310X_NAME "max310x"
> +#define MAX310X_PORT_NAME_SUFFIX "port"
> #define MAX310X_MAJOR 204
> #define MAX310X_MINOR 209
> #define MAX310X_UART_NRMAX 16
> @@ -1486,6 +1487,15 @@ static struct regmap_config regcfg = {
> .max_raw_write = MAX310X_FIFO_SIZE,
> };
>
> +static const char *max310x_regmap_name(unsigned int port_id)
> +{
> + static char buf[sizeof(MAX310X_PORT_NAME_SUFFIX
> __stringify(MAX310X_MAX_PORTS))];
> +
> + snprintf(buf, sizeof(buf), MAX310X_PORT_NAME_SUFFIX "%u", port_id);
> +
> + return buf;
> +}
> +
> #ifdef CONFIG_SPI_MASTER
> static int max310x_spi_extended_reg_enable(struct device *dev, bool enable)
> {
> @@ -1521,6 +1531,8 @@ static int max310x_spi_probe(struct spi_device *spi)
>
> for (i = 0; i < devtype->nr; i++) {
> u8 port_mask = i * 0x20;
> +
> + regcfg.name = max310x_regmap_name(i);
> regcfg.read_flag_mask = port_mask;
> regcfg.write_flag_mask = port_mask | MAX310X_WRITE_BIT;
> regmaps[i] = devm_regmap_init_spi(spi, ®cfg);
> @@ -1617,6 +1629,7 @@ static int max310x_i2c_probe(struct
> i2c_client *client)
> client->addr, devtype->slave_addr.min,
> devtype->slave_addr.max);
>
> + regcfg_i2c.name = max310x_regmap_name(0);
> regmaps[0] = devm_regmap_init_i2c(client, ®cfg_i2c);
>
> for (i = 1; i < devtype->nr; i++) {
> @@ -1625,6 +1638,7 @@ static int max310x_i2c_probe(struct
> i2c_client *client)
> client->adapter,
> port_addr);
>
> + regcfg_i2c.name = max310x_regmap_name(i);
> regmaps[i] = devm_regmap_init_i2c(port_client, ®cfg_i2c);
> }
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 7/7] serial: max310x: use separate regmap name for each port
2023-11-30 19:10 ` [PATCH 7/7] serial: max310x: use separate regmap name for each port Hugo Villeneuve
2023-12-01 16:00 ` Jan Kundrát
@ 2023-12-06 14:22 ` kernel test robot
1 sibling, 0 replies; 22+ messages in thread
From: kernel test robot @ 2023-12-06 14:22 UTC (permalink / raw)
To: Hugo Villeneuve, gregkh, jirislaby, hvilleneuve
Cc: oe-kbuild-all, linux-kernel, linux-serial, hugo
Hi Hugo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on d804987153e7bedf503f8e4ba649afe52cfd7f6d]
url: https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-fix-snprintf-format-specifier-in-sc16is7xx_regmap_name/20231201-031413
base: d804987153e7bedf503f8e4ba649afe52cfd7f6d
patch link: https://lore.kernel.org/r/20231130191050.3165862-8-hugo%40hugovil.com
patch subject: [PATCH 7/7] serial: max310x: use separate regmap name for each port
config: arm-randconfig-r081-20231201 (https://download.01.org/0day-ci/archive/20231206/202312062240.v39k99mQ-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/202312062240.v39k99mQ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312062240.v39k99mQ-lkp@intel.com/
All warnings (new ones prefixed by >>):
In function 'max310x_regmap_name',
inlined from 'max310x_i2c_probe' at drivers/tty/serial/max310x.c:1641:21:
>> drivers/tty/serial/max310x.c:30:41: warning: '%u' directive output may be truncated writing between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=]
30 | #define MAX310X_PORT_NAME_SUFFIX "port"
| ^~~~~~
drivers/tty/serial/max310x.c:1494:36: note: in expansion of macro 'MAX310X_PORT_NAME_SUFFIX'
1494 | snprintf(buf, sizeof(buf), MAX310X_PORT_NAME_SUFFIX "%u", port_id);
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/max310x.c: In function 'max310x_i2c_probe':
drivers/tty/serial/max310x.c:1494:62: note: format string is defined here
1494 | snprintf(buf, sizeof(buf), MAX310X_PORT_NAME_SUFFIX "%u", port_id);
| ^~
In function 'max310x_regmap_name',
inlined from 'max310x_i2c_probe' at drivers/tty/serial/max310x.c:1641:21:
drivers/tty/serial/max310x.c:30:41: note: directive argument in the range [1, 4294967294]
30 | #define MAX310X_PORT_NAME_SUFFIX "port"
| ^~~~~~
drivers/tty/serial/max310x.c:1494:36: note: in expansion of macro 'MAX310X_PORT_NAME_SUFFIX'
1494 | snprintf(buf, sizeof(buf), MAX310X_PORT_NAME_SUFFIX "%u", port_id);
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/max310x.c:1494:9: note: 'snprintf' output between 6 and 15 bytes into a destination of size 6
1494 | snprintf(buf, sizeof(buf), MAX310X_PORT_NAME_SUFFIX "%u", port_id);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'max310x_regmap_name',
inlined from 'max310x_spi_probe' at drivers/tty/serial/max310x.c:1535:17:
>> drivers/tty/serial/max310x.c:30:41: warning: '%u' directive output may be truncated writing between 1 and 10 bytes into a region of size 2 [-Wformat-truncation=]
30 | #define MAX310X_PORT_NAME_SUFFIX "port"
| ^~~~~~
drivers/tty/serial/max310x.c:1494:36: note: in expansion of macro 'MAX310X_PORT_NAME_SUFFIX'
1494 | snprintf(buf, sizeof(buf), MAX310X_PORT_NAME_SUFFIX "%u", port_id);
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/max310x.c: In function 'max310x_spi_probe':
drivers/tty/serial/max310x.c:1494:62: note: format string is defined here
1494 | snprintf(buf, sizeof(buf), MAX310X_PORT_NAME_SUFFIX "%u", port_id);
| ^~
In function 'max310x_regmap_name',
inlined from 'max310x_spi_probe' at drivers/tty/serial/max310x.c:1535:17:
drivers/tty/serial/max310x.c:30:41: note: directive argument in the range [0, 4294967294]
30 | #define MAX310X_PORT_NAME_SUFFIX "port"
| ^~~~~~
drivers/tty/serial/max310x.c:1494:36: note: in expansion of macro 'MAX310X_PORT_NAME_SUFFIX'
1494 | snprintf(buf, sizeof(buf), MAX310X_PORT_NAME_SUFFIX "%u", port_id);
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/tty/serial/max310x.c:1494:9: note: 'snprintf' output between 6 and 15 bytes into a destination of size 6
1494 | snprintf(buf, sizeof(buf), MAX310X_PORT_NAME_SUFFIX "%u", port_id);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +30 drivers/tty/serial/max310x.c
28
29 #define MAX310X_NAME "max310x"
> 30 #define MAX310X_PORT_NAME_SUFFIX "port"
31 #define MAX310X_MAJOR 204
32 #define MAX310X_MINOR 209
33 #define MAX310X_UART_NRMAX 16
34 #define MAX310X_MAX_PORTS 4 /* Maximum number of UART ports per IC. */
35
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 22+ messages in thread