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.

Fixes: 6c821bd9edc9 ('net: Add MOXA ART SoCs ethernet driver')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

On Wednesday 02 March 2016 13:11:10 Dan Carpenter wrote:
> We accidentally return IS_ERR(priv->base) which is 1 instead of
> PTR_ERR(priv->base) which is the error code.
> 
> Fixes: 6c821bd9edc9 ('net: Add MOXA ART SoCs ethernet driver')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

nice catch!

> diff --git a/drivers/net/ethernet/moxa/moxart_ether.c b/drivers/net/ethernet/moxa/moxart_ether.c
> index 00cfd95..3e67f45 100644
> --- a/drivers/net/ethernet/moxa/moxart_ether.c
> +++ b/drivers/net/ethernet/moxa/moxart_ether.c
> @@ -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;
>  	}

Did you find more of these?

it doesn't matter much either way, but if you do multiple such patches,
I'd suggest using a single PTR_ERR_OR_ZERO() instead of IS_ERR()+PTR_ERR().

I have found a couple of drivers in which that leads to better object
code, and avoids a warning about a possibly uninitialized variable
when the function gets inlined into another one (which won't happen
for this driver).

	Arnd

On Wed, Mar 02, 2016 at 11:52:29AM +0100, Arnd Bergmann wrote:
> Did you find more of these?
> 
> it doesn't matter much either way, but if you do multiple such patches,

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

> I'd suggest using a single PTR_ERR_OR_ZERO() instead of IS_ERR()+PTR_ERR().
> 
> I have found a couple of drivers in which that leads to better object
> code, and avoids a warning about a possibly uninitialized variable
> when the function gets inlined into another one (which won't happen
> for this driver).

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

On Wednesday 02 March 2016 14:21:29 Dan Carpenter wrote:
> On Wed, Mar 02, 2016 at 11:52:29AM +0100, Arnd Bergmann wrote:
> > Did you find more of these?
> > 
> > it doesn't matter much either way, but if you do multiple such patches,
> 
> One or two.  I already sent the fixes.  I think it was applied.
> 
> > I'd suggest using a single PTR_ERR_OR_ZERO() instead of IS_ERR()+PTR_ERR().
> > 
> > I have found a couple of drivers in which that leads to better object
> > code, and avoids a warning about a possibly uninitialized variable
> > when the function gets inlined into another one (which won't happen
> > for this driver).
> 
> 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...

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&id=07cfdc3071432a07713e4d007c2811e0224490b0

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

On Wed, Mar 02, 2016 at 12:36:05PM +0100, Arnd Bergmann wrote:
> 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 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

On Wednesday 02 March 2016 15:15:26 Dan Carpenter wrote:
> On Wed, Mar 02, 2016 at 12:36:05PM +0100, Arnd Bergmann wrote:
> > 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 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.

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.

> 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.

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 <dan.carpenter@oracle.com>
Date: Wed, 2 Mar 2016 13:11:10 +0300

> We accidentally return IS_ERR(priv->base) which is 1 instead of
> PTR_ERR(priv->base) which is the error code.
> 
> Fixes: 6c821bd9edc9 ('net: Add MOXA ART SoCs ethernet driver')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thank you Dan.