Hi,
Here are the iwlwifi patches vital for 2.6.27. Please apply.
Thanks,
-yi
[PATCH 01/11] iwlwifi: W/A for the TSF correction in IBSS
[PATCH 02/11] iwlwifi: use mac80211 band and channel settings
[PATCH 03/11] iwlwifi: fix hidden ssid discovery in passive channels
[PATCH 04/11] iwlwifi: remove false rxon if rx chain changes
[PATCH 05/11] iwlwifi: workaround interrupt handling no some platforms
[PATCH 06/11] iwlwifi: fix apm_stop
[PATCH 07/11] iwlwifi: generic init calibrations framework
[PATCH 08/11] iwlwifi: call apm stop on exit
[PATCH 09/11] iwlwifi: fix station mimo power save values
[PATCH 10/11] iwlwifi: fix rx_chain computation
[PATCH 11/11] iwlwifi: fix 64bit platform firmware loading
Hi Yi,
> This patch adds workaround for an interrupt related hardware bug on
> some platforms.
this one should go in, but since this is a workaround, I would expect
some more details why we did this. Either in the commit message or the
comment and it is a good idea to be specific. What platforms. At least
give an example.
Regards
Marcel
On Thu, Aug 28, 2008 at 3:33 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Yi,
>
>> The patch fixes CSR_GP_CNTRL_REG_FLAG_INIT_DONE was set instead of
>> cleared which disabled moving device to D0U state.
>
> when was this mistake introduced. Is it a regression compared to what we
> have in 2.6.26?
Again. think of global warming.
Tomas
Hi Tomas,
> >> This patch fixes a critical bug that only the last calibration result
> >> was applied. On reception of one calibration result all the calibration
> >> was freed. The patch fixes this problem by introducing a generic init
> >> calibration framework which allows variable number of init calibrations
> >> and allows addition new HW.
> >
> > a lot of code changes in -rc4 phase. Please give more details since the
> > patch changes a lot and even introduces a new EXPORT.
>
> The first sentence is important, the whole framework was buggy.
I got that part, but understand that just changing a framework at the
state of -rc4 is never a good idea. Any possible regression that might
come out of it, could delay the release.
In this case it would actually help to have clear Reviewed-by or
Tested-by statements in the patch. Signed-off only states the authors
and doesn't give this change more credit.
Regards
Marcel
On Thu, Aug 28, 2008 at 3:23 PM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2008-08-28 at 14:21 +0300, Tomas Winkler wrote:
>> On Thu, Aug 28, 2008 at 3:33 PM, Marcel Holtmann <[email protected]> wrote:
>> > Hi Yi,
>> >
>> >> The patch fixes CSR_GP_CNTRL_REG_FLAG_INIT_DONE was set instead of
>> >> cleared which disabled moving device to D0U state.
>> >
>> > when was this mistake introduced. Is it a regression compared to what we
>> > have in 2.6.26?
>>
>> Again. think of global warming.
>
> If you're so concerned about global warming, why didn't you fix this,
> umm, months ago? :)
Why I didn't fix everything in the single point of time. 'In the
beginning there was a driver'
>
> Frankly, this sounds like a "quick, somebody help me come up with a good
> argument" argument.
So how can I really reason this :)
Tomas
Hi Tomas,
> >> > The patch fixes CSR_GP_CNTRL_REG_FLAG_INIT_DONE was set instead of
> >> > cleared which disabled moving device to D0U state.
> >>
> >> when was this mistake introduced. Is it a regression compared to what we
> >> have in 2.6.26?
> >
> > I think you are overstretching the regression-fixes-only rule a little bit...
> > It will help nobody, if we don't fix actual _bugs_ and push such fixes
> > into the next merge window.
> >
> > Our goal is to have a stable kernel on release. We don't get to that goal, if
> > we defer bugfixes to the next merge window.
>
> Oh, good I have friends after all :)
this is not about that and you don't have to convince me to get these
patches included. The line of command is John -> Dave -> Linus. I
mentioned it in the other email, that I don't see any problem with these
patches at all. They look all good, but the question here is if they are
suitable for -rc4 or not.
If we keep sending bug fixes that are no clear regression after -rc4,
then we likely have -rc5, -rc6, -rc7 and more. Instead -rc5 could have
been the last. Linus point was clear here. After the merge window it
goes to regression fixing with the goal to release the next kernel
quickly.
Personally I would take all patches, but if I would be in the position
to explain them to Linus, I would reject them. Just see this review as
Linus asking the trick question why he should take it.
Regards
Marcel
On Thursday 28 August 2008, Marcel Holtmann wrote:
> Hi Yi,
>
> > The patch fixes CSR_GP_CNTRL_REG_FLAG_INIT_DONE was set instead of
> > cleared which disabled moving device to D0U state.
>
> when was this mistake introduced. Is it a regression compared to what we
> have in 2.6.26?
I think you are overstretching the regression-fixes-only rule a little bit...
It will help nobody, if we don't fix actual _bugs_ and push such fixes
into the next merge window.
Our goal is to have a stable kernel on release. We don't get to that goal, if
we defer bugfixes to the next merge window.
On Thu, Aug 28, 2008 at 5:02 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Tomas,
>
>> >> This patch fixes the wrong use of self mimo power save values instead of
>> >> peer's mimo power save values.
>> >
>> > please state the impact of not fixing this. When does this matter. Does
>> > the both sides have to enable this feature and things like this.
>>
>> Okay, we might not be able to do any traffic to the [peer station
>> because by mistake we configured TX to according our RX capability not
>> RX capability of the peer station.
>
> that would have been a good addition to the commit message. That the
> result of this issue is no TX, belongs in it.
But isn't it stating obvious.
Tomas
>
On Thu, Aug 28, 2008 at 3:29 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Yi,
>
>> This patch gives the HW the possibility to send direct probes in passive
>> channels (as long as traffic was detected in that channel) if such ssid
>> was requested, so hidden ssid can be found now in those channels as well.
>
> this also doesn't sound like a regression to me. Nice to have for sure,
> but not acceptable according to the merge window rules.
If you cannot scan your AP you also not able to connect to it, drawing
your card useless.
> Question here is was the driver supporting this before and we broke it
> in the 2.6.27 phase or not.
Irrelevant, this is fixing show stopper bug.
Tomas
Hi Tomas,
> >> This patch fixes the wrong use of self mimo power save values instead of
> >> peer's mimo power save values.
> >
> > please state the impact of not fixing this. When does this matter. Does
> > the both sides have to enable this feature and things like this.
>
> Okay, we might not be able to do any traffic to the [peer station
> because by mistake we configured TX to according our RX capability not
> RX capability of the peer station.
that would have been a good addition to the commit message. That the
result of this issue is no TX, belongs in it.
Regards
Marcel
Hi Tomas,
> >> The patch fixes CSR_GP_CNTRL_REG_FLAG_INIT_DONE was set instead of
> >> cleared which disabled moving device to D0U state.
> >
> > when was this mistake introduced. Is it a regression compared to what we
> > have in 2.6.26?
>
> Again. think of global warming.
I fully get why we wanna fix that. That was not my questions. Is this a
regression compared to what we have in 2.6.26? Yes or no. A clear yes
make this a regression and we will not even discuss this any further.
That patch should go in then. In the no case you have to come up with
some good argument.
Remember that I reviewed all patches under the strict after merge window
rules. There is always a gray area, but please give good argumentation
for it in the commit message.
And again, if I would be in Dave shoes, I would reject this one.
Also it would be a bad idea to just keep sending patches and then have
John forward them to Dave. At some point this backfires. Just split
these up into 2.6.27 and 2.6.28 patches and give them to John.
Regards
Marcel
On Thu, Aug 28, 2008 at 3:36 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Yi,
>
>> This patch fixes a critical bug that only the last calibration result
>> was applied. On reception of one calibration result all the calibration
>> was freed. The patch fixes this problem by introducing a generic init
>> calibration framework which allows variable number of init calibrations
>> and allows addition new HW.
>
> a lot of code changes in -rc4 phase. Please give more details since the
> patch changes a lot and even introduces a new EXPORT.
The first sentence is important, the whole framework was buggy.
Tomas
From: Tomas Winkler <[email protected]>
This patch fixes loading firmware from memory above 32bit.
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Emmanuel Grumbach <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-5000.c | 11 ++++-------
drivers/net/wireless/iwlwifi/iwl-fh.h | 1 +
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-5000.c b/drivers/net/wireless/iwlwifi/iwl-5000.c
index e04a26f..79ff288 100644
--- a/drivers/net/wireless/iwlwifi/iwl-5000.c
+++ b/drivers/net/wireless/iwlwifi/iwl-5000.c
@@ -533,14 +533,11 @@ static int iwl5000_load_section(struct iwl_priv *priv,
FH_TFDIB_CTRL0_REG(FH_SRVC_CHNL),
phy_addr & FH_MEM_TFDIB_DRAM_ADDR_LSB_MSK);
- /* FIME: write the MSB of the phy_addr in CTRL1
- * iwl_write_direct32(priv,
- IWL_FH_TFDIB_CTRL1_REG(IWL_FH_SRVC_CHNL),
- ((phy_addr & MSB_MSK)
- << FH_MEM_TFDIB_REG1_ADDR_BITSHIFT) | byte_count);
- */
iwl_write_direct32(priv,
- FH_TFDIB_CTRL1_REG(FH_SRVC_CHNL), byte_cnt);
+ FH_TFDIB_CTRL1_REG(FH_SRVC_CHNL),
+ (iwl_get_dma_hi_address(phy_addr)
+ << FH_MEM_TFDIB_REG1_ADDR_BITSHIFT) | byte_cnt);
+
iwl_write_direct32(priv,
FH_TCSR_CHNL_TX_BUF_STS_REG(FH_SRVC_CHNL),
1 << FH_TCSR_CHNL_TX_BUF_STS_REG_POS_TB_NUM |
diff --git a/drivers/net/wireless/iwlwifi/iwl-fh.h b/drivers/net/wireless/iwlwifi/iwl-fh.h
index 9446424..cd11c0c 100644
--- a/drivers/net/wireless/iwlwifi/iwl-fh.h
+++ b/drivers/net/wireless/iwlwifi/iwl-fh.h
@@ -287,6 +287,7 @@
#define FH_RSSR_CHNL0_RX_STATUS_CHNL_IDLE (0x01000000)
+#define FH_MEM_TFDIB_REG1_ADDR_BITSHIFT 28
/**
* Transmit DMA Channel Control/Status Registers (TCSR)
--
1.5.3.6
On Thu, Aug 28, 2008 at 3:30 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Yi,
>
>> Rx chain might change during power save transitions but it doesn't
>> requires full rxon. The patch avoids the rxon by removing the rx_chain
>> check.
>
> and again. Is this a regression. What is the down side or impact from
> not doing this. Did we used to do it different in 2.6.26?
Yes this one is regression this is new code and causes disconnection.
Tomas
From: Ron Rindjunsky <[email protected]>
This patch fixes the wrong use of self mimo power save values instead of
peer's mimo power save values.
Signed-off-by: Ron Rindjunsky <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn-rs.c | 3 ++-
drivers/net/wireless/iwlwifi/iwl-agn.c | 2 --
drivers/net/wireless/iwlwifi/iwl-dev.h | 1 -
3 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-rs.c b/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
index 754fef5..90a2b6d 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
@@ -1153,7 +1153,8 @@ static int rs_switch_to_mimo2(struct iwl_priv *priv,
!sta->ht_info.ht_supported)
return -1;
- if (priv->current_ht_config.tx_mimo_ps_mode == IWL_MIMO_PS_STATIC)
+ if (((sta->ht_info.cap & IEEE80211_HT_CAP_MIMO_PS) >> 2)
+ == IWL_MIMO_PS_STATIC)
return -1;
/* Need both Tx chains/antennas to support MIMO */
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index b1cefb5..01db6d3 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -586,8 +586,6 @@ static void iwl4965_ht_conf(struct iwl_priv *priv,
iwl_conf->supported_chan_width = 0;
}
- iwl_conf->tx_mimo_ps_mode =
- (u8)((ht_conf->cap & IEEE80211_HT_CAP_MIMO_PS) >> 2);
memcpy(iwl_conf->supp_mcs_set, ht_conf->supp_mcs_set, 16);
iwl_conf->control_channel = ht_bss_conf->primary_channel;
diff --git a/drivers/net/wireless/iwlwifi/iwl-dev.h b/drivers/net/wireless/iwlwifi/iwl-dev.h
index b00acf4..deec634 100644
--- a/drivers/net/wireless/iwlwifi/iwl-dev.h
+++ b/drivers/net/wireless/iwlwifi/iwl-dev.h
@@ -412,7 +412,6 @@ struct iwl_ht_info {
/* self configuration data */
u8 is_ht;
u8 supported_chan_width;
- u16 tx_mimo_ps_mode;
u8 is_green_field;
u8 sgf; /* HT_SHORT_GI_* short guard interval */
u8 max_amsdu_size;
--
1.5.3.6
From: Mohamed Abbas <[email protected]>
The patch fixes CSR_GP_CNTRL_REG_FLAG_INIT_DONE was set instead of
cleared which disabled moving device to D0U state.
Signed-off-by: Mohamed Abbas <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-4965.c | 4 ++--
drivers/net/wireless/iwlwifi/iwl-5000.c | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-4965.c b/drivers/net/wireless/iwlwifi/iwl-4965.c
index e258122..23fed32 100644
--- a/drivers/net/wireless/iwlwifi/iwl-4965.c
+++ b/drivers/net/wireless/iwlwifi/iwl-4965.c
@@ -474,8 +474,8 @@ static void iwl4965_apm_stop(struct iwl_priv *priv)
iwl_set_bit(priv, CSR_RESET, CSR_RESET_REG_FLAG_SW_RESET);
udelay(10);
-
- iwl_set_bit(priv, CSR_GP_CNTRL, CSR_GP_CNTRL_REG_FLAG_INIT_DONE);
+ /* clear "init complete" move adapter D0A* --> D0U state */
+ iwl_clear_bit(priv, CSR_GP_CNTRL, CSR_GP_CNTRL_REG_FLAG_INIT_DONE);
spin_unlock_irqrestore(&priv->lock, flags);
}
diff --git a/drivers/net/wireless/iwlwifi/iwl-5000.c b/drivers/net/wireless/iwlwifi/iwl-5000.c
index cbc01a0..d95fb42 100644
--- a/drivers/net/wireless/iwlwifi/iwl-5000.c
+++ b/drivers/net/wireless/iwlwifi/iwl-5000.c
@@ -145,7 +145,8 @@ static void iwl5000_apm_stop(struct iwl_priv *priv)
udelay(10);
- iwl_set_bit(priv, CSR_GP_CNTRL, CSR_GP_CNTRL_REG_FLAG_INIT_DONE);
+ /* clear "init complete" move adapter D0A* --> D0U state */
+ iwl_clear_bit(priv, CSR_GP_CNTRL, CSR_GP_CNTRL_REG_FLAG_INIT_DONE);
spin_unlock_irqrestore(&priv->lock, flags);
}
--
1.5.3.6
Hi Thomas,
> >> This patch gives the HW the possibility to send direct probes in passive
> >> channels (as long as traffic was detected in that channel) if such ssid
> >> was requested, so hidden ssid can be found now in those channels as well.
> >
> > this also doesn't sound like a regression to me. Nice to have for sure,
> > but not acceptable according to the merge window rules.
>
> If you cannot scan your AP you also not able to connect to it, drawing
> your card useless.
>
> > Question here is was the driver supporting this before and we broke it
> > in the 2.6.27 phase or not.
>
> Irrelevant, this is fixing show stopper bug.
it is relevant. So I read this commit message as, that now we can also
find hidden SSIDs in passive channels. Question that you should ask then
is if that was supported before. Yes or no. If yes, then it is a
regression, no then it is a feature. And regression fixes go in features
wait for the next merge window.
The term show stopper bug is irrelevant when it comes to merge window
rules.
Regards
Marcel
On Thu, Aug 28, 2008 at 3:56 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Johannes,
>
>> > > This patch calls apm stop on exit and suspend and sets STATUS_EXIT_PENDING
>> > > accordingly. Without this patch hardware consumes power even after driver
>> > > is removed or suspended.
>> >
>> > is this a regression to 2.6.26? That is always the important question
>> > here since that will determine if the patch should go in. If the 2.6.26
>> > version did this better (even if not prefect), then this patch should go
>> > on, otherwise it could wait until next merge window.
>>
>> AFAIR people have been complaining about this behaviour _forever_, so
>> it's hardly a regression.
>
> I think so too. However the question is still if 2.6.26 maybe did better
> and we are now doing worse because of other patches. This needs to be in
> the commit message. Otherwise this falls for me in the next merge window
> category.
>
Regression or not, this is an important bug fix. With millions of
laptops sold this is significant contribution to global worming.
Tomas
Hi Tomas,
> >> This patch makes use of mac80211 defaults for channel and band settings
> >> instead of setting them by the driver own. The iwlwifi driver used to
> >> set G band channel 6 as the default channel.
> >
> > that is not a regression. The driver has been doing this until now and
> > thus it can keep doing it until the next merge window.
> >
> > If you disagree, then please explain the regression in the commit
> > message.
>
> It's trashing initial configuration as two different configuration
> were applied in the initial flow. Frankly I don't remember what bug we
> had this with. Maybe it's not so urgent.
if it is not worse than what we had in 2.6.26, then it should clearly
wait. I fail to see how it could be a regression, but you are the expert
with this hardware.
Regards
Marcel
Hi Yi,
> Rx chain might change during power save transitions but it doesn't
> requires full rxon. The patch avoids the rxon by removing the rx_chain
> check.
and again. Is this a regression. What is the down side or impact from
not doing this. Did we used to do it different in 2.6.26?
Regards
Marcel
From: Ron Rindjunsky <[email protected]>
This patch gives the HW the possibility to send direct probes in passive
channels (as long as traffic was detected in that channel) if such ssid
was requested, so hidden ssid can be found now in those channels as well.
Signed-off-by: Cahill Ben <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Ron Rindjunsky <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-scan.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-scan.c b/drivers/net/wireless/iwlwifi/iwl-scan.c
index 9bb6adb..6c8ac3a 100644
--- a/drivers/net/wireless/iwlwifi/iwl-scan.c
+++ b/drivers/net/wireless/iwlwifi/iwl-scan.c
@@ -421,7 +421,7 @@ static int iwl_get_channels_for_scan(struct iwl_priv *priv,
else
scan_ch->type = SCAN_CHANNEL_TYPE_ACTIVE;
- if ((scan_ch->type & SCAN_CHANNEL_TYPE_ACTIVE) && n_probes)
+ if (n_probes)
scan_ch->type |= IWL_SCAN_PROBE_MASK(n_probes);
scan_ch->active_dwell = cpu_to_le16(active_dwell);
--
1.5.3.6
Hi Tomas,
> >> >> This patch gives the HW the possibility to send direct probes in passive
> >> >> channels (as long as traffic was detected in that channel) if such ssid
> >> >> was requested, so hidden ssid can be found now in those channels as well.
> >> >
> >> > this also doesn't sound like a regression to me. Nice to have for sure,
> >> > but not acceptable according to the merge window rules.
> >>
> >> If you cannot scan your AP you also not able to connect to it, drawing
> >> your card useless.
> >>
> >> > Question here is was the driver supporting this before and we broke it
> >> > in the 2.6.27 phase or not.
> >>
> >> Irrelevant, this is fixing show stopper bug.
> >
> > it is relevant. So I read this commit message as, that now we can also
> > find hidden SSIDs in passive channels. Question that you should ask then
> > is if that was supported before. Yes or no. If yes, then it is a
> > regression, no then it is a feature. And regression fixes go in features
> > wait for the next merge window.
> >
> > The term show stopper bug is irrelevant when it comes to merge window
> > rules.
> >
> I don't know if it worked before or not I didn't check. This is new code.
is this new code for new hardware or did we also use it in the 4965
case? And yes, we need to check 2.6.26 here. And then state this clearly
in the commit message.
Please also see this from the point of people that actually have to or
wanna backport patches. The better the commit message, the easier their
job to make good decisions on which patches to take.
Regards
Marcel
From: Tomas Winkler <[email protected]>
This patch fixes and simplifies rx_chain computation.
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Mohamed Abbas <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-core.c | 77 ++++++++++++++++++------------
1 files changed, 46 insertions(+), 31 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index 2b89c7b..a3b496e 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -592,12 +592,11 @@ static void iwlcore_free_geos(struct iwl_priv *priv)
clear_bit(STATUS_GEO_CONFIGURED, &priv->status);
}
-static u8 is_single_rx_stream(struct iwl_priv *priv)
+static bool is_single_rx_stream(struct iwl_priv *priv)
{
return !priv->current_ht_config.is_ht ||
((priv->current_ht_config.supp_mcs_set[1] == 0) &&
- (priv->current_ht_config.supp_mcs_set[2] == 0)) ||
- priv->ps_mode == IWL_MIMO_PS_STATIC;
+ (priv->current_ht_config.supp_mcs_set[2] == 0));
}
static u8 iwl_is_channel_extension(struct iwl_priv *priv,
@@ -704,33 +703,39 @@ EXPORT_SYMBOL(iwl_set_rxon_ht);
* MIMO (dual stream) requires at least 2, but works better with 3.
* This does not determine *which* chains to use, just how many.
*/
-static int iwlcore_get_rx_chain_counter(struct iwl_priv *priv,
- u8 *idle_state, u8 *rx_state)
+static int iwl_get_active_rx_chain_count(struct iwl_priv *priv)
{
- u8 is_single = is_single_rx_stream(priv);
- u8 is_cam = test_bit(STATUS_POWER_PMI, &priv->status) ? 0 : 1;
+ bool is_single = is_single_rx_stream(priv);
+ bool is_cam = !test_bit(STATUS_POWER_PMI, &priv->status);
/* # of Rx chains to use when expecting MIMO. */
if (is_single || (!is_cam && (priv->ps_mode == IWL_MIMO_PS_STATIC)))
- *rx_state = 2;
+ return 2;
else
- *rx_state = 3;
+ return 3;
+}
+static int iwl_get_idle_rx_chain_count(struct iwl_priv *priv, int active_cnt)
+{
+ int idle_cnt;
+ bool is_cam = !test_bit(STATUS_POWER_PMI, &priv->status);
/* # Rx chains when idling and maybe trying to save power */
switch (priv->ps_mode) {
case IWL_MIMO_PS_STATIC:
case IWL_MIMO_PS_DYNAMIC:
- *idle_state = (is_cam) ? 2 : 1;
+ idle_cnt = (is_cam) ? 2 : 1;
break;
case IWL_MIMO_PS_NONE:
- *idle_state = (is_cam) ? *rx_state : 1;
+ idle_cnt = (is_cam) ? active_cnt : 1;
break;
+ case IWL_MIMO_PS_INVALID:
default:
- *idle_state = 1;
+ IWL_ERROR("invalide mimo ps mode %d\n", priv->ps_mode);
+ WARN_ON(1);
+ idle_cnt = -1;
break;
}
-
- return 0;
+ return idle_cnt;
}
/**
@@ -741,34 +746,44 @@ static int iwlcore_get_rx_chain_counter(struct iwl_priv *priv,
*/
void iwl_set_rxon_chain(struct iwl_priv *priv)
{
- u8 is_single = is_single_rx_stream(priv);
- u8 idle_state, rx_state;
-
- priv->staging_rxon.rx_chain = 0;
- rx_state = idle_state = 3;
+ bool is_single = is_single_rx_stream(priv);
+ bool is_cam = !test_bit(STATUS_POWER_PMI, &priv->status);
+ u8 idle_rx_cnt, active_rx_cnt;
+ u16 rx_chain;
/* Tell uCode which antennas are actually connected.
* Before first association, we assume all antennas are connected.
* Just after first association, iwl_chain_noise_calibration()
* checks which antennas actually *are* connected. */
- priv->staging_rxon.rx_chain |=
- cpu_to_le16(priv->hw_params.valid_rx_ant <<
- RXON_RX_CHAIN_VALID_POS);
+ rx_chain = priv->hw_params.valid_rx_ant << RXON_RX_CHAIN_VALID_POS;
/* How many receivers should we use? */
- iwlcore_get_rx_chain_counter(priv, &idle_state, &rx_state);
- priv->staging_rxon.rx_chain |=
- cpu_to_le16(rx_state << RXON_RX_CHAIN_MIMO_CNT_POS);
- priv->staging_rxon.rx_chain |=
- cpu_to_le16(idle_state << RXON_RX_CHAIN_CNT_POS);
-
- if (!is_single && (rx_state >= 2) &&
- !test_bit(STATUS_POWER_PMI, &priv->status))
+ active_rx_cnt = iwl_get_active_rx_chain_count(priv);
+ idle_rx_cnt = iwl_get_idle_rx_chain_count(priv, active_rx_cnt);
+
+ /* correct rx chain count accoridng hw settings */
+ if (priv->hw_params.rx_chains_num < active_rx_cnt)
+ active_rx_cnt = priv->hw_params.rx_chains_num;
+
+ if (priv->hw_params.rx_chains_num < idle_rx_cnt)
+ idle_rx_cnt = priv->hw_params.rx_chains_num;
+
+ rx_chain |= active_rx_cnt << RXON_RX_CHAIN_MIMO_CNT_POS;
+ rx_chain |= idle_rx_cnt << RXON_RX_CHAIN_CNT_POS;
+
+ priv->staging_rxon.rx_chain = cpu_to_le16(rx_chain);
+
+ if (!is_single && (active_rx_cnt >= 2) && is_cam)
priv->staging_rxon.rx_chain |= RXON_RX_CHAIN_MIMO_FORCE_MSK;
else
priv->staging_rxon.rx_chain &= ~RXON_RX_CHAIN_MIMO_FORCE_MSK;
- IWL_DEBUG_ASSOC("rx chain %X\n", priv->staging_rxon.rx_chain);
+ IWL_DEBUG_ASSOC("rx_chain=0x%Xi active=%d idle=%d\n",
+ priv->staging_rxon.rx_chain,
+ active_rx_cnt, idle_rx_cnt);
+
+ WARN_ON(active_rx_cnt == 0 || idle_rx_cnt == 0 ||
+ active_rx_cnt < idle_rx_cnt);
}
EXPORT_SYMBOL(iwl_set_rxon_chain);
--
1.5.3.6
Hi Yi,
> This patch fixes and simplifies rx_chain computation.
come on, what does it fix? You are submitting patches for -rc4 kernels.
Regards
Marcel
On Thu, Aug 28, 2008 at 3:40 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Yi,
>
>> This patch fixes the wrong use of self mimo power save values instead of
>> peer's mimo power save values.
>
> please state the impact of not fixing this. When does this matter. Does
> the both sides have to enable this feature and things like this.
Okay, we might not be able to do any traffic to the [peer station
because by mistake we configured TX to according our RX capability not
RX capability of the peer station.
Tomas
Hi Yi,
> This patch fixes a critical bug that only the last calibration result
> was applied. On reception of one calibration result all the calibration
> was freed. The patch fixes this problem by introducing a generic init
> calibration framework which allows variable number of init calibrations
> and allows addition new HW.
a lot of code changes in -rc4 phase. Please give more details since the
patch changes a lot and even introduces a new EXPORT.
Regards
Marcel
On Thu, Aug 28, 2008 at 3:41 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Yi,
>
>> This patch fixes and simplifies rx_chain computation.
>
> come on, what does it fix? You are submitting patches for -rc4 kernels.
The fix is here
+ /* correct rx chain count accoridng hw settings */
+ if (priv->hw_params.rx_chains_num < active_rx_cnt)
+ active_rx_cnt = priv->hw_params.rx_chains_num;
+
+ if (priv->hw_params.rx_chains_num < idle_rx_cnt)
+ idle_rx_cnt = priv->hw_params.rx_chains_num;
+
We would configure 1x2 MIMO like 3x3 -- big trouble
Tomas
>
Hi Yi,
> This patch calls apm stop on exit and suspend and sets STATUS_EXIT_PENDING
> accordingly. Without this patch hardware consumes power even after driver
> is removed or suspended.
is this a regression to 2.6.26? That is always the important question
here since that will determine if the patch should go in. If the 2.6.26
version did this better (even if not prefect), then this patch should go
on, otherwise it could wait until next merge window.
Regards
Marcel
On Thu, Aug 28, 2008 at 5:09 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Tomas,
>
>> >> > > This patch calls apm stop on exit and suspend and sets STATUS_EXIT_PENDING
>> >> > > accordingly. Without this patch hardware consumes power even after driver
>> >> > > is removed or suspended.
>> >> >
>> >> > is this a regression to 2.6.26? That is always the important question
>> >> > here since that will determine if the patch should go in. If the 2.6.26
>> >> > version did this better (even if not prefect), then this patch should go
>> >> > on, otherwise it could wait until next merge window.
>> >>
>> >> AFAIR people have been complaining about this behaviour _forever_, so
>> >> it's hardly a regression.
>> >
>> > I think so too. However the question is still if 2.6.26 maybe did better
>> > and we are now doing worse because of other patches. This needs to be in
>> > the commit message. Otherwise this falls for me in the next merge window
>> > category.
>> >
>> Regression or not, this is an important bug fix. With millions of
>> laptops sold this is significant contribution to global worming.
>
> I fully agree with you here (no questions asked), but that is besides
> the point.
>
> The whole idea is to speed up the release cycle and every fix that is
> not a regression fix might have impacts. Also if it is not a regression,
> I can wait until the next merge window. I think that Linus made it
> pretty clear.
>
> So while I personally would vote for including this patch, it doesn't
> really fit the criteria for being submitted after -rc4. In the end this
> is up to Dave if he feels it is worth to explain this to Linus. There is
> a gray area.
>
> If you wanna play the "being green" card, then include numbers on how
> much power this would save :)
Okay so there won't be working iwlwifi driver in 2.6.27, it wasn't in
2.6.26 so this is not really regression.
I think you are exaggerating in exercising this rule.
Tomas
Hi Yi,
> This patch makes use of mac80211 defaults for channel and band settings
> instead of setting them by the driver own. The iwlwifi driver used to
> set G band channel 6 as the default channel.
that is not a regression. The driver has been doing this until now and
thus it can keep doing it until the next merge window.
If you disagree, then please explain the regression in the commit
message.
Regards
Marcel
Hi Yi,
> This patch fixes the wrong use of self mimo power save values instead of
> peer's mimo power save values.
please state the impact of not fixing this. When does this matter. Does
the both sides have to enable this feature and things like this.
Regards
Marcel
Hi Yi,
> This patch fixes loading firmware from memory above 32bit.
>
> Signed-off-by: Tomas Winkler <[email protected]>
> Signed-off-by: Emmanuel Grumbach <[email protected]>
> Signed-off-by: Zhu Yi <[email protected]>
Acked-by: Marcel Holtmann <[email protected]>
Regards
Marcel
On Thu, Aug 28, 2008 at 2:38 PM, Michael Buesch <[email protected]> wrote:
> On Thursday 28 August 2008, Marcel Holtmann wrote:
>> Hi Yi,
>>
>> > The patch fixes CSR_GP_CNTRL_REG_FLAG_INIT_DONE was set instead of
>> > cleared which disabled moving device to D0U state.
>>
>> when was this mistake introduced. Is it a regression compared to what we
>> have in 2.6.26?
>
> I think you are overstretching the regression-fixes-only rule a little bit...
> It will help nobody, if we don't fix actual _bugs_ and push such fixes
> into the next merge window.
>
> Our goal is to have a stable kernel on release. We don't get to that goal, if
> we defer bugfixes to the next merge window.
Oh, good I have friends after all :)
Tomas
From: Tomas Winkler <[email protected]>
This patch fixes a critical bug that only the last calibration result
was applied. On reception of one calibration result all the calibration
was freed. The patch fixes this problem by introducing a generic init
calibration framework which allows variable number of init calibrations
and allows addition new HW.
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Emmanuel Grumbach <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-5000-hw.h | 7 +++
drivers/net/wireless/iwlwifi/iwl-5000.c | 63 ++++------------------------
drivers/net/wireless/iwlwifi/iwl-calib.c | 60 ++++++++++++++++++++++++++
drivers/net/wireless/iwlwifi/iwl-core.c | 19 +--------
drivers/net/wireless/iwlwifi/iwl-core.h | 8 +++-
drivers/net/wireless/iwlwifi/iwl-dev.h | 14 +++----
6 files changed, 90 insertions(+), 81 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-5000-hw.h b/drivers/net/wireless/iwlwifi/iwl-5000-hw.h
index 17d4f31..c479ee2 100644
--- a/drivers/net/wireless/iwlwifi/iwl-5000-hw.h
+++ b/drivers/net/wireless/iwlwifi/iwl-5000-hw.h
@@ -129,6 +129,13 @@ struct iwl5000_shared {
__le32 padding2;
} __attribute__ ((packed));
+/* calibrations defined for 5000 */
+/* defines the order in which results should be sent to the runtime uCode */
+enum iwl5000_calib {
+ IWL5000_CALIB_LO,
+ IWL5000_CALIB_TX_IQ,
+ IWL5000_CALIB_TX_IQ_PERD,
+};
#endif /* __iwl_5000_hw_h__ */
diff --git a/drivers/net/wireless/iwlwifi/iwl-5000.c b/drivers/net/wireless/iwlwifi/iwl-5000.c
index d95fb42..e04a26f 100644
--- a/drivers/net/wireless/iwlwifi/iwl-5000.c
+++ b/drivers/net/wireless/iwlwifi/iwl-5000.c
@@ -445,48 +445,6 @@ static int iwl5000_send_Xtal_calib(struct iwl_priv *priv)
sizeof(cal_cmd), &cal_cmd);
}
-static int iwl5000_send_calib_results(struct iwl_priv *priv)
-{
- int ret = 0;
-
- struct iwl_host_cmd hcmd = {
- .id = REPLY_PHY_CALIBRATION_CMD,
- .meta.flags = CMD_SIZE_HUGE,
- };
-
- if (priv->calib_results.lo_res) {
- hcmd.len = priv->calib_results.lo_res_len;
- hcmd.data = priv->calib_results.lo_res;
- ret = iwl_send_cmd_sync(priv, &hcmd);
-
- if (ret)
- goto err;
- }
-
- if (priv->calib_results.tx_iq_res) {
- hcmd.len = priv->calib_results.tx_iq_res_len;
- hcmd.data = priv->calib_results.tx_iq_res;
- ret = iwl_send_cmd_sync(priv, &hcmd);
-
- if (ret)
- goto err;
- }
-
- if (priv->calib_results.tx_iq_perd_res) {
- hcmd.len = priv->calib_results.tx_iq_perd_res_len;
- hcmd.data = priv->calib_results.tx_iq_perd_res;
- ret = iwl_send_cmd_sync(priv, &hcmd);
-
- if (ret)
- goto err;
- }
-
- return 0;
-err:
- IWL_ERROR("Error %d\n", ret);
- return ret;
-}
-
static int iwl5000_send_calib_cfg(struct iwl_priv *priv)
{
struct iwl5000_calib_cfg_cmd calib_cfg_cmd;
@@ -511,33 +469,30 @@ static void iwl5000_rx_calib_result(struct iwl_priv *priv,
struct iwl_rx_packet *pkt = (void *)rxb->skb->data;
struct iwl5000_calib_hdr *hdr = (struct iwl5000_calib_hdr *)pkt->u.raw;
int len = le32_to_cpu(pkt->len) & FH_RSCSR_FRAME_SIZE_MSK;
-
- iwl_free_calib_results(priv);
+ int index;
/* reduce the size of the length field itself */
len -= 4;
+ /* Define the order in which the results will be sent to the runtime
+ * uCode. iwl_send_calib_results sends them in a row according to their
+ * index. We sort them here */
switch (hdr->op_code) {
case IWL5000_PHY_CALIBRATE_LO_CMD:
- priv->calib_results.lo_res = kzalloc(len, GFP_ATOMIC);
- priv->calib_results.lo_res_len = len;
- memcpy(priv->calib_results.lo_res, pkt->u.raw, len);
+ index = IWL5000_CALIB_LO;
break;
case IWL5000_PHY_CALIBRATE_TX_IQ_CMD:
- priv->calib_results.tx_iq_res = kzalloc(len, GFP_ATOMIC);
- priv->calib_results.tx_iq_res_len = len;
- memcpy(priv->calib_results.tx_iq_res, pkt->u.raw, len);
+ index = IWL5000_CALIB_TX_IQ;
break;
case IWL5000_PHY_CALIBRATE_TX_IQ_PERD_CMD:
- priv->calib_results.tx_iq_perd_res = kzalloc(len, GFP_ATOMIC);
- priv->calib_results.tx_iq_perd_res_len = len;
- memcpy(priv->calib_results.tx_iq_perd_res, pkt->u.raw, len);
+ index = IWL5000_CALIB_TX_IQ_PERD;
break;
default:
IWL_ERROR("Unknown calibration notification %d\n",
hdr->op_code);
return;
}
+ iwl_calib_set(&priv->calib_results[index], pkt->u.raw, len);
}
static void iwl5000_rx_calib_complete(struct iwl_priv *priv,
@@ -835,7 +790,7 @@ static int iwl5000_alive_notify(struct iwl_priv *priv)
iwl5000_send_Xtal_calib(priv);
if (priv->ucode_type == UCODE_RT)
- iwl5000_send_calib_results(priv);
+ iwl_send_calib_results(priv);
return 0;
}
diff --git a/drivers/net/wireless/iwlwifi/iwl-calib.c b/drivers/net/wireless/iwlwifi/iwl-calib.c
index ef49440..35fb4a4 100644
--- a/drivers/net/wireless/iwlwifi/iwl-calib.c
+++ b/drivers/net/wireless/iwlwifi/iwl-calib.c
@@ -66,6 +66,66 @@
#include "iwl-core.h"
#include "iwl-calib.h"
+/*****************************************************************************
+ * INIT calibrations framework
+ *****************************************************************************/
+
+ int iwl_send_calib_results(struct iwl_priv *priv)
+{
+ int ret = 0;
+ int i = 0;
+
+ struct iwl_host_cmd hcmd = {
+ .id = REPLY_PHY_CALIBRATION_CMD,
+ .meta.flags = CMD_SIZE_HUGE,
+ };
+
+ for (i = 0; i < IWL_CALIB_MAX; i++)
+ if (priv->calib_results[i].buf) {
+ hcmd.len = priv->calib_results[i].buf_len;
+ hcmd.data = priv->calib_results[i].buf;
+ ret = iwl_send_cmd_sync(priv, &hcmd);
+ if (ret)
+ goto err;
+ }
+
+ return 0;
+err:
+ IWL_ERROR("Error %d iteration %d\n", ret, i);
+ return ret;
+}
+EXPORT_SYMBOL(iwl_send_calib_results);
+
+int iwl_calib_set(struct iwl_calib_result *res, const u8 *buf, int len)
+{
+ if (res->buf_len != len) {
+ kfree(res->buf);
+ res->buf = kzalloc(len, GFP_ATOMIC);
+ }
+ if (unlikely(res->buf == NULL))
+ return -ENOMEM;
+
+ res->buf_len = len;
+ memcpy(res->buf, buf, len);
+ return 0;
+}
+EXPORT_SYMBOL(iwl_calib_set);
+
+void iwl_calib_free_results(struct iwl_priv *priv)
+{
+ int i;
+
+ for (i = 0; i < IWL_CALIB_MAX; i++) {
+ kfree(priv->calib_results[i].buf);
+ priv->calib_results[i].buf = NULL;
+ priv->calib_results[i].buf_len = 0;
+ }
+}
+
+/*****************************************************************************
+ * RUNTIME calibrations framework
+ *****************************************************************************/
+
/* "false alarms" are signals that our DSP tries to lock onto,
* but then determines that they are either noise, or transmissions
* from a distant wireless network (also "noise", really) that get
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index 09a677c..2b89c7b 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -934,22 +934,6 @@ err:
}
EXPORT_SYMBOL(iwl_init_drv);
-void iwl_free_calib_results(struct iwl_priv *priv)
-{
- kfree(priv->calib_results.lo_res);
- priv->calib_results.lo_res = NULL;
- priv->calib_results.lo_res_len = 0;
-
- kfree(priv->calib_results.tx_iq_res);
- priv->calib_results.tx_iq_res = NULL;
- priv->calib_results.tx_iq_res_len = 0;
-
- kfree(priv->calib_results.tx_iq_perd_res);
- priv->calib_results.tx_iq_perd_res = NULL;
- priv->calib_results.tx_iq_perd_res_len = 0;
-}
-EXPORT_SYMBOL(iwl_free_calib_results);
-
int iwl_set_tx_power(struct iwl_priv *priv, s8 tx_power, bool force)
{
int ret = 0;
@@ -977,10 +961,9 @@ int iwl_set_tx_power(struct iwl_priv *priv, s8 tx_power, bool force)
}
EXPORT_SYMBOL(iwl_set_tx_power);
-
void iwl_uninit_drv(struct iwl_priv *priv)
{
- iwl_free_calib_results(priv);
+ iwl_calib_free_results(priv);
iwlcore_free_geos(priv);
iwl_free_channel_map(priv);
kfree(priv->scan);
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.h b/drivers/net/wireless/iwlwifi/iwl-core.h
index ff86abc..b5db050 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.h
+++ b/drivers/net/wireless/iwlwifi/iwl-core.h
@@ -186,7 +186,6 @@ struct ieee80211_hw *iwl_alloc_all(struct iwl_cfg *cfg,
void iwl_hw_detect(struct iwl_priv *priv);
void iwl_clear_stations_table(struct iwl_priv *priv);
-void iwl_free_calib_results(struct iwl_priv *priv);
void iwl_reset_qos(struct iwl_priv *priv);
void iwl_set_rxon_chain(struct iwl_priv *priv);
int iwl_set_rxon_channel(struct iwl_priv *priv, struct ieee80211_channel *ch);
@@ -289,6 +288,13 @@ int iwl_scan_initiate(struct iwl_priv *priv);
void iwl_setup_rx_scan_handlers(struct iwl_priv *priv);
void iwl_setup_scan_deferred_work(struct iwl_priv *priv);
+/*******************************************************************************
+ * Calibrations - implemented in iwl-calib.c
+ ******************************************************************************/
+int iwl_send_calib_results(struct iwl_priv *priv);
+int iwl_calib_set(struct iwl_calib_result *res, const u8 *buf, int len);
+void iwl_calib_free_results(struct iwl_priv *priv);
+
/*****************************************************
* S e n d i n g H o s t C o m m a n d s *
*****************************************************/
diff --git a/drivers/net/wireless/iwlwifi/iwl-dev.h b/drivers/net/wireless/iwlwifi/iwl-dev.h
index c19db43..b00acf4 100644
--- a/drivers/net/wireless/iwlwifi/iwl-dev.h
+++ b/drivers/net/wireless/iwlwifi/iwl-dev.h
@@ -746,13 +746,10 @@ struct statistics_general_data {
u32 beacon_energy_c;
};
-struct iwl_calib_results {
- void *tx_iq_res;
- void *tx_iq_perd_res;
- void *lo_res;
- u32 tx_iq_res_len;
- u32 tx_iq_perd_res_len;
- u32 lo_res_len;
+/* Opaque calibration results */
+struct iwl_calib_result {
+ void *buf;
+ size_t buf_len;
};
enum ucode_type {
@@ -814,6 +811,7 @@ enum {
#define IWL_MAX_NUM_QUEUES 20 /* FIXME: do dynamic allocation */
+#define IWL_CALIB_MAX 3
struct iwl_priv {
@@ -858,7 +856,7 @@ struct iwl_priv {
s32 last_temperature;
/* init calibration results */
- struct iwl_calib_results calib_results;
+ struct iwl_calib_result calib_results[IWL_CALIB_MAX];
/* Scan related variables */
unsigned long last_scan_jiffies;
--
1.5.3.6
On Thursday 28 August 2008, Tomas Winkler wrote:
> >
> > Frankly, this sounds like a "quick, somebody help me come up with a good
> > argument" argument.
>
> So how can I really reason this :)
<example>
With this patch applied the card will draw about XX Watt less power, so this will
enhance the battery life a lot.
</example>
I'm pretty sure such things are documented in your device specifications.
Hi Tomas,
> >> This patch fixes and simplifies rx_chain computation.
> >
> > come on, what does it fix? You are submitting patches for -rc4 kernels.
>
> The fix is here
>
> + /* correct rx chain count accoridng hw settings */
while you are at it. Fix the spelling. It is "according" :)
> + if (priv->hw_params.rx_chains_num < active_rx_cnt)
> + active_rx_cnt = priv->hw_params.rx_chains_num;
> +
> + if (priv->hw_params.rx_chains_num < idle_rx_cnt)
> + idle_rx_cnt = priv->hw_params.rx_chains_num;
> +
>
> We would configure 1x2 MIMO like 3x3 -- big trouble
And that should be in the commit message. Even explain the trouble that
it causes.
Regards
Marcel
From: "Tomas Winkler" <[email protected]>
Date: Thu, 28 Aug 2008 13:58:59 +0300
> Regression or not, this is an important bug fix. With millions of
> laptops sold this is significant contribution to global worming.
Indeed, the worm population is out of control these days.
From: Gregory Greenman <[email protected]>
This patch calls apm stop on exit and suspend and sets STATUS_EXIT_PENDING
accordingly. Without this patch hardware consumes power even after driver
is removed or suspended.
Signed-off-by: Gregory Greenman <[email protected]>
Signed-off-by: Mohamed Abbas <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 16 +++++++++++-----
1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index 4986137..b1cefb5 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -2189,7 +2189,10 @@ static void __iwl4965_down(struct iwl_priv *priv)
udelay(5);
/* FIXME: apm_ops.suspend(priv) */
- priv->cfg->ops->lib->apm_ops.reset(priv);
+ if (exit_pending || test_bit(STATUS_IN_SUSPEND, &priv->status))
+ priv->cfg->ops->lib->apm_ops.stop(priv);
+ else
+ priv->cfg->ops->lib->apm_ops.reset(priv);
priv->cfg->ops->lib->free_shared_mem(priv);
exit:
@@ -4371,15 +4374,18 @@ static void __devexit iwl4965_pci_remove(struct pci_dev *pdev)
iwl_dbgfs_unregister(priv);
sysfs_remove_group(&pdev->dev.kobj, &iwl4965_attribute_group);
+ /* ieee80211_unregister_hw call wil cause iwl4965_mac_stop to
+ * to be called and iwl4965_down since we are removing the device
+ * we need to set STATUS_EXIT_PENDING bit.
+ */
+ set_bit(STATUS_EXIT_PENDING, &priv->status);
if (priv->mac80211_registered) {
ieee80211_unregister_hw(priv->hw);
priv->mac80211_registered = 0;
+ } else {
+ iwl4965_down(priv);
}
- set_bit(STATUS_EXIT_PENDING, &priv->status);
-
- iwl4965_down(priv);
-
/* make sure we flush any pending irq or
* tasklet for the driver
*/
--
1.5.3.6
Hi Yi,
> The patch fixes CSR_GP_CNTRL_REG_FLAG_INIT_DONE was set instead of
> cleared which disabled moving device to D0U state.
when was this mistake introduced. Is it a regression compared to what we
have in 2.6.26?
Regards
Marcel
On Thu, Aug 28, 2008 at 4:59 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Tomas,
>
>> >> This patch fixes and simplifies rx_chain computation.
>> >
>> > come on, what does it fix? You are submitting patches for -rc4 kernels.
>>
>> The fix is here
>>
>> + /* correct rx chain count accoridng hw settings */
>
> while you are at it. Fix the spelling. It is "according" :)
There is a cleanup patch, but it's deferred to 2.6.2X
>> + if (priv->hw_params.rx_chains_num < active_rx_cnt)
>> + active_rx_cnt = priv->hw_params.rx_chains_num;
>> +
>> + if (priv->hw_params.rx_chains_num < idle_rx_cnt)
>> + idle_rx_cnt = priv->hw_params.rx_chains_num;
>> +
>>
>> We would configure 1x2 MIMO like 3x3 -- big trouble
>
> And that should be in the commit message. Even explain the trouble that
> it causes
Correct.
> Regards
>
> Marcel
>
>
>
On Thu, Aug 28, 2008 at 3:27 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Yi,
>
>> This patch makes use of mac80211 defaults for channel and band settings
>> instead of setting them by the driver own. The iwlwifi driver used to
>> set G band channel 6 as the default channel.
>
> that is not a regression. The driver has been doing this until now and
> thus it can keep doing it until the next merge window.
>
> If you disagree, then please explain the regression in the commit
> message.
It's trashing initial configuration as two different configuration
were applied in the initial flow. Frankly I don't remember what bug we
had this with. Maybe it's not so urgent.
Tomas
On Thu, Aug 28, 2008 at 5:12 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Tomas,
>
>> >> This patch fixes a critical bug that only the last calibration result
>> >> was applied. On reception of one calibration result all the calibration
>> >> was freed. The patch fixes this problem by introducing a generic init
>> >> calibration framework which allows variable number of init calibrations
>> >> and allows addition new HW.
>> >
>> > a lot of code changes in -rc4 phase. Please give more details since the
>> > patch changes a lot and even introduces a new EXPORT.
>>
>> The first sentence is important, the whole framework was buggy.
>
> I got that part, but understand that just changing a framework at the
> state of -rc4 is never a good idea. Any possible regression that might
> come out of it, could delay the release.
>
> In this case it would actually help to have clear Reviewed-by or
> Tested-by statements in the patch. Signed-off only states the authors
> and doesn't give this change more credit.
Oh, man that fix went through Intel Validation (TM), With $500K scope
attached, checking RXIQ, TXIQ, LO, Power and got certification from
independent lab. Enough is enough.
Tomas
From: Tomas Winkler <[email protected]>
This patch adds workaround for an interrupt related hardware bug on
some platforms.
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index e35db85..4986137 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -2601,6 +2601,7 @@ static int iwl4965_mac_start(struct ieee80211_hw *hw)
{
struct iwl_priv *priv = hw->priv;
int ret;
+ u16 pci_cmd;
IWL_DEBUG_MAC80211("enter\n");
@@ -2611,6 +2612,13 @@ static int iwl4965_mac_start(struct ieee80211_hw *hw)
pci_restore_state(priv->pci_dev);
pci_enable_msi(priv->pci_dev);
+ /* enable interrupts if needed: hw bug w/a */
+ pci_read_config_word(priv->pci_dev, PCI_COMMAND, &pci_cmd);
+ if (pci_cmd & PCI_COMMAND_INTX_DISABLE) {
+ pci_cmd &= ~PCI_COMMAND_INTX_DISABLE;
+ pci_write_config_word(priv->pci_dev, PCI_COMMAND, pci_cmd);
+ }
+
ret = request_irq(priv->pci_dev->irq, iwl4965_isr, IRQF_SHARED,
DRV_NAME, priv);
if (ret) {
--
1.5.3.6
Hi Tomas,
> >> > > This patch calls apm stop on exit and suspend and sets STATUS_EXIT_PENDING
> >> > > accordingly. Without this patch hardware consumes power even after driver
> >> > > is removed or suspended.
> >> >
> >> > is this a regression to 2.6.26? That is always the important question
> >> > here since that will determine if the patch should go in. If the 2.6.26
> >> > version did this better (even if not prefect), then this patch should go
> >> > on, otherwise it could wait until next merge window.
> >>
> >> AFAIR people have been complaining about this behaviour _forever_, so
> >> it's hardly a regression.
> >
> > I think so too. However the question is still if 2.6.26 maybe did better
> > and we are now doing worse because of other patches. This needs to be in
> > the commit message. Otherwise this falls for me in the next merge window
> > category.
> >
> Regression or not, this is an important bug fix. With millions of
> laptops sold this is significant contribution to global worming.
I fully agree with you here (no questions asked), but that is besides
the point.
The whole idea is to speed up the release cycle and every fix that is
not a regression fix might have impacts. Also if it is not a regression,
I can wait until the next merge window. I think that Linus made it
pretty clear.
So while I personally would vote for including this patch, it doesn't
really fit the criteria for being submitted after -rc4. In the end this
is up to Dave if he feels it is worth to explain this to Linus. There is
a gray area.
If you wanna play the "being green" card, then include numbers on how
much power this would save :)
Regards
Marcel
Hi Yi,
> This patch gives the HW the possibility to send direct probes in passive
> channels (as long as traffic was detected in that channel) if such ssid
> was requested, so hidden ssid can be found now in those channels as well.
this also doesn't sound like a regression to me. Nice to have for sure,
but not acceptable according to the merge window rules.
Question here is was the driver supporting this before and we broke it
in the 2.6.27 phase or not.
Regards
Marcel
From: Tomas Winkler <[email protected]>
This patch makes use of mac80211 defaults for channel and band settings
instead of setting them by the driver own. The iwlwifi driver used to
set G band channel 6 as the default channel.
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 2 +-
drivers/net/wireless/iwlwifi/iwl-core.c | 11 +++++------
drivers/net/wireless/iwlwifi/iwl-core.h | 4 +---
3 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index d44587f..93af115 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -2843,7 +2843,7 @@ static int iwl4965_mac_config(struct ieee80211_hw *hw, struct ieee80211_conf *co
)
priv->staging_rxon.flags = 0;
- iwl_set_rxon_channel(priv, conf->channel->band, channel);
+ iwl_set_rxon_channel(priv, conf->channel);
iwl_set_flags_for_band(priv, conf->channel->band);
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.c b/drivers/net/wireless/iwlwifi/iwl-core.c
index c72f725..09a677c 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.c
+++ b/drivers/net/wireless/iwlwifi/iwl-core.c
@@ -773,7 +773,7 @@ void iwl_set_rxon_chain(struct iwl_priv *priv)
EXPORT_SYMBOL(iwl_set_rxon_chain);
/**
- * iwlcore_set_rxon_channel - Set the phymode and channel values in staging RXON
+ * iwl_set_rxon_channel - Set the phymode and channel values in staging RXON
* @phymode: MODE_IEEE80211A sets to 5.2GHz; all else set to 2.4GHz
* @channel: Any channel valid for the requested phymode
@@ -782,10 +782,11 @@ EXPORT_SYMBOL(iwl_set_rxon_chain);
* NOTE: Does not commit to the hardware; it sets appropriate bit fields
* in the staging RXON flag structure based on the phymode
*/
-int iwl_set_rxon_channel(struct iwl_priv *priv,
- enum ieee80211_band band,
- u16 channel)
+int iwl_set_rxon_channel(struct iwl_priv *priv, struct ieee80211_channel *ch)
{
+ enum ieee80211_band band = ch->band;
+ u16 channel = ieee80211_frequency_to_channel(ch->center_freq);
+
if (!iwl_get_channel_info(priv, band, channel)) {
IWL_DEBUG_INFO("Could not set channel to %d [%d]\n",
channel, band);
@@ -907,8 +908,6 @@ int iwl_init_drv(struct iwl_priv *priv)
priv->qos_data.qos_active = 0;
priv->qos_data.qos_cap.val = 0;
- iwl_set_rxon_channel(priv, IEEE80211_BAND_2GHZ, 6);
-
priv->rates_mask = IWL_RATES_MASK;
/* If power management is turned on, default to AC mode */
priv->power_mode = IWL_POWER_AC;
diff --git a/drivers/net/wireless/iwlwifi/iwl-core.h b/drivers/net/wireless/iwlwifi/iwl-core.h
index 64f139e..ff86abc 100644
--- a/drivers/net/wireless/iwlwifi/iwl-core.h
+++ b/drivers/net/wireless/iwlwifi/iwl-core.h
@@ -189,9 +189,7 @@ void iwl_clear_stations_table(struct iwl_priv *priv);
void iwl_free_calib_results(struct iwl_priv *priv);
void iwl_reset_qos(struct iwl_priv *priv);
void iwl_set_rxon_chain(struct iwl_priv *priv);
-int iwl_set_rxon_channel(struct iwl_priv *priv,
- enum ieee80211_band band,
- u16 channel);
+int iwl_set_rxon_channel(struct iwl_priv *priv, struct ieee80211_channel *ch);
void iwl_set_rxon_ht(struct iwl_priv *priv, struct iwl_ht_info *ht_info);
u8 iwl_is_fat_tx_allowed(struct iwl_priv *priv,
struct ieee80211_ht_info *sta_ht_inf);
--
1.5.3.6
On Thu, 2008-08-28 at 14:56 +0200, Marcel Holtmann wrote:
> > AFAIR people have been complaining about this behaviour _forever_, so
> > it's hardly a regression.
>
> I think so too. However the question is still if 2.6.26 maybe did better
> and we are now doing worse because of other patches. This needs to be in
> the commit message. Otherwise this falls for me in the next merge window
> category.
Also, this is another victim of the "oh we post patches when we think
it's convenient" rule, I took a minute to dig and the patch has been in
the iwlwifi repository on kernel.org for six days already.
johannes
Hi Yi,
> This patch is a W/A for the TSF sync issue, in which the ucode's timestamp
> is constantly a little behind incoming beacons' timestamps (in IBSS cells).
> The W/A simply stops the driver from declaring it has a reliable TSF value.
this is not a proper commit message for something that should go in
after the merge window. Besides the description of what the patch is
doing and why, you also need to justify its inclusion. So add a
paragraph that explain the impact if this patch doesn't gets merged and
why that is a regression.
If I would be in Dave's shoes, this looks like something that is not
really critical.
Regards
Marcel
From: Assaf Krauss <[email protected]>
This patch is a W/A for the TSF sync issue, in which the ucode's timestamp
is constantly a little behind incoming beacons' timestamps (in IBSS cells).
The W/A simply stops the driver from declaring it has a reliable TSF value.
Signed-off-by: Assaf Krauss <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 2 +-
drivers/net/wireless/iwlwifi/iwl-rx.c | 5 ++++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index 061ffba..d44587f 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -3580,7 +3580,7 @@ static int iwl4965_mac_beacon_update(struct ieee80211_hw *hw, struct sk_buff *sk
priv->assoc_id = 0;
timestamp = ((struct ieee80211_mgmt *)skb->data)->u.beacon.timestamp;
- priv->timestamp = le64_to_cpu(timestamp) + (priv->beacon_int * 1000);
+ priv->timestamp = le64_to_cpu(timestamp);
IWL_DEBUG_MAC80211("leave\n");
spin_unlock_irqrestore(&priv->lock, flags);
diff --git a/drivers/net/wireless/iwlwifi/iwl-rx.c b/drivers/net/wireless/iwlwifi/iwl-rx.c
index f3f6ea4..e81bfc4 100644
--- a/drivers/net/wireless/iwlwifi/iwl-rx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-rx.c
@@ -1173,7 +1173,10 @@ void iwl_rx_reply_rx(struct iwl_priv *priv,
rx_status.antenna = 0;
rx_status.flag = 0;
- rx_status.flag |= RX_FLAG_TSFT;
+
+ /* TSF isn't reliable. In order to allow smooth user experience,
+ * this W/A doesn't propagate it to the mac80211 */
+ /*rx_status.flag |= RX_FLAG_TSFT;*/
if ((unlikely(rx_start->cfg_phy_cnt > 20))) {
IWL_DEBUG_DROP("dsp size out of range [0,20]: %d/n",
--
1.5.3.6
On Thu, 2008-08-28 at 14:39 +0200, Marcel Holtmann wrote:
> Hi Yi,
>
> > This patch calls apm stop on exit and suspend and sets STATUS_EXIT_PENDING
> > accordingly. Without this patch hardware consumes power even after driver
> > is removed or suspended.
>
> is this a regression to 2.6.26? That is always the important question
> here since that will determine if the patch should go in. If the 2.6.26
> version did this better (even if not prefect), then this patch should go
> on, otherwise it could wait until next merge window.
AFAIR people have been complaining about this behaviour _forever_, so
it's hardly a regression.
johannes
Hi Johannes,
> > > This patch calls apm stop on exit and suspend and sets STATUS_EXIT_PENDING
> > > accordingly. Without this patch hardware consumes power even after driver
> > > is removed or suspended.
> >
> > is this a regression to 2.6.26? That is always the important question
> > here since that will determine if the patch should go in. If the 2.6.26
> > version did this better (even if not prefect), then this patch should go
> > on, otherwise it could wait until next merge window.
>
> AFAIR people have been complaining about this behaviour _forever_, so
> it's hardly a regression.
I think so too. However the question is still if 2.6.26 maybe did better
and we are now doing worse because of other patches. This needs to be in
the commit message. Otherwise this falls for me in the next merge window
category.
Regards
Marcel
Hi Thomas,
> >> Rx chain might change during power save transitions but it doesn't
> >> requires full rxon. The patch avoids the rxon by removing the rx_chain
> >> check.
> >
> > and again. Is this a regression. What is the down side or impact from
> > not doing this. Did we used to do it different in 2.6.26?
>
> Yes this one is regression this is new code and causes disconnection.
please put this into the commit messages. And then it should go in.
Regards
Marcel
From: Mohamed Abbas <[email protected]>
Rx chain might change during power save transitions but it doesn't
requires full rxon. The patch avoids the rxon by removing the rx_chain
check.
Signed-off-by: Mohamed Abbas <[email protected]
Signed-off-by: Tomas Winkler <[email protected]>
Signed-off-by: Zhu Yi <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-agn.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
index 93af115..e35db85 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
@@ -181,14 +181,14 @@ static int iwl4965_check_rxon_cmd(struct iwl_rxon_cmd *rxon)
}
/**
- * iwl4965_full_rxon_required - check if full RXON (vs RXON_ASSOC) cmd is needed
+ * iwl_full_rxon_required - check if full RXON (vs RXON_ASSOC) cmd is needed
* @priv: staging_rxon is compared to active_rxon
*
* If the RXON structure is changing enough to require a new tune,
* or is clearing the RXON_FILTER_ASSOC_MSK, then return 1 to indicate that
* a new tune (full RXON command, rather than RXON_ASSOC cmd) is required.
*/
-static int iwl4965_full_rxon_required(struct iwl_priv *priv)
+static int iwl_full_rxon_required(struct iwl_priv *priv)
{
/* These items are only settable from the full RXON command */
@@ -207,7 +207,6 @@ static int iwl4965_full_rxon_required(struct iwl_priv *priv)
priv->active_rxon.ofdm_ht_single_stream_basic_rates) ||
(priv->staging_rxon.ofdm_ht_dual_stream_basic_rates !=
priv->active_rxon.ofdm_ht_dual_stream_basic_rates) ||
- (priv->staging_rxon.rx_chain != priv->active_rxon.rx_chain) ||
(priv->staging_rxon.assoc_id != priv->active_rxon.assoc_id))
return 1;
@@ -263,7 +262,7 @@ static int iwl4965_commit_rxon(struct iwl_priv *priv)
/* If we don't need to send a full RXON, we can use
* iwl4965_rxon_assoc_cmd which is used to reconfigure filter
* and other flags for the current radio configuration. */
- if (!iwl4965_full_rxon_required(priv)) {
+ if (!iwl_full_rxon_required(priv)) {
ret = iwl_send_rxon_assoc(priv);
if (ret) {
IWL_ERROR("Error setting RXON_ASSOC (%d)\n", ret);
--
1.5.3.6
On Thu, Aug 28, 2008 at 3:49 PM, Marcel Holtmann
<[email protected]> wrote:
> Hi Thomas,
>
>> >> This patch gives the HW the possibility to send direct probes in passive
>> >> channels (as long as traffic was detected in that channel) if such ssid
>> >> was requested, so hidden ssid can be found now in those channels as well.
>> >
>> > this also doesn't sound like a regression to me. Nice to have for sure,
>> > but not acceptable according to the merge window rules.
>>
>> If you cannot scan your AP you also not able to connect to it, drawing
>> your card useless.
>>
>> > Question here is was the driver supporting this before and we broke it
>> > in the 2.6.27 phase or not.
>>
>> Irrelevant, this is fixing show stopper bug.
>
> it is relevant. So I read this commit message as, that now we can also
> find hidden SSIDs in passive channels. Question that you should ask then
> is if that was supported before. Yes or no. If yes, then it is a
> regression, no then it is a feature. And regression fixes go in features
> wait for the next merge window.
>
> The term show stopper bug is irrelevant when it comes to merge window
> rules.
>
I don't know if it worked before or not I didn't check. This is new code.
Tomas
On Thu, 2008-08-28 at 13:05 +0200, Johannes Berg wrote:
> Also, this is another victim of the "oh we post patches when we think
> it's convenient" rule, I took a minute to dig and the patch has been
> in the iwlwifi repository on kernel.org for six days already.
Hey, if you also search linux-wireless archive, you'll find it has been
on the list for 6 days as well.
http://marc.info/?l=linux-wireless&m=121937039517264&w=2
Thanks,
-yi
On Thu, 2008-08-28 at 14:21 +0300, Tomas Winkler wrote:
> On Thu, Aug 28, 2008 at 3:33 PM, Marcel Holtmann <[email protected]> wrote:
> > Hi Yi,
> >
> >> The patch fixes CSR_GP_CNTRL_REG_FLAG_INIT_DONE was set instead of
> >> cleared which disabled moving device to D0U state.
> >
> > when was this mistake introduced. Is it a regression compared to what we
> > have in 2.6.26?
>
> Again. think of global warming.
If you're so concerned about global warming, why didn't you fix this,
umm, months ago? :)
Frankly, this sounds like a "quick, somebody help me come up with a good
argument" argument.
johannes
On Thu, Aug 28, 2008 at 4:34 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Tomas,
>
>> >> The patch fixes CSR_GP_CNTRL_REG_FLAG_INIT_DONE was set instead of
>> >> cleared which disabled moving device to D0U state.
>> >
>> > when was this mistake introduced. Is it a regression compared to what we
>> > have in 2.6.26?
>>
>> Again. think of global warming.
>
> I fully get why we wanna fix that. That was not my questions. Is this a
> regression compared to what we have in 2.6.26? Yes or no. A clear yes
> make this a regression and we will not even discuss this any further.
> That patch should go in then. In the no case you have to come up with
> some good argument.
there is also the big vs large impact argument if the fix (note "fix"
not feature) is unlikely to break stuff there is no reason for it to
not go in.
Hi John,
> Thanks to Marcel for going through this stack of patches and
> reinforcing the point about offering better changelogs and about
> giving more careful consideration to differentiating fixes from
> features/cleanups/etc.
>
> FWIW, I think that "regressions only" is a bit too strict for a hard
> rule. Also, I think that many of these patches are still mergeable
> for 2.6.27 if we are given more thorough explanations of the actual
> problems they are meant to address and the actual solutions they
> apply to those problems.
after Tomas explanation and the discussion, I agree with the inclusion
into 2.6.27. The newly submitted series was better.
Regards
Marcel
On Thu, Aug 28, 2008 at 05:24:59PM +0800, Zhu Yi wrote:
> Hi,
>
> Here are the iwlwifi patches vital for 2.6.27. Please apply.
>
> Thanks,
> -yi
>
> [PATCH 01/11] iwlwifi: W/A for the TSF correction in IBSS
> [PATCH 02/11] iwlwifi: use mac80211 band and channel settings
> [PATCH 03/11] iwlwifi: fix hidden ssid discovery in passive channels
> [PATCH 04/11] iwlwifi: remove false rxon if rx chain changes
> [PATCH 05/11] iwlwifi: workaround interrupt handling no some platforms
> [PATCH 06/11] iwlwifi: fix apm_stop
> [PATCH 07/11] iwlwifi: generic init calibrations framework
> [PATCH 08/11] iwlwifi: call apm stop on exit
> [PATCH 09/11] iwlwifi: fix station mimo power save values
> [PATCH 10/11] iwlwifi: fix rx_chain computation
> [PATCH 11/11] iwlwifi: fix 64bit platform firmware loading
Thanks to Marcel for going through this stack of patches and
reinforcing the point about offering better changelogs and about
giving more careful consideration to differentiating fixes from
features/cleanups/etc.
FWIW, I think that "regressions only" is a bit too strict for a hard
rule. Also, I think that many of these patches are still mergeable
for 2.6.27 if we are given more thorough explanations of the actual
problems they are meant to address and the actual solutions they
apply to those problems.
I hope you will resubmit this series (more or less) with more thorough
changelogs in the near future.
Thanks!
John
--
John W. Linville
[email protected]
On Thu, Aug 28, 2008 at 05:25:07PM +0800, Zhu Yi wrote:
> From: Gregory Greenman <[email protected]>
>
> This patch calls apm stop on exit and suspend and sets STATUS_EXIT_PENDING
> accordingly. Without this patch hardware consumes power even after driver
> is removed or suspended.
>
> Signed-off-by: Gregory Greenman <[email protected]>
> Signed-off-by: Mohamed Abbas <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> Signed-off-by: Zhu Yi <[email protected]>
The two hunks of this patch seem like two different fixes, and the
changelog only matches the first hunk. When you resubmit this series
(which I presume you will be doing with better changelogs after
Marcel's commentary), please split this into two patches.
Thanks,
John
> ---
> drivers/net/wireless/iwlwifi/iwl-agn.c | 16 +++++++++++-----
> 1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/iwlwifi/iwl-agn.c b/drivers/net/wireless/iwlwifi/iwl-agn.c
> index 4986137..b1cefb5 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-agn.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-agn.c
> @@ -2189,7 +2189,10 @@ static void __iwl4965_down(struct iwl_priv *priv)
> udelay(5);
>
> /* FIXME: apm_ops.suspend(priv) */
> - priv->cfg->ops->lib->apm_ops.reset(priv);
> + if (exit_pending || test_bit(STATUS_IN_SUSPEND, &priv->status))
> + priv->cfg->ops->lib->apm_ops.stop(priv);
> + else
> + priv->cfg->ops->lib->apm_ops.reset(priv);
> priv->cfg->ops->lib->free_shared_mem(priv);
>
> exit:
> @@ -4371,15 +4374,18 @@ static void __devexit iwl4965_pci_remove(struct pci_dev *pdev)
> iwl_dbgfs_unregister(priv);
> sysfs_remove_group(&pdev->dev.kobj, &iwl4965_attribute_group);
>
> + /* ieee80211_unregister_hw call wil cause iwl4965_mac_stop to
> + * to be called and iwl4965_down since we are removing the device
> + * we need to set STATUS_EXIT_PENDING bit.
> + */
> + set_bit(STATUS_EXIT_PENDING, &priv->status);
> if (priv->mac80211_registered) {
> ieee80211_unregister_hw(priv->hw);
> priv->mac80211_registered = 0;
> + } else {
> + iwl4965_down(priv);
> }
>
> - set_bit(STATUS_EXIT_PENDING, &priv->status);
> -
> - iwl4965_down(priv);
> -
> /* make sure we flush any pending irq or
> * tasklet for the driver
> */
> --
> 1.5.3.6
>
>
--
John W. Linville
[email protected]
On Wed, Sep 3, 2008 at 12:01 AM, John W. Linville
<[email protected]> wrote:
> On Thu, Aug 28, 2008 at 05:25:07PM +0800, Zhu Yi wrote:
>> From: Gregory Greenman <[email protected]>
>>
>> This patch calls apm stop on exit and suspend and sets STATUS_EXIT_PENDING
>> accordingly. Without this patch hardware consumes power even after driver
>> is removed or suspended.
>>
>> Signed-off-by: Gregory Greenman <[email protected]>
>> Signed-off-by: Mohamed Abbas <[email protected]>
>> Signed-off-by: Tomas Winkler <[email protected]>
>> Signed-off-by: Zhu Yi <[email protected]>
>
> The two hunks of this patch seem like two different fixes, and the
> changelog only matches the first hunk. When you resubmit this series
> (which I presume you will be doing with better changelogs after
> Marcel's commentary), please split this into two patches.
>
It's not so obvious form the reading the patch but these two hunks
belongs together. Without the second hunk the exit flow is not correct
and first hunk fix has no effect. Although I don't mind to split them
providing it's easier to swallow.
Thanks
Tomas