Re: pnp_bus_resume(): inconsequent NULL checking

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

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)) {