net: moxa: fix an error code

We accidentally return IS_ERR(priv->base) which is 1 instead of PTR_ERR(priv->base) which is the error code.

index 00cfd95..3e67f45 100644
@@ -474,9 +474,9 @@ static int moxart_mac_probe(struct platform_device *pdev)
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	ndev->base_addr = res->start;
 	priv->base = devm_ioremap_resource(p_dev, res);
-	ret = IS_ERR(priv->base);
-	if (ret) {
+	if (IS_ERR(priv->base)) {
 		dev_err(p_dev, "devm_ioremap_resource failed\n");
+		ret = PTR_ERR(priv->base);
 		goto init_fail;
 	}
 

nice catch!

One or two. I already sent the fixes. I think it was applied.

Huh? I sent one where I could have done that but I deliberately didn't because I wanted the uninitialized warning if I made a mistake. It sounds like you're working around a GCC bug...

regards,
dan carpenter

The uninitialized warning here is about a type mismatch preventing gcc from noticing that two conditions are the same, I'm not sure if this is a bug in gcc, or required by the C standard.

I don't think there is a way in which you would hide a correct warning about an uninitialized warning.

Have a look at
https://git.kernel.org/cgit/linux/kernel/git/gfs2/linux-gfs2.git/commit/fs/gfs2?h=for-next&idcfdc3071432a07713e4d007c2811e0224490b0

in which get_leaf_nr() uses the IS_ERR()/PTR_ERR() combo to return an error from a pointer, or return success when the pointer was set, followed by a warning about the use of the pointer in another function. My original patch avoided the warning by using IS_ERR_VALUE() in the caller, but in retrospect, IS_ERR_OR_ZERO() would have been a nicer solution:

@@ -783,12 +783,15 @@ static int get_leaf_nr(struct gfs2_inode *dip, u32 index,

                       u64 *leaf_out)
 {
        __be64 *hash;

+ int error;

        hash = gfs2_dir_get_hash_table(dip);
-       if (IS_ERR(hash))
-               return PTR_ERR(hash);
-       *leaf_out = be64_to_cpu(*(hash + index));
-       return 0;
+       error = PTR_ERR_OR_ZERO(hash);

+
+ if (!error)
+ *leaf_out = be64_to_cpu(*(hash + index));
+
+ return error;
}

and I've used that elsewhere now when I ran into this kind of false positive warning.

Arnd

I wouldn't call it a bug, because everyone has to make trade offs between how fast the program runs and how accurate it is. And trade offs between how ambitious your warnings are vs how many false positives you can tolerate.

Anyway, I feel like we should just work around GCC on a case by case basis instead of always using PTR_ERR_OR_ZERO(). The next version of GCC will fix some false positives and introduce new ones... Next time using PTR_ERR_OR_ZERO() could cause warnings instead of fixing them.

Smatch works in a different way and it parse the code correctly. But Smatch is slow and sometimes runs out of memory and gives up trying to parse large functions. Smatch sees the two returns and tries to figure out the implications of each (uninitialized vs initialized). If you change the code to:

	error = PTR_ERR_OR_ZERO(hash);

	if (!error)
		*leaf_out = be64_to_cpu(*(hash + index));

	return error;

then Smatch still breaks that up into two separate returns which imply initialized vs uninitialized.

regards,
dan carpenter

It depends on whether we think the PTR_ERR_OR_ZERO() actually makes the code more readable too. I've actually come to like it now, the main downside being that it looks a lot like IS_ERR_OR_NULL() which is very bad style and should be avoided at all cost. ;-)

I can also see how PTR_ERR_OR_ZERO() is easier for the compiler to understand than IS_ERR()+PTR_ERR() and can't think of a case where it would result in worse object code or extra (false positive) warnings.

Right, so it does the right thing, and it presumably understands that 'if (error)' is the same as 'if (error < 0)' and 'if (IS_ERR_VALUE(error)', right? I think that is where gcc gets it wrong.

Arnd

From: Dan Carpenter <[email protected]>
Date: Wed, 2 Mar 2016 13:11:10 +0300

Applied, thank you Dan.