2010-01-21 12:26:40

by Stanislaw Gruszka

[permalink] [raw]
Subject: [PATCH 2.6.{32,31}] iwlwifi: power up all devices for EEPROM read

commit f8701fe3aec24fcfb0dfa19aab47904611f96daf upstream

On 2.6.31 and 2.6.32 iwl3945 crash during kdump, I discovered
above upstream commit fix that.

Signed-off-by: Stanislaw Gruszka <[email protected]>
---
I tested patch on 2.6.32 with iwl3945 - it fix kdump, and on 2.6.31
with iwl5300 - no regression notice.

drivers/net/wireless/iwlwifi/iwl-eeprom.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-eeprom.c b/drivers/net/wireless/iwlwifi/iwl-eeprom.c
index 18dc3a4..870c0db 100644
--- a/drivers/net/wireless/iwlwifi/iwl-eeprom.c
+++ b/drivers/net/wireless/iwlwifi/iwl-eeprom.c
@@ -486,6 +486,7 @@ int iwl_eeprom_init(struct iwl_priv *priv)
}
e = (__le16 *)priv->eeprom;

+ priv->cfg->ops->lib->apm_ops.init(priv);
ret = priv->cfg->ops->lib->eeprom_ops.verify_signature(priv);
if (ret < 0) {
IWL_ERR(priv, "EEPROM not found, EEPROM_GP=0x%08x\n", gp);
@@ -556,6 +557,8 @@ done:
err:
if (ret)
iwl_eeprom_free(priv);
+ /* Reset chip to save power until we load uCode during "up". */
+ priv->cfg->ops->lib->apm_ops.stop(priv);
alloc_err:
return ret;
}
--
1.6.2.5



2010-01-22 07:45:53

by Stanislaw Gruszka

[permalink] [raw]
Subject: Re: [PATCH 2.6.{32,31}] iwlwifi: power up all devices for EEPROM read

Hi

On Thu, Jan 21, 2010 at 10:02:24AM -0800, reinette chatre wrote:
> The 2.6.32 and 2.6.31 code you are changing here adds a device "power
> up" call into iwl_eeprom_init. This is not necessary since as you can
> see, in iwl3945_pci_probe this exact same call is made right before
> iwl_eeprom_init is called.

Sorry, missed that. I should wrote in the changelog that this is backport
of more commits and remove power up stuff from pci proble as well.

> >
> > On 2.6.31 and 2.6.32 iwl3945 crash during kdump, I discovered
> > above upstream commit fix that.
>
> This does not make sense. Was this testing done in the stable kernel
> from linux-2.6-stable or perhaps a kernel that had those other power
> saving patches backported?

I tested backported patch on 2.6.32.4. It fix kdump, this issue is
100% reproducible on my laptop with iwl3945.

The functional change introduced in this patch is we disable device
after EEPROM read. I think that make difference with kdump. Not sure
if this right fix or just workaround.

Debugging kdump kernel is somehow limited, I have no netconsole or serial
console option. I can only see last part of call trace where are iwlwifi
functions. I'm going dig into kernel to make calltrace shorter and see
where exact the oops is.

Cheers
Stanislaw

2010-01-21 21:12:03

by Greg KH

[permalink] [raw]
Subject: Re: [stable] [PATCH 2.6.{32, 31}] iwlwifi: power up all devices for EEPROM read

On Thu, Jan 21, 2010 at 12:59:55PM -0800, reinette chatre wrote:
> Hi Greg,
>
> On Thu, 2010-01-21 at 12:51 -0800, Greg KH wrote:
> > On Thu, Jan 21, 2010 at 01:22:25PM +0100, Stanislaw Gruszka wrote:
> > > commit f8701fe3aec24fcfb0dfa19aab47904611f96daf upstream
> > >
> > > On 2.6.31 and 2.6.32 iwl3945 crash during kdump, I discovered
> > > above upstream commit fix that.
> > >
> > > Signed-off-by: Stanislaw Gruszka <[email protected]>
> > > ---
> > > I tested patch on 2.6.32 with iwl3945 - it fix kdump, and on 2.6.31
> > > with iwl5300 - no regression notice.
> >
> > Thanks for the backport, I've queued this up now.
>
> This backport is on top of patches that are not present in 2.6.31 or
> 2.6.32. Please see the message I sent in response to the original post.
> Could we please finish that discussion first before looking into merging
> this patch?

Oops, ok, I'll go drop it, thanks for the quick response.

greg k-h

2010-01-21 18:02:26

by Reinette Chatre

[permalink] [raw]
Subject: Re: [PATCH 2.6.{32,31}] iwlwifi: power up all devices for EEPROM read

Hi Stanislaw,

On Thu, 2010-01-21 at 04:22 -0800, Stanislaw Gruszka wrote:
> commit f8701fe3aec24fcfb0dfa19aab47904611f96daf upstream

This is strange. As the commit you refer to indicate, it fixes problems
introduced by "iwlwifi: remove power-wasting calls to apm_ops.init()"
and "iwlagn: power up device before initializing EEPROM" - these patches
have only been introduced in 2.6.33, so targeting this patch for earlier
kernels does not seem right.

Note that what the abovementioned patches did was move the device "power
up" routines from probe to be closer to where they are needed
(iwl_eeprom_init) in an effort to optimize power usage.

In the code you are changing, the above patches are not present, so the
original behavior of device power up during probe is still present.

The 2.6.32 and 2.6.31 code you are changing here adds a device "power
up" call into iwl_eeprom_init. This is not necessary since as you can
see, in iwl3945_pci_probe this exact same call is made right before
iwl_eeprom_init is called.

>
> On 2.6.31 and 2.6.32 iwl3945 crash during kdump, I discovered
> above upstream commit fix that.

This does not make sense. Was this testing done in the stable kernel
from linux-2.6-stable or perhaps a kernel that had those other power
saving patches backported?

>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> I tested patch on 2.6.32 with iwl3945 - it fix kdump, and on 2.6.31
> with iwl5300 - no regression notice.
>
> drivers/net/wireless/iwlwifi/iwl-eeprom.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/iwlwifi/iwl-eeprom.c b/drivers/net/wireless/iwlwifi/iwl-eeprom.c
> index 18dc3a4..870c0db 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-eeprom.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-eeprom.c
> @@ -486,6 +486,7 @@ int iwl_eeprom_init(struct iwl_priv *priv)
> }
> e = (__le16 *)priv->eeprom;
>
> + priv->cfg->ops->lib->apm_ops.init(priv);
> ret = priv->cfg->ops->lib->eeprom_ops.verify_signature(priv);
> if (ret < 0) {
> IWL_ERR(priv, "EEPROM not found, EEPROM_GP=0x%08x\n", gp);
> @@ -556,6 +557,8 @@ done:
> err:
> if (ret)
> iwl_eeprom_free(priv);
> + /* Reset chip to save power until we load uCode during "up". */
> + priv->cfg->ops->lib->apm_ops.stop(priv);
> alloc_err:
> return ret;
> }

Reinette



2010-01-21 20:59:56

by Reinette Chatre

[permalink] [raw]
Subject: Re: [stable] [PATCH 2.6.{32, 31}] iwlwifi: power up all devices for EEPROM read

Hi Greg,

On Thu, 2010-01-21 at 12:51 -0800, Greg KH wrote:
> On Thu, Jan 21, 2010 at 01:22:25PM +0100, Stanislaw Gruszka wrote:
> > commit f8701fe3aec24fcfb0dfa19aab47904611f96daf upstream
> >
> > On 2.6.31 and 2.6.32 iwl3945 crash during kdump, I discovered
> > above upstream commit fix that.
> >
> > Signed-off-by: Stanislaw Gruszka <[email protected]>
> > ---
> > I tested patch on 2.6.32 with iwl3945 - it fix kdump, and on 2.6.31
> > with iwl5300 - no regression notice.
>
> Thanks for the backport, I've queued this up now.

This backport is on top of patches that are not present in 2.6.31 or
2.6.32. Please see the message I sent in response to the original post.
Could we please finish that discussion first before looking into merging
this patch?

Thank you

Reinette



2010-01-21 20:55:03

by Greg KH

[permalink] [raw]
Subject: Re: [stable] [PATCH 2.6.{32, 31}] iwlwifi: power up all devices for EEPROM read

On Thu, Jan 21, 2010 at 01:22:25PM +0100, Stanislaw Gruszka wrote:
> commit f8701fe3aec24fcfb0dfa19aab47904611f96daf upstream
>
> On 2.6.31 and 2.6.32 iwl3945 crash during kdump, I discovered
> above upstream commit fix that.
>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> I tested patch on 2.6.32 with iwl3945 - it fix kdump, and on 2.6.31
> with iwl5300 - no regression notice.

Thanks for the backport, I've queued this up now.

greg k-h