| Message ID | 20240821183639.163187-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: wilc1000: Do not operate uninitialized hardware during suspend/resume | expand |
Hi Marek, On 8/21/24 11:36, Marek Vasut wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > In case the hardware is not initialized, do not operate it during > suspend/resume cycle, the hardware is already off so there is no > reason to access it. > > In fact, wilc_sdio_enable_interrupt() in the resume callback does > interfere with the same call when initializing the hardware after > resume and makes such initialization after resume fail. Fix this > by not operating uninitialized hardware during suspend/resume. Is this behavior observed then power-save is enabled when interface is not up. Ideally registers read/write commands should pass as soon the wilc module is up. But anyway, it is good have this check to avoid these commands. if possible, please add the similar check for wilc_spi_suspend/resume() to have similar behavior. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Ajay Singh <ajay.kathat@microchip.com> > Cc: Alexis Lothoré <alexis.lothore@bootlin.com> > Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev> > Cc: Kalle Valo <kvalo@kernel.org> > Cc: Marek Vasut <marex@denx.de> > Cc: linux-wireless@vger.kernel.org > --- > drivers/net/wireless/microchip/wilc1000/sdio.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c > index 0043f7a0fdf97..7999aeb76901f 100644 > --- a/drivers/net/wireless/microchip/wilc1000/sdio.c > +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c > @@ -977,6 +977,9 @@ static int wilc_sdio_suspend(struct device *dev) > > dev_info(dev, "sdio suspend\n"); > > + if (!wilc->initialized) > + return 0; > + > if (!IS_ERR(wilc->rtc_clk)) > clk_disable_unprepare(wilc->rtc_clk); > > @@ -999,6 +1002,10 @@ static int wilc_sdio_resume(struct device *dev) > struct wilc *wilc = sdio_get_drvdata(func); > > dev_info(dev, "sdio resume\n"); > + > + if (!wilc->initialized) > + return 0; > + > wilc_sdio_init(wilc, true); > wilc_sdio_enable_interrupt(wilc); > > -- > 2.43.0 >
On 8/23/24 7:16 PM, Ajay.Kathat@microchip.com wrote: > Hi Marek, Hi, > On 8/21/24 11:36, Marek Vasut wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> In case the hardware is not initialized, do not operate it during >> suspend/resume cycle, the hardware is already off so there is no >> reason to access it. >> >> In fact, wilc_sdio_enable_interrupt() in the resume callback does >> interfere with the same call when initializing the hardware after >> resume and makes such initialization after resume fail. Fix this >> by not operating uninitialized hardware during suspend/resume. > > Is this behavior observed then power-save is enabled when interface is not up. This is observed when the system with WILC3000 boots (wilc3000 is not if-up'd or anything) and is suspended and resumed, as simple as that.
On 8/23/24 19:16, Ajay.Kathat@microchip.com wrote: > Hi Marek, > > On 8/21/24 11:36, Marek Vasut wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> In case the hardware is not initialized, do not operate it during >> suspend/resume cycle, the hardware is already off so there is no >> reason to access it. >> >> In fact, wilc_sdio_enable_interrupt() in the resume callback does >> interfere with the same call when initializing the hardware after >> resume and makes such initialization after resume fail. Fix this >> by not operating uninitialized hardware during suspend/resume. > > Is this behavior observed then power-save is enabled when interface is not up. > Ideally registers read/write commands should pass as soon the wilc module is > up. But anyway, it is good have this check to avoid these commands. if > possible, please add the similar check for wilc_spi_suspend/resume() to have > similar behavior. It looks like there is a hole and that no PM ops are implemented in the upstream driver on spi side. That may be a miss from me when I sent the power management series a few months ago. I'll fix that. So for this change: Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com> > >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- >> Cc: Ajay Singh <ajay.kathat@microchip.com> >> Cc: Alexis Lothoré <alexis.lothore@bootlin.com> >> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev> >> Cc: Kalle Valo <kvalo@kernel.org> >> Cc: Marek Vasut <marex@denx.de> >> Cc: linux-wireless@vger.kernel.org >> --- >> drivers/net/wireless/microchip/wilc1000/sdio.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c >> index 0043f7a0fdf97..7999aeb76901f 100644 >> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c >> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c >> @@ -977,6 +977,9 @@ static int wilc_sdio_suspend(struct device *dev) >> >> dev_info(dev, "sdio suspend\n"); >> >> + if (!wilc->initialized) >> + return 0; >> + >> if (!IS_ERR(wilc->rtc_clk)) >> clk_disable_unprepare(wilc->rtc_clk); >> >> @@ -999,6 +1002,10 @@ static int wilc_sdio_resume(struct device *dev) >> struct wilc *wilc = sdio_get_drvdata(func); >> >> dev_info(dev, "sdio resume\n"); >> + >> + if (!wilc->initialized) >> + return 0; >> + >> wilc_sdio_init(wilc, true); >> wilc_sdio_enable_interrupt(wilc); >> >> -- >> 2.43.0 >> >
Marek Vasut <marex@denx.de> wrote: > In case the hardware is not initialized, do not operate it during > suspend/resume cycle, the hardware is already off so there is no > reason to access it. > > In fact, wilc_sdio_enable_interrupt() in the resume callback does > interfere with the same call when initializing the hardware after > resume and makes such initialization after resume fail. Fix this > by not operating uninitialized hardware during suspend/resume. > > Signed-off-by: Marek Vasut <marex@denx.de> > Reviewed-by: Alexis Lothoré <alexis.lothore@bootlin.com> Patch applied to wireless-next.git, thanks. b0dc7018477e wifi: wilc1000: Do not operate uninitialized hardware during suspend/resume
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c index 0043f7a0fdf97..7999aeb76901f 100644 --- a/drivers/net/wireless/microchip/wilc1000/sdio.c +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c @@ -977,6 +977,9 @@ static int wilc_sdio_suspend(struct device *dev) dev_info(dev, "sdio suspend\n"); + if (!wilc->initialized) + return 0; + if (!IS_ERR(wilc->rtc_clk)) clk_disable_unprepare(wilc->rtc_clk); @@ -999,6 +1002,10 @@ static int wilc_sdio_resume(struct device *dev) struct wilc *wilc = sdio_get_drvdata(func); dev_info(dev, "sdio resume\n"); + + if (!wilc->initialized) + return 0; + wilc_sdio_init(wilc, true); wilc_sdio_enable_interrupt(wilc);
In case the hardware is not initialized, do not operate it during suspend/resume cycle, the hardware is already off so there is no reason to access it. In fact, wilc_sdio_enable_interrupt() in the resume callback does interfere with the same call when initializing the hardware after resume and makes such initialization after resume fail. Fix this by not operating uninitialized hardware during suspend/resume. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Ajay Singh <ajay.kathat@microchip.com> Cc: Alexis Lothoré <alexis.lothore@bootlin.com> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev> Cc: Kalle Valo <kvalo@kernel.org> Cc: Marek Vasut <marex@denx.de> Cc: linux-wireless@vger.kernel.org --- drivers/net/wireless/microchip/wilc1000/sdio.c | 7 +++++++ 1 file changed, 7 insertions(+)