| Message ID | 20180425140844.127279-3-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tony,
I love your patch! Yet something to improve:
[auto build test ERROR on spi/for-next]
[also build test ERROR on v4.17-rc2 next-20180426]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Tony-Lindgren/Suspend-improvments-for-omap2-mcspi/20180427-152647
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All errors (new ones prefixed by >>):
>> drivers//spi/spi-omap2-mcspi.c:1515:19: error: 'omap2_mcspi_suspend_noirq' undeclared here (not in a function); did you mean 'omap2_mcspi_suspend'?
.suspend_noirq = omap2_mcspi_suspend_noirq,
^~~~~~~~~~~~~~~~~~~~~~~~~
omap2_mcspi_suspend
>> drivers//spi/spi-omap2-mcspi.c:1516:18: error: 'omap2_mcspi_resume_noirq' undeclared here (not in a function); did you mean 'omap2_mcspi_suspend_noirq'?
.resume_noirq = omap2_mcspi_resume_noirq,
^~~~~~~~~~~~~~~~~~~~~~~~
omap2_mcspi_suspend_noirq
vim +1515 drivers//spi/spi-omap2-mcspi.c
1513
1514 static const struct dev_pm_ops omap2_mcspi_pm_ops = {
> 1515 .suspend_noirq = omap2_mcspi_suspend_noirq,
> 1516 .resume_noirq = omap2_mcspi_resume_noirq,
1517 .runtime_resume = omap_mcspi_runtime_resume,
1518 };
1519
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
* kbuild test robot <lkp@intel.com> [180427 13:08]: > Hi Tony, > > I love your patch! Yet something to improve: > > [auto build test ERROR on spi/for-next] > [also build test ERROR on v4.17-rc2 next-20180426] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Tony-Lindgren/Suspend-improvments-for-omap2-mcspi/20180427-152647 > base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next > config: sparc64-allyesconfig (attached as .config) > compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=sparc64 > > All errors (new ones prefixed by >>): > > >> drivers//spi/spi-omap2-mcspi.c:1515:19: error: 'omap2_mcspi_suspend_noirq' undeclared here (not in a function); did you mean 'omap2_mcspi_suspend'? > .suspend_noirq = omap2_mcspi_suspend_noirq, > ^~~~~~~~~~~~~~~~~~~~~~~~~ > omap2_mcspi_suspend > >> drivers//spi/spi-omap2-mcspi.c:1516:18: error: 'omap2_mcspi_resume_noirq' undeclared here (not in a function); did you mean 'omap2_mcspi_suspend_noirq'? > .resume_noirq = omap2_mcspi_resume_noirq, > ^~~~~~~~~~~~~~~~~~~~~~~~ > omap2_mcspi_suspend_noirq > > vim +1515 drivers//spi/spi-omap2-mcspi.c > > 1513 > 1514 static const struct dev_pm_ops omap2_mcspi_pm_ops = { > > 1515 .suspend_noirq = omap2_mcspi_suspend_noirq, > > 1516 .resume_noirq = omap2_mcspi_resume_noirq, > 1517 .runtime_resume = omap_mcspi_runtime_resume, > 1518 }; > 1519 Thanks will fix the naming if CONFIG_SUSPEND is not set and repost. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c --- a/drivers/spi/spi-omap2-mcspi.c +++ b/drivers/spi/spi-omap2-mcspi.c @@ -255,6 +255,7 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable) if (spi->controller_state) { int err = pm_runtime_get_sync(mcspi->dev); if (err < 0) { + pm_runtime_put_noidle(mcspi->dev); dev_err(mcspi->dev, "failed to get sync: %d\n", err); return; } @@ -1051,8 +1052,11 @@ static int omap2_mcspi_setup(struct spi_device *spi) } ret = pm_runtime_get_sync(mcspi->dev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(mcspi->dev); + return ret; + } ret = omap2_mcspi_setup_transfer(spi, NULL); pm_runtime_mark_last_busy(mcspi->dev); @@ -1270,8 +1274,11 @@ static int omap2_mcspi_master_setup(struct omap2_mcspi *mcspi) int ret = 0; ret = pm_runtime_get_sync(mcspi->dev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(mcspi->dev); + return ret; + } mcspi_write_reg(master, OMAP2_MCSPI_WAKEUPENABLE, OMAP2_MCSPI_WAKEUPENABLE_WKEN); @@ -1458,14 +1465,45 @@ static int omap2_mcspi_remove(struct platform_device *pdev) MODULE_ALIAS("platform:omap2_mcspi"); #ifdef CONFIG_SUSPEND -static int omap2_mcspi_resume(struct device *dev) +static int omap2_mcspi_suspend_noirq(struct device *dev) { - return pinctrl_pm_select_default_state(dev); + int error; + + /* + * Make sure device gets idled if other drivers call SPI + * functions between device_prepare() and device_complete() + */ + error = pm_runtime_force_suspend(dev); + if (error < 0) { + dev_err(dev, "%s: force suspend failed: %i\n", + __func__, error); + + return error; + } + + return pinctrl_pm_select_sleep_state(dev); } -static int omap2_mcspi_suspend(struct device *dev) +static int omap2_mcspi_resume_noirq(struct device *dev) { - return pinctrl_pm_select_sleep_state(dev); + struct spi_master *master = dev_get_drvdata(dev); + struct omap2_mcspi *mcspi = spi_master_get_devdata(master); + int error; + + error = pinctrl_pm_select_default_state(dev); + if (error) + dev_warn(mcspi->dev, "%s: failed to set pins: %i\n", + __func__, error); + + error = pm_runtime_force_resume(mcspi->dev); + if (error < 0) { + dev_warn(mcspi->dev, "%s: force resume failed: %i\n", + __func__, error); + + return error; + } + + return 0; } #else @@ -1474,8 +1512,8 @@ static int omap2_mcspi_suspend(struct device *dev) #endif static const struct dev_pm_ops omap2_mcspi_pm_ops = { - .resume = omap2_mcspi_resume, - .suspend = omap2_mcspi_suspend, + .suspend_noirq = omap2_mcspi_suspend_noirq, + .resume_noirq = omap2_mcspi_resume_noirq, .runtime_resume = omap_mcspi_runtime_resume, };
We currently are calling mcspi suspend and resume without considering that mcspi might provide resources for other device driver such as regulators. This means resume can fail and will produce -EACCES if errors if anything calls mcspi functions between device_prepare() and device_complete(). To fix the issue, let's do the following changes: 1. Let's add checking for return values for pm_runtime_get calls, and call pm_runtime_put_noidle() on errors. Things still fail after this change, but at least we see something is wrong as we now see -EACCES errors on resume. 2. Let's use noirq level for suspend and resume as other drivers can still call SPI related functions on suspend and resume. This still won't fix the -EACCES issue, but gets us to something a bit saner. 3. Finally, let's modify suspend and resume to call to make sure the device is idled properly on suspend. We have device_prepare() call pm_runtime_get_noresume() that won't get released until in device_complete() when it calls pm_runtime_put(). So if SPI is still active on entering suspend, it will never get idled unless we add calls to pm_runtime_force_suspend() and resume. This also fixes the -EACCES errors on resume together with changes 1 and 2 above. And since we're already rewriting suspend resume functions, let's arrange the order of suspend and resume functions to be like they usually are with suspend first. Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/spi/spi-omap2-mcspi.c | 54 +++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 8 deletions(-)