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
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
On Feb 19, 2008 3:25 PM, Greg KH <[email protected]> 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);
> 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
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 <[email protected]>
---
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) {