mbox series

[v1,0/4] Input: Increase size of phys in the drivers

Message ID 20250228121147.242115-1-andriy.shevchenko@linux.intel.com (mailing list archive)
Headers show
Series Input: Increase size of phys in the drivers | expand

Message

Andy Shevchenko Feb. 28, 2025, 12:07 p.m. UTC
The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
characters. GCC complains on that. This series fixes the issue in the affected
input drivers. Note, this is currently the biggest part of the warnings that
are being treated as errors with the default configurations on x86. With this
being applied we become quite close to enable CONFIG_WERROR=y (which is default
and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.

Andy Shevchenko (4):
  Input: ALPS - increase size of phys2 and phys3
  Input: atkbd - increase size of phys
  Input: lifebook - increase size of phys
  Input: psmouse - increase size of phys

 drivers/input/keyboard/atkbd.c | 2 +-
 drivers/input/mouse/alps.h     | 4 ++--
 drivers/input/mouse/lifebook.c | 2 +-
 drivers/input/mouse/psmouse.h  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

Comments

Andy Shevchenko Feb. 28, 2025, 12:52 p.m. UTC | #1
On Fri, Feb 28, 2025 at 02:07:43PM +0200, Andy Shevchenko wrote:
> The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
> characters. GCC complains on that. This series fixes the issue in the affected
> input drivers. Note, this is currently the biggest part of the warnings that
> are being treated as errors with the default configurations on x86. With this
> being applied we become quite close to enable CONFIG_WERROR=y (which is default
> and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.

If acceptable, it would be good to have them as fixes for v6.14 cycle.
Andy Shevchenko March 4, 2025, 12:14 p.m. UTC | #2
On Fri, Feb 28, 2025 at 02:07:43PM +0200, Andy Shevchenko wrote:
> The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
> characters. GCC complains on that. This series fixes the issue in the affected
> input drivers. Note, this is currently the biggest part of the warnings that
> are being treated as errors with the default configurations on x86. With this
> being applied we become quite close to enable CONFIG_WERROR=y (which is default
> and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.

Would be nice to have a comment on this rather sooner as this impacts
the compilation by `make W=1` with WERROR=y (which is default).
Dmitry Torokhov March 5, 2025, 7:18 a.m. UTC | #3
Hi Andy,

On Tue, Mar 04, 2025 at 02:14:12PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 28, 2025 at 02:07:43PM +0200, Andy Shevchenko wrote:
> > The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
> > characters. GCC complains on that. This series fixes the issue in the affected
> > input drivers. Note, this is currently the biggest part of the warnings that
> > are being treated as errors with the default configurations on x86. With this
> > being applied we become quite close to enable CONFIG_WERROR=y (which is default
> > and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.
> 
> Would be nice to have a comment on this rather sooner as this impacts
> the compilation by `make W=1` with WERROR=y (which is default).

I do not like the change. There are no bugs, only GCC being paranoid.
Are there any other ways to shut it up? In [1] Jeff says that switching
to scnprintf() shuts GCC up...

[1] https://lore.kernel.org/r/Z3rIvp0hzS+yzvJA@nixie71

Thanks.
Andy Shevchenko March 5, 2025, 10:06 a.m. UTC | #4
On Tue, Mar 04, 2025 at 11:18:52PM -0800, Dmitry Torokhov wrote:
> On Tue, Mar 04, 2025 at 02:14:12PM +0200, Andy Shevchenko wrote:
> > On Fri, Feb 28, 2025 at 02:07:43PM +0200, Andy Shevchenko wrote:
> > > The drivers are using local member of 32 bytes to hold up to 40 (one-byte)
> > > characters. GCC complains on that. This series fixes the issue in the affected
> > > input drivers. Note, this is currently the biggest part of the warnings that
> > > are being treated as errors with the default configurations on x86. With this
> > > being applied we become quite close to enable CONFIG_WERROR=y (which is default
> > > and basically reverted) in CIs. Clang, OTOH, has currently no issues with that.
> > 
> > Would be nice to have a comment on this rather sooner as this impacts
> > the compilation by `make W=1` with WERROR=y (which is default).
> 
> I do not like the change.

Independently on your opinion in this case GCC is correct.
We are trying to squeeze up to 40 bytes into 32-byte storage.
I.o.w. GCC can't prove that and reader of the code can't prove
that either.

> There are no bugs, only GCC being paranoid.

I'm not so sure. But probably it works because the user space is parsing full
"inputX" string in the names

> Are there any other ways to shut it up? In [1] Jeff says that switching
> to scnprintf() shuts GCC up...

I do not like this, because it's just a hiding the problem and not solving it.
At some point GCC may start issuing warning on those cases as well when it
realizes the above. If you like that solution, please fix in that way. We have
4 drivers break the compilation currently.

> [1] https://lore.kernel.org/r/Z3rIvp0hzS+yzvJA@nixie71

So, consider this series as a bug report that prevents compilation.
I would expect somebody to fix this rather sooner than later.