Re: pnp_bus_resume(): inconsequent NULL checking

Bjorn Helgaas
2008-02-20T17:05:56Z

On Tuesday 19 February 2008 05:00:43 pm Rene Herman wrote:
> On 19-02-08 23:49, Adrian Bunk wrote:
>
> > The Coverity checker spotted the following inconsequent NULL checking
> > introduced by commit 5d38998ed15b31f524bde9a193d60150af30d916:
> >
> > <-- snip -->
> >
> > ...
> > static int pnp_bus_resume(struct device *dev)
> > {
> > ...
> > if (pnp_dev->protocol && pnp_dev->protocol->resume)
> > pnp_dev->protocol->resume(pnp_dev);
> >
> > if (pnp_can_write(pnp_dev)) {
> > ...
> >
> > <-- snip -->
> >
> > pnp_can_write(pnp_dev) dereferences pnp_dev->protocol.
>
> I see, thanks. pnp_bus_suspend() has the same problem (I added this test to
> complete the mirror in fact) and/but is not a real problem since the tests
> are also the first things done inside the blocks they protect -- if
> pnp_dev->protocol isn't set here, we're dead anyway therefore.
>
> That probably means we can just delete the pnp_dev->protocol tests but this
> would need an ack from for example Bjorn Helgaas who might have an idea
> about how generically useful this is designed to be. The no brain thing to
> do would be just as per attached.

I agree with you that we can just delete the dev->protocol tests
completely. So I'd rather see something like this (built but untested):

PNP: remove dev->protocol NULL checks

Every PNP device should have a valid protocol pointer. If it doesn't,
something's wrong and we should oops so we can find and fix the problem.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work6/drivers/pnp/driver.c
===================================================================
--- work6.orig/drivers/pnp/driver.c 2008-02-20 09:46:01.000000000 -0700
+++ work6/drivers/pnp/driver.c 2008-02-20 09:46:28.000000000 -0700
@@ -167,7 +167,7 @@
return error;
}

- if (pnp_dev->protocol && pnp_dev->protocol->suspend)
+ if (pnp_dev->protocol->suspend)
pnp_dev->protocol->suspend(pnp_dev, state);
return 0;
}
@@ -181,7 +181,7 @@
if (!pnp_drv)
return 0;

- if (pnp_dev->protocol && pnp_dev->protocol->resume)
+ if (pnp_dev->protocol->resume)
pnp_dev->protocol->resume(pnp_dev);

if (pnp_can_write(pnp_dev)) {