Re: [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> > 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





[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux