Re: [PATCH] iwlegacy: warn when enabling power save

[Stanislaw Gruszka <sgruszka@redhat.com>]

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