Re: w1/ds1wm regression after 2.6.39: "bus error, retrying"

Re: w1/ds1wm regression after 2.6.39: "bus error, retrying"

[Paul Parsons]

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

Oops, yes the msleep() had been left in. I've attached debug output from the original ds1wm.c. Not sure how the mailing list copes with bzip2 compressed attachments; hopefully it will work.

Regards,
Paul

--- On Sat, 25/6/11, Jean-François Dagenais <dagenaisj@sonatest.com> wrote:
> This seems to be the output after you have restored the
> msleep. Could you provide it without so we can get a better
> idea as to what happened. If indeed this is a major
> regression for many devices, we'll have to submit a patch to
> the main branch.
> /jfd

[-- Attachment #2: dmesg.bz2 --]
[-- Type: application/x-bzip2, Size: 6987 bytes --]

[PATCH] w1: ds1wm: add a reset recovery parameter

[Jean-François Dagenais]

This follows the regression on 3.0 reported by Paul Parsons regarding
the removal of the msleep(1) in the ds1wm_reset() function. This
sleep should not be required on normal circuitry provided the
pull-ups on the bus are correctly adapted to the slaves.
Unfortunately, this is not always the case. The sleep is restored
but as a parameter to the probe function in the pdata.
---
 drivers/mfd/asic3.c        |    1 +
 drivers/mfd/htc-pasic3.c   |    1 +
 drivers/w1/masters/ds1wm.c |    5 +++++
 include/linux/mfd/ds1wm.h  |    7 +++++++
 4 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/asic3.c b/drivers/mfd/asic3.c
index c27fd1f..c71ae09 100644
--- a/drivers/mfd/asic3.c
+++ b/drivers/mfd/asic3.c
@@ -619,6 +619,7 @@ static void asic3_clk_disable(struct asic3 *asic, struct asic3_clk *clk)
 /* MFD cells (SPI, PWM, LED, DS1WM, MMC) */
 static struct ds1wm_driver_data ds1wm_pdata = {
 	.active_high = 1,
+	.reset_recover_delay = 1,
 };
 
 static struct resource ds1wm_resources[] = {
diff --git a/drivers/mfd/htc-pasic3.c b/drivers/mfd/htc-pasic3.c
index 2808bd1..04c7093 100644
--- a/drivers/mfd/htc-pasic3.c
+++ b/drivers/mfd/htc-pasic3.c
@@ -99,6 +99,7 @@ static int ds1wm_disable(struct platform_device *pdev)
 
 static struct ds1wm_driver_data ds1wm_pdata = {
 	.active_high = 0,
+	.reset_recover_delay = 1,
 };
 
 static struct resource ds1wm_resources[] __initdata = {
diff --git a/drivers/w1/masters/ds1wm.c b/drivers/w1/masters/ds1wm.c
index ad57593..66bf82f 100644
--- a/drivers/w1/masters/ds1wm.c
+++ b/drivers/w1/masters/ds1wm.c
@@ -109,6 +109,7 @@ struct ds1wm_data {
 	/* byte to write that makes all intr disabled, */
 	/* considering active_state (IAS) (optimization) */
 	u8       int_en_reg_none;
+	unsigned int reset_recover_delay; /* see ds1wm.h */
 };
 
 static inline void ds1wm_write_register(struct ds1wm_data *ds1wm_data, u32 reg,
@@ -187,6 +188,9 @@ static int ds1wm_reset(struct ds1wm_data *ds1wm_data)
 		return 1;
 	}
 
+	if(ds1wm_data->reset_recover_delay)
+		msleep(ds1wm_data->reset_recover_delay);
+
 	return 0;
 }
 
@@ -490,6 +494,7 @@ static int ds1wm_probe(struct platform_device *pdev)
 	}
 	ds1wm_data->irq = res->start;
 	ds1wm_data->int_en_reg_none = (plat->active_high ? DS1WM_INTEN_IAS : 0);
+	ds1wm_data->reset_recover_delay = plat->reset_recover_delay;
 
 	if (res->flags & IORESOURCE_IRQ_HIGHEDGE)
 		irq_set_irq_type(ds1wm_data->irq, IRQ_TYPE_EDGE_RISING);
diff --git a/include/linux/mfd/ds1wm.h b/include/linux/mfd/ds1wm.h
index be469a3..068044f 100644
--- a/include/linux/mfd/ds1wm.h
+++ b/include/linux/mfd/ds1wm.h
@@ -3,4 +3,11 @@
 struct ds1wm_driver_data {
 	int active_high;
 	int clock_rate;
+	/* in milliseconds, the amount of time to */
+	/* sleep following a reset pulse. Zero    */
+	/* should work if your bus devices recover*/
+	/* time respects the 1-wire spec since the*/
+	/* ds1wm implements the precise timings of*/
+	/* a reset pulse/presence detect sequence.*/ 
+	unsigned int reset_recover_delay;
 };
-- 
1.7.5.4

Jean-Francois Dagenais 
Software Architect - B.Sc.A


Experience the new veo phased array flaw detector at www.sonatestveo.com.


Sonatest AP
2900 chemin des Quatre-Bourgeois
Bureau 305
Quebec City Quebec G1V 1Y4

T| +1 (418) 683 6222 x106   F| +1 (418) 683 7032   M|    W| www.sonatest.com



This message (and any associated files) is intended only for the use of linux-kernel@vger.kernel.org, johnpol@2ka.mipt.ru, lost.distance@yahoo.com and may contain information that is confidential, subject to copyright or constitutes a trade secret. If you are not the above recipient(s) you are hereby notified that any dissemination, copying or distribution of this message, or files associated with this message, is strictly prohibited. If you have received this message in error, please notify us immediately by replying to the message and deleting it from your computer. Any views or opinions presented are solely those of the author and do not necessarily represent those of the company.

Think green - help the environment by not printing this email.

Re: [PATCH] w1: ds1wm: add a reset recovery parameter

[Paul Parsons]

Thanks Jean-François; that patch set has resolved the problem.

Tested-by: Paul Parsons <lost.distance@yahoo.com>

--- On Mon, 27/6/11, Jean-François Dagenais <dagenaisj@sonatest.com> wrote:

> From: Jean-François Dagenais <dagenaisj@sonatest.com>
> Subject: [PATCH] w1: ds1wm: add a reset recovery parameter
> To: linux-kernel@vger.kernel.org
> Cc: johnpol@2ka.mipt.ru, "Paul Parsons" <lost.distance@yahoo.com>
> Date: Monday, 27 June, 2011, 14:17
> This follows the regression on 3.0
> reported by Paul Parsons regarding
> the removal of the msleep(1) in the ds1wm_reset() function.
> This
> sleep should not be required on normal circuitry provided
> the
> pull-ups on the bus are correctly adapted to the slaves.
> Unfortunately, this is not always the case. The sleep is
> restored
> but as a parameter to the probe function in the pdata.
> ---
>  drivers/mfd/asic3.c        | 
>   1 +
>  drivers/mfd/htc-pasic3.c   |    1
> +
>  drivers/w1/masters/ds1wm.c |    5 +++++
>  include/linux/mfd/ds1wm.h  |    7 +++++++
>  4 files changed, 14 insertions(+), 0 deletions(-)

Re: [PATCH] w1: ds1wm: add a reset recovery parameter

[Andrew Morton]

On Mon, 27 Jun 2011 09:17:58 -0400
Jean-Fran__ois Dagenais <dagenaisj@sonatest.com> wrote:

> This follows the regression on 3.0 reported by Paul Parsons regarding
> the removal of the msleep(1) in the ds1wm_reset() function. This
> sleep should not be required on normal circuitry provided the
> pull-ups on the bus are correctly adapted to the slaves.
> Unfortunately, this is not always the case. The sleep is restored
> but as a parameter to the probe function in the pdata.

A few things.

- When fixing a bug, please fully describe that bug in the changelog.
  So that others can determine whether your patch might fix a bug
  which they are observing.

  That's not so important when we're fixing a regression which was
  introduced since the most recent major kernel release, but it's good
  practice and who knows, someone might have backported the
  regression-introducing patch.

- The patch was missing your signed-off-by.  I added it.  Please
  ack this.

- It's conventional to give bug-reporters a hat tip via the
  Reported-by: tag.

Here's what I merged:


From: Jean-Fran_ois Dagenais <dagenaisj@sonatest.com>

This fixes a regression on 3.0 reported by Paul Parsons regarding the
removal of the msleep(1) in the ds1wm_reset() function:

: The linux-3.0-rc4 DS1WM 1-wire driver is logging "bus error, retrying"
: error messages on an HP iPAQ hx4700 PDA (XScale-PXA270):
: 
: <snip>
: Driver for 1-wire Dallas network protocol.
: DS1WM w1 busmaster driver - (c) 2004 Szabolcs Gyurko
: 1-Wire driver for the DS2760 battery monitor  chip  - (c) 2004-2005, Szabolcs Gyurko
: ds1wm ds1wm: pass: 1 bus error, retrying
: ds1wm ds1wm: pass: 2 bus error, retrying
: ds1wm ds1wm: pass: 3 bus error, retrying
: ds1wm ds1wm: pass: 4 bus error, retrying
: ds1wm ds1wm: pass: 5 bus error, retrying
: ...
: 
: The visible result is that the battery charging LED is erratic; sometimes
: it works, mostly it doesn't.
: 
: The linux-2.6.39 DS1WM 1-wire driver worked OK.  I haven't tried 3.0-rc1,
: 3.0-rc2, or 3.0-rc3.

This sleep should not be required on normal circuitry provided the
pull-ups on the bus are correctly adapted to the slaves.  Unfortunately,
this is not always the case.  The sleep is restored but as a parameter to
the probe function in the pdata.

Reported-by: Paul Parsons <lost.distance@yahoo.com>
Tested-by: Paul Parsons <lost.distance@yahoo.com>
Signed-off-by: Jean-Fran_ois Dagenais <dagenaisj@sonatest.com>
Cc: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/mfd/asic3.c        |    1 +
 drivers/mfd/htc-pasic3.c   |    1 +
 drivers/w1/masters/ds1wm.c |    5 +++++
 include/linux/mfd/ds1wm.h  |    7 +++++++
 4 files changed, 14 insertions(+)

diff -puN drivers/mfd/asic3.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/mfd/asic3.c
--- a/drivers/mfd/asic3.c~w1-ds1wm-add-a-reset-recovery-parameter
+++ a/drivers/mfd/asic3.c
@@ -619,6 +619,7 @@ static void asic3_clk_disable(struct asi
 /* MFD cells (SPI, PWM, LED, DS1WM, MMC) */
 static struct ds1wm_driver_data ds1wm_pdata = {
 	.active_high = 1,
+	.reset_recover_delay = 1,
 };
 
 static struct resource ds1wm_resources[] = {
diff -puN drivers/mfd/htc-pasic3.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/mfd/htc-pasic3.c
--- a/drivers/mfd/htc-pasic3.c~w1-ds1wm-add-a-reset-recovery-parameter
+++ a/drivers/mfd/htc-pasic3.c
@@ -99,6 +99,7 @@ static int ds1wm_disable(struct platform
 
 static struct ds1wm_driver_data ds1wm_pdata = {
 	.active_high = 0,
+	.reset_recover_delay = 1,
 };
 
 static struct resource ds1wm_resources[] __initdata = {
diff -puN drivers/w1/masters/ds1wm.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/w1/masters/ds1wm.c
--- a/drivers/w1/masters/ds1wm.c~w1-ds1wm-add-a-reset-recovery-parameter
+++ a/drivers/w1/masters/ds1wm.c
@@ -109,6 +109,7 @@ struct ds1wm_data {
 	/* byte to write that makes all intr disabled, */
 	/* considering active_state (IAS) (optimization) */
 	u8       int_en_reg_none;
+	unsigned int reset_recover_delay; /* see ds1wm.h */
 };
 
 static inline void ds1wm_write_register(struct ds1wm_data *ds1wm_data, u32 reg,
@@ -187,6 +188,9 @@ static int ds1wm_reset(struct ds1wm_data
 		return 1;
 	}
 
+	if(ds1wm_data->reset_recover_delay)
+		msleep(ds1wm_data->reset_recover_delay);
+
 	return 0;
 }
 
@@ -490,6 +494,7 @@ static int ds1wm_probe(struct platform_d
 	}
 	ds1wm_data->irq = res->start;
 	ds1wm_data->int_en_reg_none = (plat->active_high ? DS1WM_INTEN_IAS : 0);
+	ds1wm_data->reset_recover_delay = plat->reset_recover_delay;
 
 	if (res->flags & IORESOURCE_IRQ_HIGHEDGE)
 		irq_set_irq_type(ds1wm_data->irq, IRQ_TYPE_EDGE_RISING);
diff -puN include/linux/mfd/ds1wm.h~w1-ds1wm-add-a-reset-recovery-parameter include/linux/mfd/ds1wm.h
--- a/include/linux/mfd/ds1wm.h~w1-ds1wm-add-a-reset-recovery-parameter
+++ a/include/linux/mfd/ds1wm.h
@@ -3,4 +3,11 @@
 struct ds1wm_driver_data {
 	int active_high;
 	int clock_rate;
+	/* in milliseconds, the amount of time to */
+	/* sleep following a reset pulse. Zero    */
+	/* should work if your bus devices recover*/
+	/* time respects the 1-wire spec since the*/
+	/* ds1wm implements the precise timings of*/
+	/* a reset pulse/presence detect sequence.*/ 
+	unsigned int reset_recover_delay;
 };
_

Re: [PATCH] w1: ds1wm: add a reset recovery parameter

[Jean-François Dagenais]

On Jul 5, 2011, at 17:46, Andrew Morton wrote:

> On Mon, 27 Jun 2011 09:17:58 -0400
> Jean-Fran__ois Dagenais <dagenaisj@sonatest.com> wrote:
> 
>> This follows the regression on 3.0 reported by Paul Parsons regarding
>> the removal of the msleep(1) in the ds1wm_reset() function. This
>> sleep should not be required on normal circuitry provided the
>> pull-ups on the bus are correctly adapted to the slaves.
>> Unfortunately, this is not always the case. The sleep is restored
>> but as a parameter to the probe function in the pdata.
> 
> A few things.
> 
> - When fixing a bug, please fully describe that bug in the changelog.
>  So that others can determine whether your patch might fix a bug
>  which they are observing.
> 
>  That's not so important when we're fixing a regression which was
>  introduced since the most recent major kernel release, but it's good
>  practice and who knows, someone might have backported the
>  regression-introducing patch.
> 
> - The patch was missing your signed-off-by.  I added it.  Please
>  ack this.
> 
> - It's conventional to give bug-reporters a hat tip via the
>  Reported-by: tag.
> 
All noted. Thanks for the constructive feedback. ds1wm mods are my first contrib to the kernel.
> Here's what I merged:
> 
> 
> From: Jean-Fran_ois Dagenais <dagenaisj@sonatest.com>
If the c-cedilla doesn't work please replace the '_' by a regular 'c'... actually I will re-submit the patch as v2.
> 
> This fixes a regression on 3.0 reported by Paul Parsons regarding the
> removal of the msleep(1) in the ds1wm_reset() function:
> 
> : The linux-3.0-rc4 DS1WM 1-wire driver is logging "bus error, retrying"
> : error messages on an HP iPAQ hx4700 PDA (XScale-PXA270):
> : 
> : <snip>
> : Driver for 1-wire Dallas network protocol.
> : DS1WM w1 busmaster driver - (c) 2004 Szabolcs Gyurko
> : 1-Wire driver for the DS2760 battery monitor  chip  - (c) 2004-2005, Szabolcs Gyurko
> : ds1wm ds1wm: pass: 1 bus error, retrying
> : ds1wm ds1wm: pass: 2 bus error, retrying
> : ds1wm ds1wm: pass: 3 bus error, retrying
> : ds1wm ds1wm: pass: 4 bus error, retrying
> : ds1wm ds1wm: pass: 5 bus error, retrying
> : ...
> : 
> : The visible result is that the battery charging LED is erratic; sometimes
> : it works, mostly it doesn't.
> : 
> : The linux-2.6.39 DS1WM 1-wire driver worked OK.  I haven't tried 3.0-rc1,
> : 3.0-rc2, or 3.0-rc3.
> 
> This sleep should not be required on normal circuitry provided the
> pull-ups on the bus are correctly adapted to the slaves.  Unfortunately,
> this is not always the case.  The sleep is restored but as a parameter to
> the probe function in the pdata.
> 
> Reported-by: Paul Parsons <lost.distance@yahoo.com>
> Tested-by: Paul Parsons <lost.distance@yahoo.com>
> Signed-off-by: Jean-Fran_ois Dagenais <dagenaisj@sonatest.com>
again, the '_' 
> Cc: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
> drivers/mfd/asic3.c        |    1 +
> drivers/mfd/htc-pasic3.c   |    1 +
> drivers/w1/masters/ds1wm.c |    5 +++++
> include/linux/mfd/ds1wm.h  |    7 +++++++
> 4 files changed, 14 insertions(+)
> 
> diff -puN drivers/mfd/asic3.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/mfd/asic3.c
> --- a/drivers/mfd/asic3.c~w1-ds1wm-add-a-reset-recovery-parameter
> +++ a/drivers/mfd/asic3.c
> @@ -619,6 +619,7 @@ static void asic3_clk_disable(struct asi
> /* MFD cells (SPI, PWM, LED, DS1WM, MMC) */
> static struct ds1wm_driver_data ds1wm_pdata = {
> 	.active_high = 1,
> +	.reset_recover_delay = 1,
> };
> 
> static struct resource ds1wm_resources[] = {
> diff -puN drivers/mfd/htc-pasic3.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/mfd/htc-pasic3.c
> --- a/drivers/mfd/htc-pasic3.c~w1-ds1wm-add-a-reset-recovery-parameter
> +++ a/drivers/mfd/htc-pasic3.c
> @@ -99,6 +99,7 @@ static int ds1wm_disable(struct platform
> 
> static struct ds1wm_driver_data ds1wm_pdata = {
> 	.active_high = 0,
> +	.reset_recover_delay = 1,
> };
> 
> static struct resource ds1wm_resources[] __initdata = {
> diff -puN drivers/w1/masters/ds1wm.c~w1-ds1wm-add-a-reset-recovery-parameter drivers/w1/masters/ds1wm.c
> --- a/drivers/w1/masters/ds1wm.c~w1-ds1wm-add-a-reset-recovery-parameter
> +++ a/drivers/w1/masters/ds1wm.c
> @@ -109,6 +109,7 @@ struct ds1wm_data {
> 	/* byte to write that makes all intr disabled, */
> 	/* considering active_state (IAS) (optimization) */
> 	u8       int_en_reg_none;
> +	unsigned int reset_recover_delay; /* see ds1wm.h */
> };
> 
> static inline void ds1wm_write_register(struct ds1wm_data *ds1wm_data, u32 reg,
> @@ -187,6 +188,9 @@ static int ds1wm_reset(struct ds1wm_data
> 		return 1;
> 	}
> 
> +	if(ds1wm_data->reset_recover_delay)
> +		msleep(ds1wm_data->reset_recover_delay);
> +
> 	return 0;
> }
> 
> @@ -490,6 +494,7 @@ static int ds1wm_probe(struct platform_d
> 	}
> 	ds1wm_data->irq = res->start;
> 	ds1wm_data->int_en_reg_none = (plat->active_high ? DS1WM_INTEN_IAS : 0);
> +	ds1wm_data->reset_recover_delay = plat->reset_recover_delay;
> 
> 	if (res->flags & IORESOURCE_IRQ_HIGHEDGE)
> 		irq_set_irq_type(ds1wm_data->irq, IRQ_TYPE_EDGE_RISING);
> diff -puN include/linux/mfd/ds1wm.h~w1-ds1wm-add-a-reset-recovery-parameter include/linux/mfd/ds1wm.h
> --- a/include/linux/mfd/ds1wm.h~w1-ds1wm-add-a-reset-recovery-parameter
> +++ a/include/linux/mfd/ds1wm.h
> @@ -3,4 +3,11 @@
> struct ds1wm_driver_data {
> 	int active_high;
> 	int clock_rate;
> +	/* in milliseconds, the amount of time to */
> +	/* sleep following a reset pulse. Zero    */
> +	/* should work if your bus devices recover*/
> +	/* time respects the 1-wire spec since the*/
> +	/* ds1wm implements the precise timings of*/
> +	/* a reset pulse/presence detect sequence.*/ 
> +	unsigned int reset_recover_delay;
> };
> _
> 
>

Jean-Francois Dagenais 
Software Architect - B.Sc.A


Experience the new veo phased array flaw detector at www.sonatestveo.com.


Sonatest AP
2900 chemin des Quatre-Bourgeois
Bureau 305
Quebec City Quebec G1V 1Y4

T| +1 (418) 683 6222 x106   F| +1 (418) 683 7032   M|    W| www.sonatest.com



This message (and any associated files) is intended only for the use of akpm@linux-foundation.org, linux-kernel@vger.kernel.org and may contain information that is confidential, subject to copyright or constitutes a trade secret. If you are not the above recipient(s) you are hereby notified that any dissemination, copying or distribution of this message, or files associated with this message, is strictly prohibited. If you have received this message in error, please notify us immediately by replying to the message and deleting it from your computer. Any views or opinions presented are solely those of the author and do not necessarily represent those of the company.

Think green - help the environment by not printing this email.

Re: [PATCH] w1: ds1wm: add a reset recovery parameter

[Andrew Morton]

On Wed, 6 Jul 2011 09:16:57 -0400
Jean-François Dagenais <dagenaisj@Sonatest.com> wrote:

> > From: Jean-Fran_ois Dagenais <dagenaisj@sonatest.com>
> If the c-cedilla doesn't work please replace the '_' by a regular 'c'... actually I will re-submit the patch as v2.

It seems that a python script whcih Uwe wrote for me has broken.  Let
me see if I can send it back for repair..