On Wed, May 22, 2024 at 10:23 AM Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
>
> On Mon, May 20, 2024 at 08:55:30PM -0700, Ping Cheng wrote:
> > On Mon, May 20, 2024 at 4:28 PM Dmitry Torokhov
> > <dmitry.torokhov@xxxxxxxxx> wrote:
> >
> > Hi Dmitry,
> >
> > > > This fix is the same as [1]. Without checking the return value,
> > > > Wolfram's patch [2] fails our downstream build script. I'm adding my
> > > > r-b, if it makes any difference ;).
> > >
> > > Could you please tell me how exactly it makes your build script to fail?
> >
> > We got an "unused-result warning". Jason made a temporary workaround
> > at https://github.com/linuxwacom/input-wacom/commit/e83a9bb3e48d2d1b52ec709e60f73b9960d568e5.
>
> I do not think strscpy is declared as __must_check. Do you have a repro
> for the vanilla kernel build?
>
There is no build error with the upstream code. The build error occurs
when compiling an updated version of wacom_w8001.c on older (pre-4.16)
kernels. These older kernels did declare strscpy as __must_check.
We maintain backports of the upstream sources for users who are stuck
running old kernels. To ease the maintenance burden, we desire to keep
the upstream/downstream diff as small as possible. We understand that
not every upstream change is compatible with older kernels and will
introduce changes where necessary. In this particular situation we
believed that addressing the warning upstream made more sense than
maintaining a diff. It isn't wrong for upstream to verify return
values are sane, even if we admittedly do expect it to be a bit
unnecessary here.
> >
> > > My concern is that the warnings are not actionable and therefore pretty
> > > much worthless.
> >
IMO, the vast majority of the worth of this patch is a minimized
upstream / downstream diff -- not the warning itself. I know that this
means that such a patch is "not your problem", but it doesn't mean
that it is "nobody's" problem. If you don't like the idea of
introducing non-actionable warnings, would you be open to a small
cleanup patch instead (see attached as an example)? There's no
particularly good reason to split string generation across calls to
strscpy and strlcat when a single call to snprintf would do, and
snprintf is not __must_check on any of the kernels we backport to.
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
> > The return value tells us which strscpy call(s) didn't have enough space.
>
> Right, and what can be done about it? The driver does not control what
> tty is being used for the serial port (and so can't control the prefix
> of the 'phys'), we do not extend 'phys' (because it is used very
> rarely). The only option is to truncate (which we already do).
>
> So the warnings are not actionable.
>
> Thanks.
>
> --
> Dmitry
From 3b13656819a2c5dbf2a1c1f8fb407595eb3e5d94 Mon Sep 17 00:00:00 2001
From: Jason Gerecke <jason.gerecke@xxxxxxxxx>
Date: Thu, 23 May 2024 08:58:56 -0700
Subject: [PATCH] input: wacom_w8001: Simplify device name generation
Replace pairs of strscpy/strlcat calls with snprintf.
Signed-off-by: Jason Gerecke <jason.gerecke@xxxxxxxxx>
---
drivers/input/touchscreen/wacom_w8001.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index 928c5ee3ac36c..e3767db8a664b 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -595,7 +595,7 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
struct w8001 *w8001;
struct input_dev *input_dev_pen;
struct input_dev *input_dev_touch;
- char basename[64];
+ char basename[64] = "Wacom Serial";
int err, err_pen, err_touch;
w8001 = kzalloc(sizeof(struct w8001), GFP_KERNEL);
@@ -625,8 +625,6 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
/* For backwards-compatibility we compose the basename based on
* capabilities and then just append the tool type
*/
- strscpy(basename, "Wacom Serial", sizeof(basename));
-
err_pen = w8001_setup_pen(w8001, basename, sizeof(basename));
err_touch = w8001_setup_touch(w8001, basename, sizeof(basename));
if (err_pen && err_touch) {
@@ -635,8 +633,7 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
}
if (!err_pen) {
- strscpy(w8001->pen_name, basename, sizeof(w8001->pen_name));
- strlcat(w8001->pen_name, " Pen", sizeof(w8001->pen_name));
+ snprintf(w8001->pen_name, sizeof(w8001->pen_name), "%s Pen", basename);
input_dev_pen->name = w8001->pen_name;
w8001_set_devdata(input_dev_pen, w8001, serio);
@@ -651,9 +648,7 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
}
if (!err_touch) {
- strscpy(w8001->touch_name, basename, sizeof(w8001->touch_name));
- strlcat(w8001->touch_name, " Finger",
- sizeof(w8001->touch_name));
+ snprintf(w8001->pen_name, sizeof(w8001->pen_name), "%s Finger", basename);
input_dev_touch->name = w8001->touch_name;
w8001_set_devdata(input_dev_touch, w8001, serio);
--
2.45.1
