[PATCH RESEND] twl4030_charger: fix charging current out-of-bounds

[PATCH RESEND] twl4030_charger: fix charging current out-of-bounds

[Andreas Kemnade]

the charging current uses unsigned int variables, if we step back
if the current is still low, we would run into negative which
means setting the target to a huge value.
Better add checks here.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/power/supply/twl4030_charger.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
index b72838663872..c954b7234393 100644
--- a/drivers/power/supply/twl4030_charger.c
+++ b/drivers/power/supply/twl4030_charger.c
@@ -421,7 +421,8 @@ static void twl4030_current_worker(struct work_struct *data)
 
 	if (v < USB_MIN_VOLT) {
 		/* Back up and stop adjusting. */
-		bci->usb_cur -= USB_CUR_STEP;
+		if (bci->usb_cur >= USB_CUR_STEP)
+			bci->usb_cur -= USB_CUR_STEP;
 		bci->usb_cur_target = bci->usb_cur;
 	} else if (bci->usb_cur >= bci->usb_cur_target ||
 		   bci->usb_cur + USB_CUR_STEP > USB_MAX_CURRENT) {
-- 
2.11.0

[PATCH RESEND] twl4030_charger: fix charging current out-of-bounds

[Andreas Kemnade]

the charging current uses unsigned int variables, if we step back
if the current is still low, we would run into negative which
means setting the target to a huge value.
Better add checks here.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/power/supply/twl4030_charger.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
index b72838663872..c954b7234393 100644
--- a/drivers/power/supply/twl4030_charger.c
+++ b/drivers/power/supply/twl4030_charger.c
@@ -421,7 +421,8 @@ static void twl4030_current_worker(struct work_struct *data)
 
 	if (v < USB_MIN_VOLT) {
 		/* Back up and stop adjusting. */
-		bci->usb_cur -= USB_CUR_STEP;
+		if (bci->usb_cur >= USB_CUR_STEP)
+			bci->usb_cur -= USB_CUR_STEP;
 		bci->usb_cur_target = bci->usb_cur;
 	} else if (bci->usb_cur >= bci->usb_cur_target ||
 		   bci->usb_cur + USB_CUR_STEP > USB_MAX_CURRENT) {
-- 
2.11.0

Re: [PATCH RESEND] twl4030_charger: fix charging current out-of-bounds

[Sebastian Reichel]

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

Hi,

On Mon, Sep 17, 2018 at 07:20:35AM +0200, Andreas Kemnade wrote:
> the charging current uses unsigned int variables, if we step back
> if the current is still low, we would run into negative which
> means setting the target to a huge value.
> Better add checks here.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/supply/twl4030_charger.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index b72838663872..c954b7234393 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -421,7 +421,8 @@ static void twl4030_current_worker(struct work_struct *data)
>  
>  	if (v < USB_MIN_VOLT) {
>  		/* Back up and stop adjusting. */
> -		bci->usb_cur -= USB_CUR_STEP;
> +		if (bci->usb_cur >= USB_CUR_STEP)
> +			bci->usb_cur -= USB_CUR_STEP;
>  		bci->usb_cur_target = bci->usb_cur;
>  	} else if (bci->usb_cur >= bci->usb_cur_target ||
>  		   bci->usb_cur + USB_CUR_STEP > USB_MAX_CURRENT) {
> -- 
> 2.11.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RESEND] twl4030_charger: fix charging current out-of-bounds

[Sebastian Reichel]

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

Hi,

On Mon, Sep 17, 2018 at 07:20:35AM +0200, Andreas Kemnade wrote:
> the charging current uses unsigned int variables, if we step back
> if the current is still low, we would run into negative which
> means setting the target to a huge value.
> Better add checks here.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/supply/twl4030_charger.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index b72838663872..c954b7234393 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -421,7 +421,8 @@ static void twl4030_current_worker(struct work_struct *data)
>  
>  	if (v < USB_MIN_VOLT) {
>  		/* Back up and stop adjusting. */
> -		bci->usb_cur -= USB_CUR_STEP;
> +		if (bci->usb_cur >= USB_CUR_STEP)
> +			bci->usb_cur -= USB_CUR_STEP;
>  		bci->usb_cur_target = bci->usb_cur;
>  	} else if (bci->usb_cur >= bci->usb_cur_target ||
>  		   bci->usb_cur + USB_CUR_STEP > USB_MAX_CURRENT) {
> -- 
> 2.11.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RESEND] twl4030_charger: fix charging current out-of-bounds

[Pavel Machek]

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

On Mon 2018-09-17 07:20:35, Andreas Kemnade wrote:
> the charging current uses unsigned int variables, if we step back
> if the current is still low, we would run into negative which
> means setting the target to a huge value.
> Better add checks here.

Do you expect this to happen in practice? Too high current while
charging seems bad, right?

Cc: stable?
								Pavel

> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/power/supply/twl4030_charger.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index b72838663872..c954b7234393 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -421,7 +421,8 @@ static void twl4030_current_worker(struct work_struct *data)
>  
>  	if (v < USB_MIN_VOLT) {
>  		/* Back up and stop adjusting. */
> -		bci->usb_cur -= USB_CUR_STEP;
> +		if (bci->usb_cur >= USB_CUR_STEP)
> +			bci->usb_cur -= USB_CUR_STEP;
>  		bci->usb_cur_target = bci->usb_cur;
>  	} else if (bci->usb_cur >= bci->usb_cur_target ||
>  		   bci->usb_cur + USB_CUR_STEP > USB_MAX_CURRENT) {

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH RESEND] twl4030_charger: fix charging current out-of-bounds

[Pavel Machek]

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

On Mon 2018-09-17 07:20:35, Andreas Kemnade wrote:
> the charging current uses unsigned int variables, if we step back
> if the current is still low, we would run into negative which
> means setting the target to a huge value.
> Better add checks here.

Do you expect this to happen in practice? Too high current while
charging seems bad, right?

Cc: stable?
								Pavel

> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/power/supply/twl4030_charger.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index b72838663872..c954b7234393 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -421,7 +421,8 @@ static void twl4030_current_worker(struct work_struct *data)
>  
>  	if (v < USB_MIN_VOLT) {
>  		/* Back up and stop adjusting. */
> -		bci->usb_cur -= USB_CUR_STEP;
> +		if (bci->usb_cur >= USB_CUR_STEP)
> +			bci->usb_cur -= USB_CUR_STEP;
>  		bci->usb_cur_target = bci->usb_cur;
>  	} else if (bci->usb_cur >= bci->usb_cur_target ||
>  		   bci->usb_cur + USB_CUR_STEP > USB_MAX_CURRENT) {

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH RESEND] twl4030_charger: fix charging current out-of-bounds

[Andreas Kemnade]

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

Hi Pavel,

On Sun, 30 Sep 2018 22:16:42 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> On Mon 2018-09-17 07:20:35, Andreas Kemnade wrote:
> > the charging current uses unsigned int variables, if we step back
> > if the current is still low, we would run into negative which
> > means setting the target to a huge value.
> > Better add checks here.  
> 
> Do you expect this to happen in practice? Too high current while
> charging seems bad, right?

I think you need a power supply delivering < 4.75V  and > 4.3 V without load
and still > 4.3 V (so that vbusunplug detection does not trigger) at 1.6A
(the maximal charge current) including loss in cables.
I think that is really rare. It is not the standard charger you find in your
cupboard. Could probably be a lab power supply with a good cable connection.

As a side effect of some other bug (I do none like this):
If some regulators are not enabled, voltage measurement can be wrong. And
then the regulators have to be turned on right in time for the charging to
start.

But I know one way to produce that behavior:
Without my "phy: phy-twl4030-usb: fix denied runtime access"
The following steps are possible to achieve that with the gta04:
1. put your device to suspend without charger connected
2. connect charger which could provide high currents.
   phy runtime will not be resumed, usb voltage measured will be very low
   (I remember something like 1.8V), so the power ramping up will be
   stopped immediately and that step-back will set current it to an
   erroneous value but it will not start charging
3. echo auto >/sys/class/power_supply/twl4030_usb/mode
   then the charging starts with the target current set in the last step.
> 
> Cc: stable?

Rethinking it, it would be a nice idea, but I think the mentioned 
"phy: phy-twl4030-usb: fix denied runtime access"
would even be more important to have in stable, since it fixes actual
charging problems. Well, if your device does not boot because it is empty
or if there is sometimes a kernel panic is no substancial difference to me.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RESEND] twl4030_charger: fix charging current out-of-bounds

[Andreas Kemnade]

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

Hi Pavel,

On Sun, 30 Sep 2018 22:16:42 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> On Mon 2018-09-17 07:20:35, Andreas Kemnade wrote:
> > the charging current uses unsigned int variables, if we step back
> > if the current is still low, we would run into negative which
> > means setting the target to a huge value.
> > Better add checks here.  
> 
> Do you expect this to happen in practice? Too high current while
> charging seems bad, right?

I think you need a power supply delivering < 4.75V  and > 4.3 V without load
and still > 4.3 V (so that vbusunplug detection does not trigger) at 1.6A
(the maximal charge current) including loss in cables.
I think that is really rare. It is not the standard charger you find in your
cupboard. Could probably be a lab power supply with a good cable connection.

As a side effect of some other bug (I do none like this):
If some regulators are not enabled, voltage measurement can be wrong. And
then the regulators have to be turned on right in time for the charging to
start.

But I know one way to produce that behavior:
Without my "phy: phy-twl4030-usb: fix denied runtime access"
The following steps are possible to achieve that with the gta04:
1. put your device to suspend without charger connected
2. connect charger which could provide high currents.
   phy runtime will not be resumed, usb voltage measured will be very low
   (I remember something like 1.8V), so the power ramping up will be
   stopped immediately and that step-back will set current it to an
   erroneous value but it will not start charging
3. echo auto >/sys/class/power_supply/twl4030_usb/mode
   then the charging starts with the target current set in the last step.
> 
> Cc: stable?

Rethinking it, it would be a nice idea, but I think the mentioned 
"phy: phy-twl4030-usb: fix denied runtime access"
would even be more important to have in stable, since it fixes actual
charging problems. Well, if your device does not boot because it is empty
or if there is sometimes a kernel panic is no substancial difference to me.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]