usb/serial/io_ti.c: inconsequent NULL checking

usb/serial/io_ti.c: inconsequent NULL checking

[Adrian Bunk]

The Coverity checker spotted the following inconsequent NULL checking 
introduced by commit d5f5bcd425b771c0b7ff5a650b2ce061ac8bbb87:

<--  snip  -->

static int edge_open (struct usb_serial_port *port, struct file * filp)
{
...
        if (port->tty)   <---------------------------------------------
                port->tty->low_latency = low_latency;

        port_number = port->number - port->serial->minor;
        switch (port_number) {
                case 0:
                        edge_port->uart_base = UMPMEM_BASE_UART1;
                        edge_port->dma_address = UMPD_OEDB1_ADDRESS;
                        break;
                case 1:
                        edge_port->uart_base = UMPMEM_BASE_UART2;
                        edge_port->dma_address = UMPD_OEDB2_ADDRESS;
                        break;
                default:
                        dev_err (&port->dev, "Unknown port number!!!\n");
                        return -ENODEV;
        }

        dbg ("%s - port_number = %d, uart_base = %04x, dma_address = %04x",
             __FUNCTION__, port_number, edge_port->uart_base, edge_port->dma_address);

        dev = port->serial->dev;

        memset (&(edge_port->icount), 0x00, sizeof(edge_port->icount));
        init_waitqueue_head (&edge_port->delta_msr_wait);

        /* turn off loopback */
        status = TIClearLoopBack (edge_port);
        if (status) {
                dev_err(&port->dev,"%s - cannot send clear loopback command, %d\n",
                        __FUNCTION__, status);
                return status;
        }
        
        /* set up the port settings */
        edge_set_termios (port, port->tty->termios);
...                             ^^^^^^^^^^^^^^^^^^

<--  snip  -->

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: usb/serial/io_ti.c: inconsequent NULL checking

[Greg KH]

On Wed, Feb 20, 2008 at 12:49:15AM +0200, Adrian Bunk wrote:
> The Coverity checker spotted the following inconsequent NULL checking 
> introduced by commit d5f5bcd425b771c0b7ff5a650b2ce061ac8bbb87:
> 
> <--  snip  -->

It's not a real problem, that pointer can't go away.

thanks,

greg k-h

Re: usb/serial/io_ti.c: inconsequent NULL checking

[Ray Lee]

On Feb 19, 2008 3:25 PM, Greg KH <gregkh@suse.de> wrote:
> On Wed, Feb 20, 2008 at 12:49:15AM +0200, Adrian Bunk wrote:
> > The Coverity checker spotted the following inconsequent NULL checking
> > introduced by commit d5f5bcd425b771c0b7ff5a650b2ce061ac8bbb87:
> >
> > <--  snip  -->
>
> It's not a real problem, that pointer can't go away.

I think he meant inconsistent not "inconsequent."

Either the test of port->tty here is unneeded:

 if (port->tty)
               port->tty->low_latency = low_latency;

...or the lack of test of port->tty here is a mistake:

	 edge_set_termios (port, port->tty->termios);

Re: usb/serial/io_ti.c: inconsequent NULL checking

[Alan Cox]

> Either the test of port->tty here is unneeded:
> 
>  if (port->tty)
>                port->tty->low_latency = low_latency;
> 
> ...or the lack of test of port->tty here is a mistake:
> 
> 	 edge_set_termios (port, port->tty->termios);

Interesting - so coverity doesn't understand the BKL. It's producing the
right answer, for the right reason but the assumption it makes isn't 100%
safe.

The check can go.

Alan

[2.6 patch] usb/serial/io_ti.c: remove unneeded NULL check

[Adrian Bunk]

On Fri, Feb 22, 2008 at 10:10:16AM +0000, Alan Cox wrote:
> > Either the test of port->tty here is unneeded:
> > 
> >  if (port->tty)
> >                port->tty->low_latency = low_latency;
> > 
> > ...or the lack of test of port->tty here is a mistake:
> > 
> > 	 edge_set_termios (port, port->tty->termios);
> 
> Interesting - so coverity doesn't understand the BKL. It's producing the
> right answer, for the right reason but the assumption it makes isn't 100%
> safe.

Coverity doesn't perform magic.
It simply notices that it's once checked and once dereferenced 
unconditionally.

> The check can go.

Thanks, patch below.

> Alan

cu
Adrian


<--  snip  -->


There's no reason for checking pdev->bus for being NULL here (and we'd 
anyway Oops below if it was).

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---
afefd6935c788b3e4071092ebc04c4d356dd7496 diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index cd34059..700683c 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -1944,8 +1944,7 @@ static int edge_open (struct usb_serial_port *port, struct file * filp)
 	if (edge_port == NULL)
 		return -ENODEV;
 
-	if (port->tty)
-		port->tty->low_latency = low_latency;
+	port->tty->low_latency = low_latency;
 
 	port_number = port->number - port->serial->minor;
 	switch (port_number) {

Re: [2.6 patch] usb/serial/io_ti.c: remove unneeded NULL check

[Alan Cox]

Already patched in my tree