* [PATCH] iwlegacy: warn when enabling power save @ 2017-05-15 7:59 Stanislaw Gruszka 2017-05-15 8:41 ` Kalle Valo 0 siblings, 1 reply; 6+ messages in thread From: Stanislaw Gruszka @ 2017-05-15 7:59 UTC (permalink / raw) To: linux-wireless; +Cc: Stanislaw Gruszka iwlegacy firmware can crash when power save is configured. PS was allowed in "dbdac2b iwlegacy: properly enable power saving" with belive that user who enable PS is aware of that and can relate firmware crahes with PS. However some distributions seems to enable PS without user intervention, so warn about that. Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> --- drivers/net/wireless/intel/iwlegacy/common.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c index 140b6ea..6aaa0e7 100644 --- a/drivers/net/wireless/intel/iwlegacy/common.c +++ b/drivers/net/wireless/intel/iwlegacy/common.c @@ -5147,6 +5147,8 @@ void il_mac_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif, if (changed & (IEEE80211_CONF_CHANGE_PS | IEEE80211_CONF_CHANGE_IDLE)) { il->power_data.ps_disabled = !(conf->flags & IEEE80211_CONF_PS); + WARN_ONCE(!il->power_data.ps_disabled, + "Enabling power save might cause firmware crashes\n"); ret = il_power_update_mode(il, false); if (ret) D_MAC80211("Error setting sleep level\n"); -- 1.7.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iwlegacy: warn when enabling power save 2017-05-15 7:59 [PATCH] iwlegacy: warn when enabling power save Stanislaw Gruszka @ 2017-05-15 8:41 ` Kalle Valo 2017-05-15 9:20 ` Stanislaw Gruszka 0 siblings, 1 reply; 6+ messages in thread From: Kalle Valo @ 2017-05-15 8:41 UTC (permalink / raw) To: Stanislaw Gruszka; +Cc: linux-wireless Stanislaw Gruszka <sgruszka@redhat.com> writes: > iwlegacy firmware can crash when power save is configured. PS was > allowed in "dbdac2b iwlegacy: properly enable power saving" with belive > that user who enable PS is aware of that and can relate firmware crahes > with PS. However some distributions seems to enable PS without user > intervention, so warn about that. > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> > --- > drivers/net/wireless/intel/iwlegacy/common.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c > index 140b6ea..6aaa0e7 100644 > --- a/drivers/net/wireless/intel/iwlegacy/common.c > +++ b/drivers/net/wireless/intel/iwlegacy/common.c > @@ -5147,6 +5147,8 @@ void il_mac_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > > if (changed & (IEEE80211_CONF_CHANGE_PS | IEEE80211_CONF_CHANGE_IDLE)) { > il->power_data.ps_disabled = !(conf->flags & IEEE80211_CONF_PS); > + WARN_ONCE(!il->power_data.ps_disabled, > + "Enabling power save might cause firmware crashes\n"); This prints the whole stack trace, right? Isn't that excessive and fooling the users to think that they found a bug, which would mean more bug reports sent to us? So maybe a simple printk is better here? -- Kalle Valo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iwlegacy: warn when enabling power save 2017-05-15 8:41 ` Kalle Valo @ 2017-05-15 9:20 ` Stanislaw Gruszka 2017-05-15 9:25 ` Arend van Spriel 0 siblings, 1 reply; 6+ messages in thread From: Stanislaw Gruszka @ 2017-05-15 9:20 UTC (permalink / raw) To: Kalle Valo; +Cc: linux-wireless On Mon, May 15, 2017 at 11:41:05AM +0300, Kalle Valo wrote: > Stanislaw Gruszka <sgruszka@redhat.com> writes: > > > iwlegacy firmware can crash when power save is configured. PS was > > allowed in "dbdac2b iwlegacy: properly enable power saving" with belive > > that user who enable PS is aware of that and can relate firmware crahes > > with PS. However some distributions seems to enable PS without user > > intervention, so warn about that. > > > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> > > --- > > drivers/net/wireless/intel/iwlegacy/common.c | 2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c > > index 140b6ea..6aaa0e7 100644 > > --- a/drivers/net/wireless/intel/iwlegacy/common.c > > +++ b/drivers/net/wireless/intel/iwlegacy/common.c > > @@ -5147,6 +5147,8 @@ void il_mac_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > > > > if (changed & (IEEE80211_CONF_CHANGE_PS | IEEE80211_CONF_CHANGE_IDLE)) { > > il->power_data.ps_disabled = !(conf->flags & IEEE80211_CONF_PS); > > + WARN_ONCE(!il->power_data.ps_disabled, > > + "Enabling power save might cause firmware crashes\n"); > > This prints the whole stack trace, right? Isn't that excessive and > fooling the users to think that they found a bug, which would mean more > bug reports sent to us? So maybe a simple printk is better here? I wanted to have back trace to assure problem will not be missed, but I think you have right, I'll post v2. Thanks Stanislaw ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iwlegacy: warn when enabling power save 2017-05-15 9:20 ` Stanislaw Gruszka @ 2017-05-15 9:25 ` Arend van Spriel 2017-05-15 9:33 ` Stanislaw Gruszka 0 siblings, 1 reply; 6+ messages in thread From: Arend van Spriel @ 2017-05-15 9:25 UTC (permalink / raw) To: Stanislaw Gruszka, Kalle Valo; +Cc: linux-wireless On 5/15/2017 11:20 AM, Stanislaw Gruszka wrote: > On Mon, May 15, 2017 at 11:41:05AM +0300, Kalle Valo wrote: >> Stanislaw Gruszka <sgruszka@redhat.com> writes: >> >>> iwlegacy firmware can crash when power save is configured. PS was >>> allowed in "dbdac2b iwlegacy: properly enable power saving" with belive >>> that user who enable PS is aware of that and can relate firmware crahes >>> with PS. However some distributions seems to enable PS without user >>> intervention, so warn about that. >>> >>> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> >>> --- >>> drivers/net/wireless/intel/iwlegacy/common.c | 2 ++ >>> 1 files changed, 2 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c >>> index 140b6ea..6aaa0e7 100644 >>> --- a/drivers/net/wireless/intel/iwlegacy/common.c >>> +++ b/drivers/net/wireless/intel/iwlegacy/common.c >>> @@ -5147,6 +5147,8 @@ void il_mac_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif, >>> >>> if (changed & (IEEE80211_CONF_CHANGE_PS | IEEE80211_CONF_CHANGE_IDLE)) { >>> il->power_data.ps_disabled = !(conf->flags & IEEE80211_CONF_PS); >>> + WARN_ONCE(!il->power_data.ps_disabled, >>> + "Enabling power save might cause firmware crashes\n"); >> >> This prints the whole stack trace, right? Isn't that excessive and >> fooling the users to think that they found a bug, which would mean more >> bug reports sent to us? So maybe a simple printk is better here? > > I wanted to have back trace to assure problem will not be missed, but > I think you have right, I'll post v2. I think instead of printk, a wiphy_warn() would be better here using hw->wiphy. Regards, Arend ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iwlegacy: warn when enabling power save 2017-05-15 9:25 ` Arend van Spriel @ 2017-05-15 9:33 ` Stanislaw Gruszka 2017-05-15 9:58 ` Arend van Spriel 0 siblings, 1 reply; 6+ messages in thread From: Stanislaw Gruszka @ 2017-05-15 9:33 UTC (permalink / raw) To: Arend van Spriel; +Cc: Kalle Valo, linux-wireless On Mon, May 15, 2017 at 11:25:51AM +0200, Arend van Spriel wrote: > On 5/15/2017 11:20 AM, Stanislaw Gruszka wrote: > >On Mon, May 15, 2017 at 11:41:05AM +0300, Kalle Valo wrote: > >>Stanislaw Gruszka <sgruszka@redhat.com> writes: > >> > >>>iwlegacy firmware can crash when power save is configured. PS was > >>>allowed in "dbdac2b iwlegacy: properly enable power saving" with belive > >>>that user who enable PS is aware of that and can relate firmware crahes > >>>with PS. However some distributions seems to enable PS without user > >>>intervention, so warn about that. > >>> > >>>Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> > >>>--- > >>> drivers/net/wireless/intel/iwlegacy/common.c | 2 ++ > >>> 1 files changed, 2 insertions(+), 0 deletions(-) > >>> > >>>diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c > >>>index 140b6ea..6aaa0e7 100644 > >>>--- a/drivers/net/wireless/intel/iwlegacy/common.c > >>>+++ b/drivers/net/wireless/intel/iwlegacy/common.c > >>>@@ -5147,6 +5147,8 @@ void il_mac_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif, > >>> if (changed & (IEEE80211_CONF_CHANGE_PS | IEEE80211_CONF_CHANGE_IDLE)) { > >>> il->power_data.ps_disabled = !(conf->flags & IEEE80211_CONF_PS); > >>>+ WARN_ONCE(!il->power_data.ps_disabled, > >>>+ "Enabling power save might cause firmware crashes\n"); > >> > >>This prints the whole stack trace, right? Isn't that excessive and > >>fooling the users to think that they found a bug, which would mean more > >>bug reports sent to us? So maybe a simple printk is better here? > > > >I wanted to have back trace to assure problem will not be missed, but > >I think you have right, I'll post v2. > > I think instead of printk, a wiphy_warn() would be better here using > hw->wiphy. I used dev_warn variant, what is consistent with the driver code. Stanislaw ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iwlegacy: warn when enabling power save 2017-05-15 9:33 ` Stanislaw Gruszka @ 2017-05-15 9:58 ` Arend van Spriel 0 siblings, 0 replies; 6+ messages in thread From: Arend van Spriel @ 2017-05-15 9:58 UTC (permalink / raw) To: Stanislaw Gruszka; +Cc: Kalle Valo, linux-wireless On 5/15/2017 11:33 AM, Stanislaw Gruszka wrote: > On Mon, May 15, 2017 at 11:25:51AM +0200, Arend van Spriel wrote: >> On 5/15/2017 11:20 AM, Stanislaw Gruszka wrote: >>> On Mon, May 15, 2017 at 11:41:05AM +0300, Kalle Valo wrote: >>>> Stanislaw Gruszka <sgruszka@redhat.com> writes: >>>> >>>>> iwlegacy firmware can crash when power save is configured. PS was >>>>> allowed in "dbdac2b iwlegacy: properly enable power saving" with belive >>>>> that user who enable PS is aware of that and can relate firmware crahes >>>>> with PS. However some distributions seems to enable PS without user >>>>> intervention, so warn about that. >>>>> >>>>> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> >>>>> --- >>>>> drivers/net/wireless/intel/iwlegacy/common.c | 2 ++ >>>>> 1 files changed, 2 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c >>>>> index 140b6ea..6aaa0e7 100644 >>>>> --- a/drivers/net/wireless/intel/iwlegacy/common.c >>>>> +++ b/drivers/net/wireless/intel/iwlegacy/common.c >>>>> @@ -5147,6 +5147,8 @@ void il_mac_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif, >>>>> if (changed & (IEEE80211_CONF_CHANGE_PS | IEEE80211_CONF_CHANGE_IDLE)) { >>>>> il->power_data.ps_disabled = !(conf->flags & IEEE80211_CONF_PS); >>>>> + WARN_ONCE(!il->power_data.ps_disabled, >>>>> + "Enabling power save might cause firmware crashes\n"); >>>> >>>> This prints the whole stack trace, right? Isn't that excessive and >>>> fooling the users to think that they found a bug, which would mean more >>>> bug reports sent to us? So maybe a simple printk is better here? >>> >>> I wanted to have back trace to assure problem will not be missed, but >>> I think you have right, I'll post v2. >> >> I think instead of printk, a wiphy_warn() would be better here using >> hw->wiphy. > > I used dev_warn variant, what is consistent with the driver code. I noticed. Works for me ;-) Regards, Arend ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-15 9:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-15 7:59 [PATCH] iwlegacy: warn when enabling power save Stanislaw Gruszka 2017-05-15 8:41 ` Kalle Valo 2017-05-15 9:20 ` Stanislaw Gruszka 2017-05-15 9:25 ` Arend van Spriel 2017-05-15 9:33 ` Stanislaw Gruszka 2017-05-15 9:58 ` Arend van Spriel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox