[smatch stuff] altera_uart: inconsistent checks for null

[smatch stuff] altera_uart: inconsistent checks for null

[Dan Carpenter]

Hi Tobias,

Patch 2780ad42f5fe67 "tty: serial: altera_uart: Use port->regshift to
store bus shift" added a NULL check for platp in altera_uart_probe() but
not consistently through out the function.

drivers/tty/serial/altera_uart.c +553 altera_uart_probe(43)
	error: we previously assumed 'platp' could be null.

   545          if (platp)
                   ^^^^^^^
checked here.

   546                  port->regshift = platp->bus_shift;
   547          else
   548                  port->regshift = 0;
   549
   550          port->line = i;
   551          port->type = PORT_ALTERA_UART;
   552          port->iotype = SERIAL_IO_MEM;
   553          port->uartclk = platp->uartclk;
                                ^^^^^^^
potential NULL dereference?

   554          port->ops = &altera_uart_ops;

I don't know the right way to address this.

regards,
dan carpenter

Re: [smatch stuff] altera_uart: inconsistent checks for null

[Tobias Klauser]

Hi Dan,

On 2011-02-19 at 10:31:07 +0100, Dan Carpenter <error27@gmail.com> wrote:
> Patch 2780ad42f5fe67 "tty: serial: altera_uart: Use port->regshift to
> store bus shift" added a NULL check for platp in altera_uart_probe() but
> not consistently through out the function.
> 
> drivers/tty/serial/altera_uart.c +553 altera_uart_probe(43)
> 	error: we previously assumed 'platp' could be null.
> 
>    545          if (platp)
>                    ^^^^^^^
> checked here.
> 
>    546                  port->regshift = platp->bus_shift;
>    547          else
>    548                  port->regshift = 0;
>    549
>    550          port->line = i;
>    551          port->type = PORT_ALTERA_UART;
>    552          port->iotype = SERIAL_IO_MEM;
>    553          port->uartclk = platp->uartclk;
>                                 ^^^^^^^
> potential NULL dereference?

Not at the moment, because everybody is using platform data at the
momenti, so platp should never be NULL. Still this is inconsistent and
shouldn't be there like this. The dereference on line 553 will go away
with the patch introducing device tree support to altera_uart [1].

[1] https://lkml.org/lkml/2011/2/18/100

>    554          port->ops = &altera_uart_ops;
> 
> I don't know the right way to address this.

I guess either Greg or Grant will take the above mentioned patch (it
depends on a commit in Grant's tree).

Or do you think we should fix this in another way until the patch is
merged?

Thanks a lot for pointing this out
Tobias

Re: [smatch stuff] altera_uart: inconsistent checks for null

[Dan Carpenter]

On Sat, Feb 19, 2011 at 02:22:17PM +0100, Tobias Klauser wrote:
> Hi Dan,
> 
> On 2011-02-19 at 10:31:07 +0100, Dan Carpenter <error27@gmail.com> wrote:
> > Patch 2780ad42f5fe67 "tty: serial: altera_uart: Use port->regshift to
> > store bus shift" added a NULL check for platp in altera_uart_probe() but
> > not consistently through out the function.
> > 
> > drivers/tty/serial/altera_uart.c +553 altera_uart_probe(43)
> > 	error: we previously assumed 'platp' could be null.
> > 
> >    545          if (platp)
> >                    ^^^^^^^
> > checked here.
> > 
> >    546                  port->regshift = platp->bus_shift;
> >    547          else
> >    548                  port->regshift = 0;
> >    549
> >    550          port->line = i;
> >    551          port->type = PORT_ALTERA_UART;
> >    552          port->iotype = SERIAL_IO_MEM;
> >    553          port->uartclk = platp->uartclk;
> >                                 ^^^^^^^
> > potential NULL dereference?
> 
> Not at the moment, because everybody is using platform data at the
> momenti, so platp should never be NULL. Still this is inconsistent and
> shouldn't be there like this. The dereference on line 553 will go away
> with the patch introducing device tree support to altera_uart [1].
> 
> [1] https://lkml.org/lkml/2011/2/18/100
> 
> >    554          port->ops = &altera_uart_ops;
> > 
> > I don't know the right way to address this.
> 
> I guess either Greg or Grant will take the above mentioned patch (it
> depends on a commit in Grant's tree).
> 

Sounds good.

regards,
dan carpenter

Re: [smatch stuff] altera_uart: inconsistent checks for null

[Tobias Klauser]

On 2011-02-19 at 15:02:05 +0100, Dan Carpenter <error27@gmail.com> wrote:
> On Sat, Feb 19, 2011 at 02:22:17PM +0100, Tobias Klauser wrote:
> > On 2011-02-19 at 10:31:07 +0100, Dan Carpenter <error27@gmail.com> wrote:
> > > Patch 2780ad42f5fe67 "tty: serial: altera_uart: Use port->regshift to
> > > store bus shift" added a NULL check for platp in altera_uart_probe() but
> > > not consistently through out the function.
> > > 
> > > drivers/tty/serial/altera_uart.c +553 altera_uart_probe(43)
> > > 	error: we previously assumed 'platp' could be null.
> > > 
> > >    545          if (platp)
> > >                    ^^^^^^^
> > > checked here.
> > > 
> > >    546                  port->regshift = platp->bus_shift;
> > >    547          else
> > >    548                  port->regshift = 0;
> > >    549
> > >    550          port->line = i;
> > >    551          port->type = PORT_ALTERA_UART;
> > >    552          port->iotype = SERIAL_IO_MEM;
> > >    553          port->uartclk = platp->uartclk;
> > >                                 ^^^^^^^
> > > potential NULL dereference?
> > 
> > Not at the moment, because everybody is using platform data at the
> > momenti, so platp should never be NULL. Still this is inconsistent and
> > shouldn't be there like this. The dereference on line 553 will go away
> > with the patch introducing device tree support to altera_uart [1].
> > 
> > [1] https://lkml.org/lkml/2011/2/18/100
> > 
> > >    554          port->ops = &altera_uart_ops;
> > > 
> > > I don't know the right way to address this.
> > 
> > I guess either Greg or Grant will take the above mentioned patch (it
> > depends on a commit in Grant's tree).

Grant has taken the patch into his tree and it is now in linux-next.
I ran smatch against it and the error went away.

Cheers
Tobias