Re: pnp_bus_resume(): inconsequent NULL checking

[Rene Herman <rene.herman@keyaccess.nl>]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

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.

Bjorn?

[-- Attachment #2: coverity-pnp_bus_suspend_resume.diff --]
[-- Type: text/plain, Size: 1681 bytes --]

pnp: be consistent about testing pnp_dev->protocol in pnp_bus_{suspend,resume}

pnp_can_{disable,write}() dereference pnp_dev->protocol. So do the
pnp_{stop,start}_dev() the tests protect, but let's be consistent
at least.

Spotted by Adrian Bunk <bunk@kernel.org> and the Coverity checker.

Signed-off-by: Rene Herman <rene.herman@gmail.com>

diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c
index 12a1645..170af61 100644
--- a/drivers/pnp/driver.c
+++ b/drivers/pnp/driver.c
@@ -160,15 +160,15 @@ static int pnp_bus_suspend(struct device *dev, pm_message_t state)
 		if (error)
 			return error;
 	}
-
-	if (pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
+	if (pnp_dev->protocol) {
+		if (pnp_can_disable(pnp_dev)) {
+			error = pnp_stop_dev(pnp_dev);
+			if (error)
+				return error;
+		}
+		if (pnp_dev->protocol->suspend)
+			pnp_dev->protocol->suspend(pnp_dev, state);
 	}
-
-	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
-		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;
 }
 
@@ -181,21 +181,21 @@ static int pnp_bus_resume(struct device *dev)
 	if (!pnp_drv)
 		return 0;
 
-	if (pnp_dev->protocol && pnp_dev->protocol->resume)
-		pnp_dev->protocol->resume(pnp_dev);
+	if (pnp_dev->protocol) {
+		if (pnp_dev->protocol->resume)
+			pnp_dev->protocol->resume(pnp_dev);
 
-	if (pnp_can_write(pnp_dev)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
+		if (pnp_can_write(pnp_dev)) {
+			error = pnp_start_dev(pnp_dev);
+			if (error)
+				return error;
+		}
 	}
-
 	if (pnp_drv->resume) {
 		error = pnp_drv->resume(pnp_dev);
 		if (error)
 			return error;
 	}
-
 	return 0;
 }