[RFT/RFC] power_supply: bq2415x_charger: Initialize workqueue before scheduling it

[RFT/RFC] power_supply: bq2415x_charger: Initialize workqueue before scheduling it

[Krzysztof Kozlowski]

The driver during probe registers a power supply notifier
(with bq2415x_notifier_call() callback) and calls it manually right
after. The notifier callback function schedules driver's workqueue
(bq->work).

However the workqueue was initialized after these two events (after
registering power supply notifier and calling manually the callback).

When power supply core notified the driver (executing its
bq2415x_notifier_call() callback) the scheduled workqueue could be
still uninitialized.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reported-by: Pali Rohár <pali.rohar@gmail.com>

---

I don't have the hardware, please test. Additionally I could not find
the commit which introduced the issue so I did not cc-stable.
---
 drivers/power/bq2415x_charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/bq2415x_charger.c b/drivers/power/bq2415x_charger.c
index e98dcb661cc9..527ed0f18796 100644
--- a/drivers/power/bq2415x_charger.c
+++ b/drivers/power/bq2415x_charger.c
@@ -1601,6 +1601,7 @@ static int bq2415x_probe(struct i2c_client *client,
 	bq->reported_mode = BQ2415X_MODE_OFF;
 	bq->autotimer = 0;
 	bq->automode = 0;
+	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
 
 	if (np || ACPI_HANDLE(bq->dev)) {
 		ret = device_property_read_u32(bq->dev,
@@ -1677,7 +1678,6 @@ static int bq2415x_probe(struct i2c_client *client,
 		dev_info(bq->dev, "automode not supported\n");
 	}
 
-	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
 	bq2415x_set_autotimer(bq, 1);
 
 	dev_info(bq->dev, "driver registered\n");
-- 
1.9.1

Re: [RFT/RFC] power_supply: bq2415x_charger: Initialize workqueue before scheduling it

[Pali Rohár]

On Tuesday 14 July 2015 09:34:13 Krzysztof Kozlowski wrote:
> The driver during probe registers a power supply notifier
> (with bq2415x_notifier_call() callback) and calls it manually right
> after. The notifier callback function schedules driver's workqueue
> (bq->work).
> 
> However the workqueue was initialized after these two events (after
> registering power supply notifier and calling manually the callback).
> 
> When power supply core notified the driver (executing its
> bq2415x_notifier_call() callback) the scheduled workqueue could be
> still uninitialized.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Reported-by: Pali Rohár <pali.rohar@gmail.com>
> 
> ---
> 
> I don't have the hardware, please test. Additionally I could not find
> the commit which introduced the issue so I did not cc-stable.

Thanks for the patch! Please wait some time (about week) and I will test
it on real Nokia N900 hardware.

Because bq2415x depends on isp1704 on Nokia N900 and isp1704 is not
emulated by qemu, we cannot test any bq2415x changes in qemu :-(

> ---
>  drivers/power/bq2415x_charger.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/bq2415x_charger.c b/drivers/power/bq2415x_charger.c
> index e98dcb661cc9..527ed0f18796 100644
> --- a/drivers/power/bq2415x_charger.c
> +++ b/drivers/power/bq2415x_charger.c
> @@ -1601,6 +1601,7 @@ static int bq2415x_probe(struct i2c_client *client,
>  	bq->reported_mode = BQ2415X_MODE_OFF;
>  	bq->autotimer = 0;
>  	bq->automode = 0;
> +	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
>  
>  	if (np || ACPI_HANDLE(bq->dev)) {
>  		ret = device_property_read_u32(bq->dev,
> @@ -1677,7 +1678,6 @@ static int bq2415x_probe(struct i2c_client *client,
>  		dev_info(bq->dev, "automode not supported\n");
>  	}
>  
> -	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
>  	bq2415x_set_autotimer(bq, 1);
>  
>  	dev_info(bq->dev, "driver registered\n");

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [RFT/RFC] power_supply: bq2415x_charger: Initialize workqueue before scheduling it

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1971 bytes --]

On Tuesday 14 July 2015 02:34:13 Krzysztof Kozlowski wrote:
> The driver during probe registers a power supply notifier
> (with bq2415x_notifier_call() callback) and calls it manually right
> after. The notifier callback function schedules driver's workqueue
> (bq->work).
> 
> However the workqueue was initialized after these two events (after
> registering power supply notifier and calling manually the callback).
> 
> When power supply core notified the driver (executing its
> bq2415x_notifier_call() callback) the scheduled workqueue could be
> still uninitialized.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Reported-by: Pali Rohár <pali.rohar@gmail.com>
> 
> ---
> 
> I don't have the hardware, please test. Additionally I could not find
> the commit which introduced the issue so I did not cc-stable.
> ---
>  drivers/power/bq2415x_charger.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/bq2415x_charger.c
> b/drivers/power/bq2415x_charger.c index e98dcb661cc9..527ed0f18796
> 100644
> --- a/drivers/power/bq2415x_charger.c
> +++ b/drivers/power/bq2415x_charger.c
> @@ -1601,6 +1601,7 @@ static int bq2415x_probe(struct i2c_client
> *client, bq->reported_mode = BQ2415X_MODE_OFF;
>  	bq->autotimer = 0;
>  	bq->automode = 0;
> +	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
> 
>  	if (np || ACPI_HANDLE(bq->dev)) {
>  		ret = device_property_read_u32(bq->dev,
> @@ -1677,7 +1678,6 @@ static int bq2415x_probe(struct i2c_client
> *client, dev_info(bq->dev, "automode not supported\n");
>  	}
> 
> -	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
>  	bq2415x_set_autotimer(bq, 1);
> 
>  	dev_info(bq->dev, "driver registered\n");

Looks like this is really problem. I sent alternative patch to mailing 
list which should fix this problem too plus allows to load 
bq2415x_charger.ko in n900 qemu.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFT/RFC] power_supply: bq2415x_charger: Initialize workqueue before scheduling it

[Sebastian Reichel]

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

Hi,

On Sat, Jul 25, 2015 at 11:49:06PM +0200, Pali Rohár wrote:
> On Tuesday 14 July 2015 02:34:13 Krzysztof Kozlowski wrote:
> > The driver during probe registers a power supply notifier
> > (with bq2415x_notifier_call() callback) and calls it manually right
> > after. The notifier callback function schedules driver's workqueue
> > (bq->work).
> > 
> > However the workqueue was initialized after these two events (after
> > registering power supply notifier and calling manually the callback).
> > 
> > When power supply core notified the driver (executing its
> > bq2415x_notifier_call() callback) the scheduled workqueue could be
> > still uninitialized.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Reported-by: Pali Rohár <pali.rohar@gmail.com>
> > 
> > ---
> > 
> > I don't have the hardware, please test. Additionally I could not find
> > the commit which introduced the issue so I did not cc-stable.
> > ---
> >  drivers/power/bq2415x_charger.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/power/bq2415x_charger.c
> > b/drivers/power/bq2415x_charger.c index e98dcb661cc9..527ed0f18796
> > 100644
> > --- a/drivers/power/bq2415x_charger.c
> > +++ b/drivers/power/bq2415x_charger.c
> > @@ -1601,6 +1601,7 @@ static int bq2415x_probe(struct i2c_client
> > *client, bq->reported_mode = BQ2415X_MODE_OFF;
> >  	bq->autotimer = 0;
> >  	bq->automode = 0;
> > +	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
> > 
> >  	if (np || ACPI_HANDLE(bq->dev)) {
> >  		ret = device_property_read_u32(bq->dev,
> > @@ -1677,7 +1678,6 @@ static int bq2415x_probe(struct i2c_client
> > *client, dev_info(bq->dev, "automode not supported\n");
> >  	}
> > 
> > -	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
> >  	bq2415x_set_autotimer(bq, 1);
> > 
> >  	dev_info(bq->dev, "driver registered\n");
> 
> Looks like this is really problem. I sent alternative patch to mailing 
> list which should fix this problem too plus allows to load 
> bq2415x_charger.ko in n900 qemu.

I have not yet reviewed your patch (just skipped over it), but the
qemu changes are a bit intrusive for -rc. I suggest to apply this
patch for 4.2-rc. The extended patch can then be added in 4.3.

-- Sebastian

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

Re: [RFT/RFC] power_supply: bq2415x_charger: Initialize workqueue before scheduling it

[Pali Rohár]

On Monday 27 July 2015 16:22:29 Sebastian Reichel wrote:
> Hi,
> 
> On Sat, Jul 25, 2015 at 11:49:06PM +0200, Pali Rohár wrote:
> > On Tuesday 14 July 2015 02:34:13 Krzysztof Kozlowski wrote:
> > > The driver during probe registers a power supply notifier
> > > (with bq2415x_notifier_call() callback) and calls it manually right
> > > after. The notifier callback function schedules driver's workqueue
> > > (bq->work).
> > > 
> > > However the workqueue was initialized after these two events (after
> > > registering power supply notifier and calling manually the callback).
> > > 
> > > When power supply core notified the driver (executing its
> > > bq2415x_notifier_call() callback) the scheduled workqueue could be
> > > still uninitialized.
> > > 
> > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > Reported-by: Pali Rohár <pali.rohar@gmail.com>
> > > 
> > > ---
> > > 
> > > I don't have the hardware, please test. Additionally I could not find
> > > the commit which introduced the issue so I did not cc-stable.
> > > ---
> > >  drivers/power/bq2415x_charger.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/power/bq2415x_charger.c
> > > b/drivers/power/bq2415x_charger.c index e98dcb661cc9..527ed0f18796
> > > 100644
> > > --- a/drivers/power/bq2415x_charger.c
> > > +++ b/drivers/power/bq2415x_charger.c
> > > @@ -1601,6 +1601,7 @@ static int bq2415x_probe(struct i2c_client
> > > *client, bq->reported_mode = BQ2415X_MODE_OFF;
> > >  	bq->autotimer = 0;
> > >  	bq->automode = 0;
> > > +	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
> > > 
> > >  	if (np || ACPI_HANDLE(bq->dev)) {
> > >  		ret = device_property_read_u32(bq->dev,
> > > @@ -1677,7 +1678,6 @@ static int bq2415x_probe(struct i2c_client
> > > *client, dev_info(bq->dev, "automode not supported\n");
> > >  	}
> > > 
> > > -	INIT_DELAYED_WORK(&bq->work, bq2415x_timer_work);
> > >  	bq2415x_set_autotimer(bq, 1);
> > > 
> > >  	dev_info(bq->dev, "driver registered\n");
> > 
> > Looks like this is really problem. I sent alternative patch to mailing 
> > list which should fix this problem too plus allows to load 
> > bq2415x_charger.ko in n900 qemu.
> 
> I have not yet reviewed your patch (just skipped over it), but the
> qemu changes are a bit intrusive for -rc. I suggest to apply this
> patch for 4.2-rc. The extended patch can then be added in 4.3.
> 
> -- Sebastian

I'm not sure if this patch is enough... as timer_work should be called
after that init bq2415x_set_autotimer call.

I'm ok with postponing my patch later for 4.3. So if you think apply
this one for 4.2...

-- 
Pali Rohár
pali.rohar@gmail.com