[PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Dan Carpenter]

I just noticed this in review.  The get_register_interruptible() should
return zero on success but it instead returns the value that it read.

I looked at all the places that called this directly and they check for
negatives and treat greater than or equal to zero as success.  This
function is also called as the ->get_register() function pointer.  Some
of the callers of that treat all non-zero returns as errors, so it's
possible that this bug causes some problems in real life.

I could not find any callers that rely on the current behavior, and this
makes the function align with the get_register_interruptible() in
ab3100-core.c.

Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/mfd/ab8500-core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 30d09d177171..66458a329127 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
 	mutex_lock(&ab8500->lock);
 
 	ret = ab8500->read(ab8500, addr);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
 			addr, ret);
-	else
-		*value = ret;
+		return ret;
+	}
+
+	*value = ret;
 
 	mutex_unlock(&ab8500->lock);
 	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
 
-	return ret;
+	return 0;
 }
 
 static int ab8500_get_register(struct device *dev, u8 bank,
-- 
2.11.0

[PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Dan Carpenter]

I just noticed this in review.  The get_register_interruptible() should
return zero on success but it instead returns the value that it read.

I looked at all the places that called this directly and they check for
negatives and treat greater than or equal to zero as success.  This
function is also called as the ->get_register() function pointer.  Some
of the callers of that treat all non-zero returns as errors, so it's
possible that this bug causes some problems in real life.

I could not find any callers that rely on the current behavior, and this
makes the function align with the get_register_interruptible() in
ab3100-core.c.

Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/mfd/ab8500-core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 30d09d177171..66458a329127 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
 	mutex_lock(&ab8500->lock);
 
 	ret = ab8500->read(ab8500, addr);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
 			addr, ret);
-	else
-		*value = ret;
+		return ret;
+	}
+
+	*value = ret;
 
 	mutex_unlock(&ab8500->lock);
 	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
 
-	return ret;
+	return 0;
 }
 
 static int ab8500_get_register(struct device *dev, u8 bank,
-- 
2.11.0

[PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Dan Carpenter]

I just noticed this in review.  The get_register_interruptible() should
return zero on success but it instead returns the value that it read.

I looked at all the places that called this directly and they check for
negatives and treat greater than or equal to zero as success.  This
function is also called as the ->get_register() function pointer.  Some
of the callers of that treat all non-zero returns as errors, so it's
possible that this bug causes some problems in real life.

I could not find any callers that rely on the current behavior, and this
makes the function align with the get_register_interruptible() in
ab3100-core.c.

Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/mfd/ab8500-core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 30d09d177171..66458a329127 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
 	mutex_lock(&ab8500->lock);
 
 	ret = ab8500->read(ab8500, addr);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
 			addr, ret);
-	else
-		*value = ret;
+		return ret;
+	}
+
+	*value = ret;
 
 	mutex_unlock(&ab8500->lock);
 	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
 
-	return ret;
+	return 0;
 }
 
 static int ab8500_get_register(struct device *dev, u8 bank,
-- 
2.11.0

Re: [PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Lee Jones]

On Thu, 18 Oct 2018, Dan Carpenter wrote:

> I just noticed this in review.  The get_register_interruptible() should
> return zero on success but it instead returns the value that it read.
> 
> I looked at all the places that called this directly and they check for
> negatives and treat greater than or equal to zero as success.  This
> function is also called as the ->get_register() function pointer.  Some
> of the callers of that treat all non-zero returns as errors, so it's
> possible that this bug causes some problems in real life.
> 
> I could not find any callers that rely on the current behavior, and this
> makes the function align with the get_register_interruptible() in
> ab3100-core.c.
> 
> Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/mfd/ab8500-core.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> index 30d09d177171..66458a329127 100644
> --- a/drivers/mfd/ab8500-core.c
> +++ b/drivers/mfd/ab8500-core.c
> @@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
>  	mutex_lock(&ab8500->lock);
>  
>  	ret = ab8500->read(ab8500, addr);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
>  			addr, ret);
> -	else
> -		*value = ret;
> +		return ret;

Looks like you just broke the AB8500.

What is it you're trying to achieve here?  Apart from attempting to
return with the mutex still held, what semantics have you changed?

> +	}
> +
> +	*value = ret;
>  
>  	mutex_unlock(&ab8500->lock);
>  	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int ab8500_get_register(struct device *dev, u8 bank,

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Lee Jones]

On Thu, 18 Oct 2018, Dan Carpenter wrote:

> I just noticed this in review.  The get_register_interruptible() should
> return zero on success but it instead returns the value that it read.
> 
> I looked at all the places that called this directly and they check for
> negatives and treat greater than or equal to zero as success.  This
> function is also called as the ->get_register() function pointer.  Some
> of the callers of that treat all non-zero returns as errors, so it's
> possible that this bug causes some problems in real life.
> 
> I could not find any callers that rely on the current behavior, and this
> makes the function align with the get_register_interruptible() in
> ab3100-core.c.
> 
> Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/mfd/ab8500-core.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> index 30d09d177171..66458a329127 100644
> --- a/drivers/mfd/ab8500-core.c
> +++ b/drivers/mfd/ab8500-core.c
> @@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
>  	mutex_lock(&ab8500->lock);
>  
>  	ret = ab8500->read(ab8500, addr);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
>  			addr, ret);
> -	else
> -		*value = ret;
> +		return ret;

Looks like you just broke the AB8500.

What is it you're trying to achieve here?  Apart from attempting to
return with the mutex still held, what semantics have you changed?

> +	}
> +
> +	*value = ret;
>  
>  	mutex_unlock(&ab8500->lock);
>  	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int ab8500_get_register(struct device *dev, u8 bank,

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Lee Jones]

On Thu, 18 Oct 2018, Dan Carpenter wrote:

> I just noticed this in review.  The get_register_interruptible() should
> return zero on success but it instead returns the value that it read.
> 
> I looked at all the places that called this directly and they check for
> negatives and treat greater than or equal to zero as success.  This
> function is also called as the ->get_register() function pointer.  Some
> of the callers of that treat all non-zero returns as errors, so it's
> possible that this bug causes some problems in real life.
> 
> I could not find any callers that rely on the current behavior, and this
> makes the function align with the get_register_interruptible() in
> ab3100-core.c.
> 
> Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/mfd/ab8500-core.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> index 30d09d177171..66458a329127 100644
> --- a/drivers/mfd/ab8500-core.c
> +++ b/drivers/mfd/ab8500-core.c
> @@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
>  	mutex_lock(&ab8500->lock);
>  
>  	ret = ab8500->read(ab8500, addr);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
>  			addr, ret);
> -	else
> -		*value = ret;
> +		return ret;

Looks like you just broke the AB8500.

What is it you're trying to achieve here?  Apart from attempting to
return with the mutex still held, what semantics have you changed?

> +	}
> +
> +	*value = ret;
>  
>  	mutex_unlock(&ab8500->lock);
>  	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int ab8500_get_register(struct device *dev, u8 bank,

-- 
Lee Jones [???]
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Dan Carpenter]

On Thu, Oct 25, 2018 at 09:25:08AM +0100, Lee Jones wrote:
> On Thu, 18 Oct 2018, Dan Carpenter wrote:
> 
> > I just noticed this in review.  The get_register_interruptible() should
> > return zero on success but it instead returns the value that it read.
> > 
> > I looked at all the places that called this directly and they check for
> > negatives and treat greater than or equal to zero as success.  This
> > function is also called as the ->get_register() function pointer.  Some
> > of the callers of that treat all non-zero returns as errors, so it's
> > possible that this bug causes some problems in real life.
> > 
> > I could not find any callers that rely on the current behavior, and this
> > makes the function align with the get_register_interruptible() in
> > ab3100-core.c.
> > 
> > Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/mfd/ab8500-core.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> > index 30d09d177171..66458a329127 100644
> > --- a/drivers/mfd/ab8500-core.c
> > +++ b/drivers/mfd/ab8500-core.c
> > @@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
> >  	mutex_lock(&ab8500->lock);
> >  
> >  	ret = ab8500->read(ab8500, addr);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> >  		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
> >  			addr, ret);
> > -	else
> > -		*value = ret;
> > +		return ret;
> 
> Looks like you just broke the AB8500.
> 


Oh wow.  I screwed up the locking.


> What is it you're trying to achieve here?  Apart from attempting to
> return with the mutex still held, what semantics have you changed?
> 

Sorry that wasn't clear.  Here is the relevant bits from the commit
message.

> > function is also called as the ->get_register() function pointer.  Some
> > of the callers of that treat all non-zero returns as errors, so it's
> > possible that this bug causes some problems in real life.

We're returning positive non-zero values on success instead of zero.
It's definitely a bug, but I'm not sure if it has an impact in real
life.

regards,
dan carpenter

Re: [PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Dan Carpenter]

On Thu, Oct 25, 2018 at 09:25:08AM +0100, Lee Jones wrote:
> On Thu, 18 Oct 2018, Dan Carpenter wrote:
> 
> > I just noticed this in review.  The get_register_interruptible() should
> > return zero on success but it instead returns the value that it read.
> > 
> > I looked at all the places that called this directly and they check for
> > negatives and treat greater than or equal to zero as success.  This
> > function is also called as the ->get_register() function pointer.  Some
> > of the callers of that treat all non-zero returns as errors, so it's
> > possible that this bug causes some problems in real life.
> > 
> > I could not find any callers that rely on the current behavior, and this
> > makes the function align with the get_register_interruptible() in
> > ab3100-core.c.
> > 
> > Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/mfd/ab8500-core.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> > index 30d09d177171..66458a329127 100644
> > --- a/drivers/mfd/ab8500-core.c
> > +++ b/drivers/mfd/ab8500-core.c
> > @@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
> >  	mutex_lock(&ab8500->lock);
> >  
> >  	ret = ab8500->read(ab8500, addr);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> >  		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
> >  			addr, ret);
> > -	else
> > -		*value = ret;
> > +		return ret;
> 
> Looks like you just broke the AB8500.
> 


Oh wow.  I screwed up the locking.


> What is it you're trying to achieve here?  Apart from attempting to
> return with the mutex still held, what semantics have you changed?
> 

Sorry that wasn't clear.  Here is the relevant bits from the commit
message.

> > function is also called as the ->get_register() function pointer.  Some
> > of the callers of that treat all non-zero returns as errors, so it's
> > possible that this bug causes some problems in real life.

We're returning positive non-zero values on success instead of zero.
It's definitely a bug, but I'm not sure if it has an impact in real
life.

regards,
dan carpenter

[PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Dan Carpenter]

On Thu, Oct 25, 2018 at 09:25:08AM +0100, Lee Jones wrote:
> On Thu, 18 Oct 2018, Dan Carpenter wrote:
> 
> > I just noticed this in review.  The get_register_interruptible() should
> > return zero on success but it instead returns the value that it read.
> > 
> > I looked at all the places that called this directly and they check for
> > negatives and treat greater than or equal to zero as success.  This
> > function is also called as the ->get_register() function pointer.  Some
> > of the callers of that treat all non-zero returns as errors, so it's
> > possible that this bug causes some problems in real life.
> > 
> > I could not find any callers that rely on the current behavior, and this
> > makes the function align with the get_register_interruptible() in
> > ab3100-core.c.
> > 
> > Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/mfd/ab8500-core.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> > index 30d09d177171..66458a329127 100644
> > --- a/drivers/mfd/ab8500-core.c
> > +++ b/drivers/mfd/ab8500-core.c
> > @@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
> >  	mutex_lock(&ab8500->lock);
> >  
> >  	ret = ab8500->read(ab8500, addr);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> >  		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
> >  			addr, ret);
> > -	else
> > -		*value = ret;
> > +		return ret;
> 
> Looks like you just broke the AB8500.
> 


Oh wow.  I screwed up the locking.


> What is it you're trying to achieve here?  Apart from attempting to
> return with the mutex still held, what semantics have you changed?
> 

Sorry that wasn't clear.  Here is the relevant bits from the commit
message.

> > function is also called as the ->get_register() function pointer.  Some
> > of the callers of that treat all non-zero returns as errors, so it's
> > possible that this bug causes some problems in real life.

We're returning positive non-zero values on success instead of zero.
It's definitely a bug, but I'm not sure if it has an impact in real
life.

regards,
dan carpenter

Re: [PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Lee Jones]

On Thu, 25 Oct 2018, Dan Carpenter wrote:

> On Thu, Oct 25, 2018 at 09:25:08AM +0100, Lee Jones wrote:
> > On Thu, 18 Oct 2018, Dan Carpenter wrote:
> > 
> > > I just noticed this in review.  The get_register_interruptible() should
> > > return zero on success but it instead returns the value that it read.
> > > 
> > > I looked at all the places that called this directly and they check for
> > > negatives and treat greater than or equal to zero as success.  This
> > > function is also called as the ->get_register() function pointer.  Some
> > > of the callers of that treat all non-zero returns as errors, so it's
> > > possible that this bug causes some problems in real life.
> > > 
> > > I could not find any callers that rely on the current behavior, and this
> > > makes the function align with the get_register_interruptible() in
> > > ab3100-core.c.
> > > 
> > > Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > >  drivers/mfd/ab8500-core.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> > > index 30d09d177171..66458a329127 100644
> > > --- a/drivers/mfd/ab8500-core.c
> > > +++ b/drivers/mfd/ab8500-core.c
> > > @@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
> > >  	mutex_lock(&ab8500->lock);
> > >  
> > >  	ret = ab8500->read(ab8500, addr);
> > > -	if (ret < 0)
> > > +	if (ret < 0) {
> > >  		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
> > >  			addr, ret);
> > > -	else
> > > -		*value = ret;
> > > +		return ret;
> > 
> > Looks like you just broke the AB8500.
> > 
> 
> 
> Oh wow.  I screwed up the locking.
> 
> 
> > What is it you're trying to achieve here?  Apart from attempting to
> > return with the mutex still held, what semantics have you changed?
> > 
> 
> Sorry that wasn't clear.  Here is the relevant bits from the commit
> message.
> 
> > > function is also called as the ->get_register() function pointer.  Some
> > > of the callers of that treat all non-zero returns as errors, so it's
> > > possible that this bug causes some problems in real life.
> 
> We're returning positive non-zero values on success instead of zero.
> It's definitely a bug, but I'm not sure if it has an impact in real
> life.

Oh, I see.

Maybe:

	return (ret > 0) ? 0 : ret;

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Lee Jones]

On Thu, 25 Oct 2018, Dan Carpenter wrote:

> On Thu, Oct 25, 2018 at 09:25:08AM +0100, Lee Jones wrote:
> > On Thu, 18 Oct 2018, Dan Carpenter wrote:
> > 
> > > I just noticed this in review.  The get_register_interruptible() should
> > > return zero on success but it instead returns the value that it read.
> > > 
> > > I looked at all the places that called this directly and they check for
> > > negatives and treat greater than or equal to zero as success.  This
> > > function is also called as the ->get_register() function pointer.  Some
> > > of the callers of that treat all non-zero returns as errors, so it's
> > > possible that this bug causes some problems in real life.
> > > 
> > > I could not find any callers that rely on the current behavior, and this
> > > makes the function align with the get_register_interruptible() in
> > > ab3100-core.c.
> > > 
> > > Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > >  drivers/mfd/ab8500-core.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> > > index 30d09d177171..66458a329127 100644
> > > --- a/drivers/mfd/ab8500-core.c
> > > +++ b/drivers/mfd/ab8500-core.c
> > > @@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
> > >  	mutex_lock(&ab8500->lock);
> > >  
> > >  	ret = ab8500->read(ab8500, addr);
> > > -	if (ret < 0)
> > > +	if (ret < 0) {
> > >  		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
> > >  			addr, ret);
> > > -	else
> > > -		*value = ret;
> > > +		return ret;
> > 
> > Looks like you just broke the AB8500.
> > 
> 
> 
> Oh wow.  I screwed up the locking.
> 
> 
> > What is it you're trying to achieve here?  Apart from attempting to
> > return with the mutex still held, what semantics have you changed?
> > 
> 
> Sorry that wasn't clear.  Here is the relevant bits from the commit
> message.
> 
> > > function is also called as the ->get_register() function pointer.  Some
> > > of the callers of that treat all non-zero returns as errors, so it's
> > > possible that this bug causes some problems in real life.
> 
> We're returning positive non-zero values on success instead of zero.
> It's definitely a bug, but I'm not sure if it has an impact in real
> life.

Oh, I see.

Maybe:

	return (ret > 0) ? 0 : ret;

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Lee Jones]

On Thu, 25 Oct 2018, Dan Carpenter wrote:

> On Thu, Oct 25, 2018 at 09:25:08AM +0100, Lee Jones wrote:
> > On Thu, 18 Oct 2018, Dan Carpenter wrote:
> > 
> > > I just noticed this in review.  The get_register_interruptible() should
> > > return zero on success but it instead returns the value that it read.
> > > 
> > > I looked at all the places that called this directly and they check for
> > > negatives and treat greater than or equal to zero as success.  This
> > > function is also called as the ->get_register() function pointer.  Some
> > > of the callers of that treat all non-zero returns as errors, so it's
> > > possible that this bug causes some problems in real life.
> > > 
> > > I could not find any callers that rely on the current behavior, and this
> > > makes the function align with the get_register_interruptible() in
> > > ab3100-core.c.
> > > 
> > > Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > >  drivers/mfd/ab8500-core.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> > > index 30d09d177171..66458a329127 100644
> > > --- a/drivers/mfd/ab8500-core.c
> > > +++ b/drivers/mfd/ab8500-core.c
> > > @@ -252,16 +252,18 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
> > >  	mutex_lock(&ab8500->lock);
> > >  
> > >  	ret = ab8500->read(ab8500, addr);
> > > -	if (ret < 0)
> > > +	if (ret < 0) {
> > >  		dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
> > >  			addr, ret);
> > > -	else
> > > -		*value = ret;
> > > +		return ret;
> > 
> > Looks like you just broke the AB8500.
> > 
> 
> 
> Oh wow.  I screwed up the locking.
> 
> 
> > What is it you're trying to achieve here?  Apart from attempting to
> > return with the mutex still held, what semantics have you changed?
> > 
> 
> Sorry that wasn't clear.  Here is the relevant bits from the commit
> message.
> 
> > > function is also called as the ->get_register() function pointer.  Some
> > > of the callers of that treat all non-zero returns as errors, so it's
> > > possible that this bug causes some problems in real life.
> 
> We're returning positive non-zero values on success instead of zero.
> It's definitely a bug, but I'm not sure if it has an impact in real
> life.

Oh, I see.

Maybe:

	return (ret > 0) ? 0 : ret;

-- 
Lee Jones [???]
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/2 v2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Dan Carpenter]

This function is supposed to return zero on success or negative error
codes on error.  Unfortunately, there is a bug so it sometimes returns
non-zero, positive numbers on success.

I noticed this bug during review and I can't test it.  It does appear
that the return is sometimes propogated back to _regmap_read() where all
non-zero returns are treated as failure so this may affect run time.

Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: In v1, I introduced an new error path and forgot to drop the lock.

 drivers/mfd/ab8500-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 30d09d177171..11ab17f64c64 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -261,7 +261,7 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
 	mutex_unlock(&ab8500->lock);
 	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
 
-	return ret;
+	return (ret < 0) ? ret : 0;
 }
 
 static int ab8500_get_register(struct device *dev, u8 bank,

[PATCH 2/2 v2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Dan Carpenter]

This function is supposed to return zero on success or negative error
codes on error.  Unfortunately, there is a bug so it sometimes returns
non-zero, positive numbers on success.

I noticed this bug during review and I can't test it.  It does appear
that the return is sometimes propogated back to _regmap_read() where all
non-zero returns are treated as failure so this may affect run time.

Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: In v1, I introduced an new error path and forgot to drop the lock.

 drivers/mfd/ab8500-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
index 30d09d177171..11ab17f64c64 100644
--- a/drivers/mfd/ab8500-core.c
+++ b/drivers/mfd/ab8500-core.c
@@ -261,7 +261,7 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
 	mutex_unlock(&ab8500->lock);
 	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
 
-	return ret;
+	return (ret < 0) ? ret : 0;
 }
 
 static int ab8500_get_register(struct device *dev, u8 bank,

Re: [PATCH 2/2 v2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Lee Jones]

On Thu, 25 Oct 2018, Dan Carpenter wrote:

> This function is supposed to return zero on success or negative error
> codes on error.  Unfortunately, there is a bug so it sometimes returns
> non-zero, positive numbers on success.
> 
> I noticed this bug during review and I can't test it.  It does appear
> that the return is sometimes propogated back to _regmap_read() where all
> non-zero returns are treated as failure so this may affect run time.
> 
> Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: In v1, I introduced an new error path and forgot to drop the lock.
> 
>  drivers/mfd/ab8500-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks okay at first blush.

I'd really like Linus to review/test though.

> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> index 30d09d177171..11ab17f64c64 100644
> --- a/drivers/mfd/ab8500-core.c
> +++ b/drivers/mfd/ab8500-core.c
> @@ -261,7 +261,7 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
>  	mutex_unlock(&ab8500->lock);
>  	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
>  
> -	return ret;
> +	return (ret < 0) ? ret : 0;
>  }
>  
>  static int ab8500_get_register(struct device *dev, u8 bank,

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/2 v2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Lee Jones]

On Thu, 25 Oct 2018, Dan Carpenter wrote:

> This function is supposed to return zero on success or negative error
> codes on error.  Unfortunately, there is a bug so it sometimes returns
> non-zero, positive numbers on success.
> 
> I noticed this bug during review and I can't test it.  It does appear
> that the return is sometimes propogated back to _regmap_read() where all
> non-zero returns are treated as failure so this may affect run time.
> 
> Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: In v1, I introduced an new error path and forgot to drop the lock.
> 
>  drivers/mfd/ab8500-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks okay at first blush.

I'd really like Linus to review/test though.

> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> index 30d09d177171..11ab17f64c64 100644
> --- a/drivers/mfd/ab8500-core.c
> +++ b/drivers/mfd/ab8500-core.c
> @@ -261,7 +261,7 @@ static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
>  	mutex_unlock(&ab8500->lock);
>  	dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
>  
> -	return ret;
> +	return (ret < 0) ? ret : 0;
>  }
>  
>  static int ab8500_get_register(struct device *dev, u8 bank,

-- 
Lee Jones [???]
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2 v2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Linus Walleij]

On Thu, Oct 25, 2018 at 2:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:

> This function is supposed to return zero on success or negative error
> codes on error.  Unfortunately, there is a bug so it sometimes returns
> non-zero, positive numbers on success.
>
> I noticed this bug during review and I can't test it.  It does appear
> that the return is sometimes propogated back to _regmap_read() where all
> non-zero returns are treated as failure so this may affect run time.
>
> Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: In v1, I introduced an new error path and forgot to drop the lock.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I've got some cleanup work to do with this driver.
Getting to it.

Yours,
Linus Walleij

[PATCH 2/2 v2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Linus Walleij]

On Thu, Oct 25, 2018 at 2:44 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:

> This function is supposed to return zero on success or negative error
> codes on error.  Unfortunately, there is a bug so it sometimes returns
> non-zero, positive numbers on success.
>
> I noticed this bug during review and I can't test it.  It does appear
> that the return is sometimes propogated back to _regmap_read() where all
> non-zero returns are treated as failure so this may affect run time.
>
> Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: In v1, I introduced an new error path and forgot to drop the lock.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I've got some cleanup work to do with this driver.
Getting to it.

Yours,
Linus Walleij

Re: [PATCH 2/2 v2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Lee Jones]

On Thu, 25 Oct 2018, Dan Carpenter wrote:

> This function is supposed to return zero on success or negative error
> codes on error.  Unfortunately, there is a bug so it sometimes returns
> non-zero, positive numbers on success.
> 
> I noticed this bug during review and I can't test it.  It does appear
> that the return is sometimes propogated back to _regmap_read() where all
> non-zero returns are treated as failure so this may affect run time.
> 
> Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: In v1, I introduced an new error path and forgot to drop the lock.
> 
>  drivers/mfd/ab8500-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/2 v2] mfd: ab8500-core: Return zero in get_register_interruptible()

[Lee Jones]

On Thu, 25 Oct 2018, Dan Carpenter wrote:

> This function is supposed to return zero on success or negative error
> codes on error.  Unfortunately, there is a bug so it sometimes returns
> non-zero, positive numbers on success.
> 
> I noticed this bug during review and I can't test it.  It does appear
> that the return is sometimes propogated back to _regmap_read() where all
> non-zero returns are treated as failure so this may affect run time.
> 
> Fixes: 47c1697508f2 ("mfd: Align ab8500 with the abx500 interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: In v1, I introduced an new error path and forgot to drop the lock.
> 
>  drivers/mfd/ab8500-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

-- 
Lee Jones [???]
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog