keyspan_pda.c use of keyspan_pda_get_modem_info

keyspan_pda.c use of keyspan_pda_get_modem_info

[Benny Halevy]

with gcc 4.3.0 i see these warnings:

drivers/usb/serial/keyspan_pda.c: In function 'keyspan_pda_tiocmget':
drivers/usb/serial/keyspan_pda.c:444: warning: 'status' may be used uninitialized in this function
drivers/usb/serial/keyspan_pda.c: In function 'keyspan_pda_tiocmset':
drivers/usb/serial/keyspan_pda.c:465: warning: 'status' may be used uninitialized in this function

In these two call sites the callers bail out if
keyspan_pda_get_modem_info return value is < 0
e.g.
static int keyspan_pda_tiocmget(struct usb_serial_port *port, struct file *file)
{
...
	unsigned char status;
...
	rc = keyspan_pda_get_modem_info(serial, &status);
	if (rc < 0)
		return rc;
	value =
		((status & (1<<7)) ? TIOCM_DTR : 0) |
...
	return value;

However, keyspan_pda_get_modem_info sets status only for rc > 0
so it may indeed be used uninitialized in case keyspan_pda_get_modem_info
returns 0.

static int keyspan_pda_get_modem_info(struct usb_serial *serial,
				      unsigned char *value)
{
	int rc;
	unsigned char data;
	rc = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
			     3, /* get pins */
			     USB_TYPE_VENDOR|USB_RECIP_INTERFACE|USB_DIR_IN,
			     0, 0, &data, 1, 2000);
	if (rc > 0)
		*value = data;
	return rc;
}

In the usb_control_msg/usb_internal_control_msg/usb_start_wait_urb path,
if usb_submit_urb

That said, I'm not sure if that can happen at all but regardless,
it seems like a good idea to handle this case.

Benny

Re: keyspan_pda.c use of keyspan_pda_get_modem_info

[Alan Cox]

> In these two call sites the callers bail out if
> keyspan_pda_get_modem_info return value is < 0

I don't think that is what the compiler is warning about.

We error on rc < 0
We use the returned status on rc >= 0

We set the returned status on rc > 0

So the rc = 0 case is broken and gcc seems to be correct about that

Re: keyspan_pda.c use of keyspan_pda_get_modem_info

[Greg KH]

On Thu, Jun 26, 2008 at 04:23:46PM +0100, Alan Cox wrote:
> > In these two call sites the callers bail out if
> > keyspan_pda_get_modem_info return value is < 0
> 
> I don't think that is what the compiler is warning about.
> 
> We error on rc < 0
> We use the returned status on rc >= 0
> 
> We set the returned status on rc > 0
> 
> So the rc = 0 case is broken and gcc seems to be correct about that

Yes, that is correct, fortunatly that function can never return rc = 0,
so this will not happen in real life.

But there is really no way the compiler can ever figure that out, so I
don't blame it for complaining.

If you want to make a simple patch to make the compiler happy and be
quiet about the warning, I'll take it.

thanks,

greg k-h

Re: keyspan_pda.c use of keyspan_pda_get_modem_info

[Benny Halevy]

On Jun. 27, 2008, 7:17 +0300, Greg KH <greg@kroah.com> wrote:
> On Thu, Jun 26, 2008 at 04:23:46PM +0100, Alan Cox wrote:
>>> In these two call sites the callers bail out if
>>> keyspan_pda_get_modem_info return value is < 0
>> I don't think that is what the compiler is warning about.
>>
>> We error on rc < 0
>> We use the returned status on rc >= 0
>>
>> We set the returned status on rc > 0
>>
>> So the rc = 0 case is broken and gcc seems to be correct about that
> 
> Yes, that is correct, fortunatly that function can never return rc = 0,
> so this will not happen in real life.
> 
> But there is really no way the compiler can ever figure that out, so I
> don't blame it for complaining.
> 
> If you want to make a simple patch to make the compiler happy and be
> quiet about the warning, I'll take it.

Cool. I'll send a patch initializing status to 0.
Now, what about the rc == 0 case?
Does it warrant a BUG_ON() if it should ever happen?

Benny

> 
> thanks,
> 
> greg k-h

[PATCH] usb: fix uninitialized variables in keyspan_pda

[Benny Halevy]

The compiler (gcc 4.3.0) warns that status might be used uninitialized
in keyspan_pda_tiocmget and keyspan_pda_tiocmset.

It is technically correct and therefore this patch initializes
status to 0 in both cases.

Note that keyspan_pda_get_modem_info sets *value only when
usb_control_msg returns rc > 0, otherwise it must
return an error rc < 0, and it must never return rc == 0, hence
a WARN_ON(rc == 0) was added in case usb_control_msg ever return 0.

Both its callers bail out for rc < 0 and use the value
of "status" in the rc >= 0 case.  If the WARN_ON is ever seen,
status will be set to the initial value of 0.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 drivers/usb/serial/keyspan_pda.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/keyspan_pda.c b/drivers/usb/serial/keyspan_pda.c
index ff54203..97ce8a3 100644
--- a/drivers/usb/serial/keyspan_pda.c
+++ b/drivers/usb/serial/keyspan_pda.c
@@ -420,6 +420,7 @@ static int keyspan_pda_get_modem_info(struct usb_serial *serial,
 			     3, /* get pins */
 			     USB_TYPE_VENDOR|USB_RECIP_INTERFACE|USB_DIR_IN,
 			     0, 0, &data, 1, 2000);
+	WARN_ON(rc == 0);
 	if (rc > 0)
 		*value = data;
 	return rc;
@@ -441,7 +442,7 @@ static int keyspan_pda_tiocmget(struct usb_serial_port *port, struct file *file)
 {
 	struct usb_serial *serial = port->serial;
 	int rc;
-	unsigned char status;
+	unsigned char status = 0;
 	int value;
 
 	rc = keyspan_pda_get_modem_info(serial, &status);
@@ -462,7 +463,7 @@ static int keyspan_pda_tiocmset(struct usb_serial_port *port, struct file *file,
 {
 	struct usb_serial *serial = port->serial;
 	int rc;
-	unsigned char status;
+	unsigned char status = 0;
 
 	rc = keyspan_pda_get_modem_info(serial, &status);
 	if (rc < 0)
-- 
1.5.6.GIT

Re: [PATCH] usb: fix uninitialized variables in keyspan_pda

[Alan Cox]

On Fri, 27 Jun 2008 10:58:54 +0300
Benny Halevy <bhalevy@panasas.com> wrote:

> The compiler (gcc 4.3.0) warns that status might be used uninitialized
> in keyspan_pda_tiocmget and keyspan_pda_tiocmset.
> 
> It is technically correct and therefore this patch initializes
> status to 0 in both cases.
> 
> Note that keyspan_pda_get_modem_info sets *value only when
> usb_control_msg returns rc > 0, otherwise it must
> return an error rc < 0, and it must never return rc == 0, hence
> a WARN_ON(rc == 0) was added in case usb_control_msg ever return 0.

That seems like overkill - and by forcing status to 0 you will hide
future gcc error catches. Far better to change the two callers to test
for > 0 not >= 0 surely ?

Alan

Re: [PATCH] usb: fix uninitialized variables in keyspan_pda

[Benny Halevy]

On Jun. 27, 2008, 11:58 +0300, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Fri, 27 Jun 2008 10:58:54 +0300
> Benny Halevy <bhalevy@panasas.com> wrote:
> 
>> The compiler (gcc 4.3.0) warns that status might be used uninitialized
>> in keyspan_pda_tiocmget and keyspan_pda_tiocmset.
>>
>> It is technically correct and therefore this patch initializes
>> status to 0 in both cases.
>>
>> Note that keyspan_pda_get_modem_info sets *value only when
>> usb_control_msg returns rc > 0, otherwise it must
>> return an error rc < 0, and it must never return rc == 0, hence
>> a WARN_ON(rc == 0) was added in case usb_control_msg ever return 0.
> 
> That seems like overkill - and by forcing status to 0 you will hide
> future gcc error catches. Far better to change the two callers to test
> for > 0 not >= 0 surely ?

Well, since usb_control_msg is not supposed to ever return 0
I'm really not sure what to do if it does. I'll gladly defer that
decision to Greg...

The following patch makes gcc happy as you said and if it's
correct I, too, like it much better.

Benny

diff --git a/drivers/usb/serial/keyspan_pda.c b/drivers/usb/serial/keyspan_pda.c
index ff54203..197d283 100644
--- a/drivers/usb/serial/keyspan_pda.c
+++ b/drivers/usb/serial/keyspan_pda.c
@@ -420,7 +420,7 @@ static int keyspan_pda_get_modem_info(struct usb_serial *serial,
                             3, /* get pins */
                             USB_TYPE_VENDOR|USB_RECIP_INTERFACE|USB_DIR_IN,
                             0, 0, &data, 1, 2000);
-       if (rc > 0)
+       if (rc >= 0)
                *value = data;
        return rc;
 }

> 
> Alan

Re: [PATCH] usb: fix uninitialized variables in keyspan_pda

[Greg KH]

On Fri, Jun 27, 2008 at 12:22:32PM +0300, Benny Halevy wrote:
> On Jun. 27, 2008, 11:58 +0300, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > On Fri, 27 Jun 2008 10:58:54 +0300
> > Benny Halevy <bhalevy@panasas.com> wrote:
> > 
> >> The compiler (gcc 4.3.0) warns that status might be used uninitialized
> >> in keyspan_pda_tiocmget and keyspan_pda_tiocmset.
> >>
> >> It is technically correct and therefore this patch initializes
> >> status to 0 in both cases.
> >>
> >> Note that keyspan_pda_get_modem_info sets *value only when
> >> usb_control_msg returns rc > 0, otherwise it must
> >> return an error rc < 0, and it must never return rc == 0, hence
> >> a WARN_ON(rc == 0) was added in case usb_control_msg ever return 0.
> > 
> > That seems like overkill - and by forcing status to 0 you will hide
> > future gcc error catches. Far better to change the two callers to test
> > for > 0 not >= 0 surely ?
> 
> Well, since usb_control_msg is not supposed to ever return 0
> I'm really not sure what to do if it does. I'll gladly defer that
> decision to Greg...

It's not supposed to return 0 unless you pass in 0 as the length to be
transferred (not likely, but possible.)  So I'm referring to the case in
the keyspan driver where the return value can not be zero, sorry for any
confusion.

> The following patch makes gcc happy as you said and if it's
> correct I, too, like it much better.

I like it better too, make it more obvious.

thanks,

greg k-h