On Wed, 2023-09-13 at 20:38 +0300, Dan Carpenter wrote:
> On Wed, Sep 13, 2023 at 04:43:29PM +0200, Bastien Nocera wrote:
> > Hey Dan,
> >
> > On Thu, 2023-09-07 at 12:55 +0300, Dan Carpenter wrote:
> > > The hid_hw_raw_request() function returns negative error codes or
> > > the
> > > number bytes transferred. If it returns negative error codes,
> > > then
> > > the
> > > problem is that when we check if "ret <
> > > sizeof(arctis_1_battery_request)",
> > > negative values are type promoted to high unsigned values and
> > > treated
> > > as
> > > success. Add an explicit check for negatives to address this
> > > issue.
> >
> > I would be grateful if you could either add the compiler's error
> > message or explain in greater details the integer type promotion
> > (or
> > demotion from signed to unsigned in this case) in the commit
> > message.
> >
>
> This is a Smatch warning but you have to have built the cross
> function
> database and I only know one other person who does that for sure...
>
> drivers/hid/hid-steelseries.c:393
> steelseries_headset_arctis_1_fetch_battery()
> warn: error code type promoted to positive: 'ret'
>
> I can add that and resend.
That would be great, thanks!
>
> > > Fixes: a0c76896c3fb ("HID: steelseries: Add support for Arctis 1
> > > XBox")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> >
> >
> >
> > > ---
> > > drivers/hid/hid-steelseries.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
> > > steelseries.c
> > > index 43d2cf7153d7..485d2287d58a 100644
> > > --- a/drivers/hid/hid-steelseries.c
> > > +++ b/drivers/hid/hid-steelseries.c
> > > @@ -390,7 +390,7 @@ static int
> > > steelseries_headset_arctis_1_fetch_battery(struct hid_device
> > > *hdev)
> > > ret = hid_hw_raw_request(hdev,
> > > arctis_1_battery_request[0],
> > > write_buf,
> > > sizeof(arctis_1_battery_request),
> > > HID_OUTPUT_REPORT,
> > > HID_REQ_SET_REPORT);
> > > - if (ret < sizeof(arctis_1_battery_request)) {
> > > + if (ret < 0 || ret < sizeof(arctis_1_battery_request)) {
> >
> > I prefer:
> > - if (ret < sizeof(arctis_1_battery_request)) {
> > + if (ret < (int) sizeof(arctis_1_battery_request)) {
> >
> > although I'm not sure that's the kernel-style for this sort of
> > checks.
> >
>
> I grepped to see which format is more popular and it's 14 vs 13 in
> favor
> of casting. I prefer to avoid casts, but I can resend.
>
> regards,
> dan carpenter
>
>
