Some drivers require sleeping, for example if the pwm device is connected
over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc
with the generated IR signal when sleeping occurs.
This patch makes it possible to use pwm when the driver does not sleep,
by introducing the pwm_can_sleep() function.
Signed-off-by: Sean Young <[email protected]>
Cc: Ivaylo Dimitrov <[email protected]>
---
drivers/pwm/core.c | 2 +-
drivers/pwm/pwm-fsl-ftm.c | 1 +
drivers/pwm/pwm-imx-tpm.c | 1 +
drivers/pwm/pwm-iqs620a.c | 1 +
drivers/pwm/pwm-lpc18xx-sct.c | 1 +
drivers/pwm/pwm-microchip-core.c | 1 +
drivers/pwm/pwm-omap-dmtimer.c | 1 +
drivers/pwm/pwm-pca9685.c | 1 +
drivers/pwm/pwm-renesas-tpu.c | 1 -
drivers/pwm/pwm-rz-mtu3.c | 1 +
drivers/pwm/pwm-sifive.c | 1 +
drivers/pwm/pwm-sti.c | 1 +
drivers/pwm/pwm-stm32.c | 1 +
drivers/pwm/pwm-twl-led.c | 1 +
drivers/pwm/pwm-twl.c | 1 +
include/linux/pwm.h | 27 +++++++++++++++++++++++----
16 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index dc66e3405bf5..d9679ae5b2be 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
* is a bad idea. So make it explicit that calling this function might
* sleep.
*/
- might_sleep();
+ might_sleep_if(pwm_can_sleep(pwm));
if (!pwm || !state || !state->period ||
state->duty_cycle > state->period)
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index b7c6045c5d08..b8b9392844e9 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
fpc->soc = of_device_get_match_data(&pdev->dev);
fpc->chip.dev = &pdev->dev;
+ fpc->chip.can_sleep = true;
base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(base))
diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
index 98ab65c89685..6fd579089240 100644
--- a/drivers/pwm/pwm-imx-tpm.c
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -365,6 +365,7 @@ static int pwm_imx_tpm_probe(struct platform_device *pdev)
tpm->chip.dev = &pdev->dev;
tpm->chip.ops = &imx_tpm_pwm_ops;
+ tpm->chip.can_sleep = true;
/* get number of channels */
val = readl(tpm->base + PWM_IMX_TPM_PARAM);
diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
index 47b3141135f3..ebce9e06b32e 100644
--- a/drivers/pwm/pwm-iqs620a.c
+++ b/drivers/pwm/pwm-iqs620a.c
@@ -209,6 +209,7 @@ static int iqs620_pwm_probe(struct platform_device *pdev)
iqs620_pwm->chip.dev = &pdev->dev;
iqs620_pwm->chip.ops = &iqs620_pwm_ops;
iqs620_pwm->chip.npwm = 1;
+ iqs620_pwm->chip.can_sleep = true;
mutex_init(&iqs620_pwm->lock);
diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index 7a19a840bca5..e26fc18b5304 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -395,6 +395,7 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
lpc18xx_pwm->chip.dev = &pdev->dev;
lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
lpc18xx_pwm->chip.npwm = LPC18XX_NUM_PWMS;
+ lpc18xx_pwm->chip.can_sleep = true;
/* SCT counter must be in unify (32 bit) mode */
lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
diff --git a/drivers/pwm/pwm-microchip-core.c b/drivers/pwm/pwm-microchip-core.c
index e7525c98105e..503b5b427d69 100644
--- a/drivers/pwm/pwm-microchip-core.c
+++ b/drivers/pwm/pwm-microchip-core.c
@@ -474,6 +474,7 @@ static int mchp_core_pwm_probe(struct platform_device *pdev)
mchp_core_pwm->chip.dev = &pdev->dev;
mchp_core_pwm->chip.ops = &mchp_core_pwm_ops;
mchp_core_pwm->chip.npwm = 16;
+ mchp_core_pwm->chip.can_sleep = true;
mchp_core_pwm->channel_enabled = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(0));
mchp_core_pwm->channel_enabled |=
diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 4889fbd8a431..438abbb80daf 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -404,6 +404,7 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
omap->chip.dev = &pdev->dev;
omap->chip.ops = &pwm_omap_dmtimer_ops;
omap->chip.npwm = 1;
+ omap->chip.can_sleep = true;
mutex_init(&omap->mutex);
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 3038a68412a7..a47e21977e49 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -571,6 +571,7 @@ static int pca9685_pwm_probe(struct i2c_client *client)
pca->chip.npwm = PCA9685_MAXCHAN + 1;
pca->chip.dev = &client->dev;
+ pca->chip.can_sleep = true;
ret = pwmchip_add(&pca->chip);
if (ret < 0)
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index d7311614c846..96797a33d8c6 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -11,7 +11,6 @@
#include <linux/init.h>
#include <linux/ioport.h>
#include <linux/module.h>
-#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
index a56cecb0e46e..6d874a2a8785 100644
--- a/drivers/pwm/pwm-rz-mtu3.c
+++ b/drivers/pwm/pwm-rz-mtu3.c
@@ -516,6 +516,7 @@ static int rz_mtu3_pwm_probe(struct platform_device *pdev)
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
rz_mtu3_pwm->chip.dev = &pdev->dev;
+ rz_mtu3_pwm->chip.can_sleep = true;
ret = devm_add_action_or_reset(&pdev->dev, rz_mtu3_pwm_pm_disable,
rz_mtu3_pwm);
if (ret < 0)
diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index eabddb7c7820..5677ed6eb4d5 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -240,6 +240,7 @@ static int pwm_sifive_probe(struct platform_device *pdev)
chip->dev = dev;
chip->ops = &pwm_sifive_ops;
chip->npwm = 4;
+ chip->can_sleep = true;
ddata->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(ddata->regs))
diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index b1d1373648a3..de9fbd570104 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -643,6 +643,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
pc->chip.dev = dev;
pc->chip.ops = &sti_pwm_ops;
pc->chip.npwm = pc->cdata->pwm_num_devs;
+ pc->chip.can_sleep = true;
ret = pwmchip_add(&pc->chip);
if (ret < 0) {
diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 3d6be7749e23..cd408158e55b 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -636,6 +636,7 @@ static int stm32_pwm_probe(struct platform_device *pdev)
priv->chip.dev = dev;
priv->chip.ops = &stm32pwm_ops;
priv->chip.npwm = stm32_pwm_detect_channels(priv);
+ priv->chip.can_sleep = true;
ret = devm_pwmchip_add(dev, &priv->chip);
if (ret < 0)
diff --git a/drivers/pwm/pwm-twl-led.c b/drivers/pwm/pwm-twl-led.c
index 8fb84b441853..9e71429ecaed 100644
--- a/drivers/pwm/pwm-twl-led.c
+++ b/drivers/pwm/pwm-twl-led.c
@@ -362,6 +362,7 @@ static int twl_pwmled_probe(struct platform_device *pdev)
}
twl->chip.dev = &pdev->dev;
+ twl->chip.can_sleep = true;
mutex_init(&twl->mutex);
diff --git a/drivers/pwm/pwm-twl.c b/drivers/pwm/pwm-twl.c
index 86567add79db..bd08014fec44 100644
--- a/drivers/pwm/pwm-twl.c
+++ b/drivers/pwm/pwm-twl.c
@@ -356,6 +356,7 @@ static int twl_pwm_probe(struct platform_device *pdev)
twl->chip.dev = &pdev->dev;
twl->chip.npwm = 2;
+ twl->chip.can_sleep = true;
mutex_init(&twl->mutex);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d2f9f690a9c1..c94894ffa4c4 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -287,6 +287,7 @@ struct pwm_ops {
* @ops: callbacks for this PWM controller
* @base: number of first PWM controlled by this chip
* @npwm: number of PWMs controlled by this chip
+ * @can_sleep: can the driver sleep in pwm_apply_state
* @of_xlate: request a PWM device given a device tree PWM specifier
* @of_pwm_n_cells: number of cells expected in the device tree PWM specifier
* @list: list node for internal use
@@ -297,6 +298,7 @@ struct pwm_chip {
const struct pwm_ops *ops;
int base;
unsigned int npwm;
+ bool can_sleep;
struct pwm_device * (*of_xlate)(struct pwm_chip *chip,
const struct of_phandle_args *args);
@@ -380,6 +382,18 @@ static inline void pwm_disable(struct pwm_device *pwm)
pwm_apply_state(pwm, &state);
}
+/**
+ * pwm_can_sleep() - can a pwm driver sleep in pwm_apply_state()
+ * @pwm: PWM device
+ *
+ * Returns: true if the driver may sleep, false if pwm_apply_state()
+ * can be called from atomic context.
+ */
+static inline bool pwm_can_sleep(struct pwm_device *pwm)
+{
+ return pwm->chip->can_sleep;
+}
+
/* PWM provider APIs */
int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
unsigned long timeout);
@@ -411,7 +425,7 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
static inline int pwm_apply_state(struct pwm_device *pwm,
const struct pwm_state *state)
{
- might_sleep();
+ might_sleep_if(pwm_can_sleep(pwm));
return -ENOTSUPP;
}
@@ -423,19 +437,24 @@ static inline int pwm_adjust_config(struct pwm_device *pwm)
static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
int period_ns)
{
- might_sleep();
+ might_sleep_if(pwm_can_sleep(pwm));
return -EINVAL;
}
static inline int pwm_enable(struct pwm_device *pwm)
{
- might_sleep();
+ might_sleep_if(pwm_can_sleep(pwm));
return -EINVAL;
}
static inline void pwm_disable(struct pwm_device *pwm)
{
- might_sleep();
+ might_sleep_if(pwm_can_sleep(pwm));
+}
+
+static inline bool pwm_can_sleep(struct pwm_device *pwm)
+{
+ return true;
}
static inline int pwm_capture(struct pwm_device *pwm,
--
2.42.0
Hi Sean,
kernel test robot noticed the following build errors:
[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on shawnguo/for-next atorgue-stm32/stm32-next media-tree/master linus/master v6.6-rc3 next-20230929]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sean-Young/media-pwm-ir-tx-trigger-edges-from-hrtimer-interrupt-context/20231001-194056
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
patch link: https://lore.kernel.org/r/1bd5241d584ceb4d6b731c4dc3203fb9686ee1d1.1696156485.git.sean%40mess.org
patch subject: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context
config: arm-randconfig-002-20231001 (https://download.01.org/0day-ci/archive/20231001/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231001/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from include/linux/cpumask.h:10,
from include/linux/smp.h:13,
from include/linux/lockdep.h:14,
from include/linux/spinlock.h:63,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/slab.h:16,
from include/linux/resource_ext.h:11,
from include/linux/acpi.h:13,
from include/linux/i2c.h:13,
from drivers/input/misc/da7280.c:12:
include/linux/pwm.h: In function 'pwm_apply_state':
>> include/linux/pwm.h:428:24: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'cant_sleep'? [-Werror=implicit-function-declaration]
428 | might_sleep_if(pwm_can_sleep(pwm));
| ^~~~~~~~~~~~~
include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
194 | #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
| ^~~~
In file included from drivers/input/misc/da7280.c:16:
include/linux/pwm.h: At top level:
>> include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'; have 'bool(struct pwm_device *)' {aka '_Bool(struct pwm_device *)'}
455 | static inline bool pwm_can_sleep(struct pwm_device *pwm)
| ^~~~~~~~~~~~~
include/linux/pwm.h:428:24: note: previous implicit declaration of 'pwm_can_sleep' with type 'int()'
428 | might_sleep_if(pwm_can_sleep(pwm));
| ^~~~~~~~~~~~~
include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
194 | #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
| ^~~~
cc1: some warnings being treated as errors
vim +428 include/linux/pwm.h
419
420 struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id);
421 struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
422 struct fwnode_handle *fwnode,
423 const char *con_id);
424 #else
425 static inline int pwm_apply_state(struct pwm_device *pwm,
426 const struct pwm_state *state)
427 {
> 428 might_sleep_if(pwm_can_sleep(pwm));
429 return -ENOTSUPP;
430 }
431
432 static inline int pwm_adjust_config(struct pwm_device *pwm)
433 {
434 return -ENOTSUPP;
435 }
436
437 static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
438 int period_ns)
439 {
440 might_sleep_if(pwm_can_sleep(pwm));
441 return -EINVAL;
442 }
443
444 static inline int pwm_enable(struct pwm_device *pwm)
445 {
446 might_sleep_if(pwm_can_sleep(pwm));
447 return -EINVAL;
448 }
449
450 static inline void pwm_disable(struct pwm_device *pwm)
451 {
452 might_sleep_if(pwm_can_sleep(pwm));
453 }
454
> 455 static inline bool pwm_can_sleep(struct pwm_device *pwm)
456 {
457 return true;
458 }
459
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Mon, Oct 02, 2023 at 12:07:58AM +0800, kernel test robot wrote:
> Hi Sean,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on thierry-reding-pwm/for-next]
> [also build test ERROR on shawnguo/for-next atorgue-stm32/stm32-next media-tree/master linus/master v6.6-rc3 next-20230929]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
There are indeed some configs in which there are build errors if
CONFIG_PWM is disabled, I'll fix it in the next version.
In the mean time it would be great to have some feedback on this patch,
and see if there is any other rework needed.
Thanks
Sean
>
> url: https://github.com/intel-lab-lkp/linux/commits/Sean-Young/media-pwm-ir-tx-trigger-edges-from-hrtimer-interrupt-context/20231001-194056
> base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
> patch link: https://lore.kernel.org/r/1bd5241d584ceb4d6b731c4dc3203fb9686ee1d1.1696156485.git.sean%40mess.org
> patch subject: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context
> config: i386-buildonly-randconfig-004-20231001 (https://download.01.org/0day-ci/archive/20231001/[email protected]/config)
> compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231001/[email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All errors (new ones prefixed by >>):
>
> In file included from arch/x86/include/asm/percpu.h:27:0,
> from arch/x86/include/asm/current.h:10,
> from include/linux/sched.h:12,
> from include/linux/ratelimit.h:6,
> from include/linux/dev_printk.h:16,
> from include/linux/device.h:15,
> from include/linux/backlight.h:12,
> from drivers/video/fbdev/ssd1307fb.c:8:
> include/linux/pwm.h: In function 'pwm_apply_state':
> include/linux/pwm.h:428:17: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'gpiod_cansleep'? [-Werror=implicit-function-declaration]
> might_sleep_if(pwm_can_sleep(pwm));
> ^
> include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
> #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
> ^~~~
> In file included from drivers/video/fbdev/ssd1307fb.c:16:0:
> include/linux/pwm.h: At top level:
> >> include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'
> static inline bool pwm_can_sleep(struct pwm_device *pwm)
> ^~~~~~~~~~~~~
> In file included from arch/x86/include/asm/percpu.h:27:0,
> from arch/x86/include/asm/current.h:10,
> from include/linux/sched.h:12,
> from include/linux/ratelimit.h:6,
> from include/linux/dev_printk.h:16,
> from include/linux/device.h:15,
> from include/linux/backlight.h:12,
> from drivers/video/fbdev/ssd1307fb.c:8:
> include/linux/pwm.h:428:17: note: previous implicit declaration of 'pwm_can_sleep' was here
> might_sleep_if(pwm_can_sleep(pwm));
> ^
> include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
> #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
> ^~~~
> cc1: some warnings being treated as errors
> --
> In file included from arch/x86/include/asm/percpu.h:27:0,
> from arch/x86/include/asm/preempt.h:6,
> from include/linux/preempt.h:79,
> from include/linux/spinlock.h:56,
> from include/linux/mmzone.h:8,
> from include/linux/gfp.h:7,
> from include/linux/slab.h:16,
> from include/linux/resource_ext.h:11,
> from include/linux/acpi.h:13,
> from include/linux/i2c.h:13,
> from drivers/mfd/intel_soc_pmic_crc.c:11:
> include/linux/pwm.h: In function 'pwm_apply_state':
> include/linux/pwm.h:428:17: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'cant_sleep'? [-Werror=implicit-function-declaration]
> might_sleep_if(pwm_can_sleep(pwm));
> ^
> include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
> #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
> ^~~~
> In file included from drivers/mfd/intel_soc_pmic_crc.c:18:0:
> include/linux/pwm.h: At top level:
> >> include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'
> static inline bool pwm_can_sleep(struct pwm_device *pwm)
> ^~~~~~~~~~~~~
> In file included from arch/x86/include/asm/percpu.h:27:0,
> from arch/x86/include/asm/preempt.h:6,
> from include/linux/preempt.h:79,
> from include/linux/spinlock.h:56,
> from include/linux/mmzone.h:8,
> from include/linux/gfp.h:7,
> from include/linux/slab.h:16,
> from include/linux/resource_ext.h:11,
> from include/linux/acpi.h:13,
> from include/linux/i2c.h:13,
> from drivers/mfd/intel_soc_pmic_crc.c:11:
> include/linux/pwm.h:428:17: note: previous implicit declaration of 'pwm_can_sleep' was here
> might_sleep_if(pwm_can_sleep(pwm));
> ^
> include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
> #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
> ^~~~
> cc1: some warnings being treated as errors
>
>
> vim +/pwm_can_sleep +455 include/linux/pwm.h
>
> 454
> > 455 static inline bool pwm_can_sleep(struct pwm_device *pwm)
> 456 {
> 457 return true;
> 458 }
> 459
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
Hi Sean,
kernel test robot noticed the following build errors:
[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on shawnguo/for-next atorgue-stm32/stm32-next media-tree/master linus/master v6.6-rc3 next-20230929]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sean-Young/media-pwm-ir-tx-trigger-edges-from-hrtimer-interrupt-context/20231001-194056
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
patch link: https://lore.kernel.org/r/1bd5241d584ceb4d6b731c4dc3203fb9686ee1d1.1696156485.git.sean%40mess.org
patch subject: [PATCH 1/2] pwm: make it possible to apply pwm changes in atomic context
config: i386-buildonly-randconfig-004-20231001 (https://download.01.org/0day-ci/archive/20231001/[email protected]/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231001/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from arch/x86/include/asm/percpu.h:27:0,
from arch/x86/include/asm/current.h:10,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from include/linux/backlight.h:12,
from drivers/video/fbdev/ssd1307fb.c:8:
include/linux/pwm.h: In function 'pwm_apply_state':
include/linux/pwm.h:428:17: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'gpiod_cansleep'? [-Werror=implicit-function-declaration]
might_sleep_if(pwm_can_sleep(pwm));
^
include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
^~~~
In file included from drivers/video/fbdev/ssd1307fb.c:16:0:
include/linux/pwm.h: At top level:
>> include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'
static inline bool pwm_can_sleep(struct pwm_device *pwm)
^~~~~~~~~~~~~
In file included from arch/x86/include/asm/percpu.h:27:0,
from arch/x86/include/asm/current.h:10,
from include/linux/sched.h:12,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from include/linux/backlight.h:12,
from drivers/video/fbdev/ssd1307fb.c:8:
include/linux/pwm.h:428:17: note: previous implicit declaration of 'pwm_can_sleep' was here
might_sleep_if(pwm_can_sleep(pwm));
^
include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
^~~~
cc1: some warnings being treated as errors
--
In file included from arch/x86/include/asm/percpu.h:27:0,
from arch/x86/include/asm/preempt.h:6,
from include/linux/preempt.h:79,
from include/linux/spinlock.h:56,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/slab.h:16,
from include/linux/resource_ext.h:11,
from include/linux/acpi.h:13,
from include/linux/i2c.h:13,
from drivers/mfd/intel_soc_pmic_crc.c:11:
include/linux/pwm.h: In function 'pwm_apply_state':
include/linux/pwm.h:428:17: error: implicit declaration of function 'pwm_can_sleep'; did you mean 'cant_sleep'? [-Werror=implicit-function-declaration]
might_sleep_if(pwm_can_sleep(pwm));
^
include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
^~~~
In file included from drivers/mfd/intel_soc_pmic_crc.c:18:0:
include/linux/pwm.h: At top level:
>> include/linux/pwm.h:455:20: error: conflicting types for 'pwm_can_sleep'
static inline bool pwm_can_sleep(struct pwm_device *pwm)
^~~~~~~~~~~~~
In file included from arch/x86/include/asm/percpu.h:27:0,
from arch/x86/include/asm/preempt.h:6,
from include/linux/preempt.h:79,
from include/linux/spinlock.h:56,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/slab.h:16,
from include/linux/resource_ext.h:11,
from include/linux/acpi.h:13,
from include/linux/i2c.h:13,
from drivers/mfd/intel_soc_pmic_crc.c:11:
include/linux/pwm.h:428:17: note: previous implicit declaration of 'pwm_can_sleep' was here
might_sleep_if(pwm_can_sleep(pwm));
^
include/linux/kernel.h:194:39: note: in definition of macro 'might_sleep_if'
#define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
^~~~
cc1: some warnings being treated as errors
vim +/pwm_can_sleep +455 include/linux/pwm.h
454
> 455 static inline bool pwm_can_sleep(struct pwm_device *pwm)
456 {
457 return true;
458 }
459
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hello Sean,
On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index dc66e3405bf5..d9679ae5b2be 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> * is a bad idea. So make it explicit that calling this function might
> * sleep.
> */
> - might_sleep();
> + might_sleep_if(pwm_can_sleep(pwm));
>
> if (!pwm || !state || !state->period ||
> state->duty_cycle > state->period)
I'd like to have a mechanism to catch drivers that missed to set
.can_sleep. The best idea I currently have for that is to disable
preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while
.apply() is running.
> diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> index b7c6045c5d08..b8b9392844e9 100644
> --- a/drivers/pwm/pwm-fsl-ftm.c
> +++ b/drivers/pwm/pwm-fsl-ftm.c
> @@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
>
> fpc->soc = of_device_get_match_data(&pdev->dev);
> fpc->chip.dev = &pdev->dev;
> + fpc->chip.can_sleep = true;
As .apply() being callable in non-sleepable context only depends on
.apply() I think a better place for this property is in struct pwm_ops.
Also I wonder if the distinction between atomic and sleeping
pwm_state_apply() should be more explicit. For GPIOs you have a sleeping
variant gpiod_set_value_cansleep() that allows to immediately determine
the intended context in the caller. This would allow that programming
a PWM stays a preemption point (if possible/desired) even if the
underlying hardware/driver is atomic. To not have to touch all consumer
drivers, maybe the pair for pwm should better be
pwm_apply_state()
pwm_apply_state_atomic()
instead of a "cansleep" suffix for the sleeping variant? Or maybe it's
better to accept touching all consumer drivers to get semantics similar
to gpio? I couldn't decide quickly what I really like better here, so
that's your chance to comment and influence the outcome :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Uwe,
On Wed, Oct 04, 2023 at 11:59:20AM +0200, Uwe Kleine-K?nig wrote:
> On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index dc66e3405bf5..d9679ae5b2be 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> > * is a bad idea. So make it explicit that calling this function might
> > * sleep.
> > */
> > - might_sleep();
> > + might_sleep_if(pwm_can_sleep(pwm));
> >
> > if (!pwm || !state || !state->period ||
> > state->duty_cycle > state->period)
>
> I'd like to have a mechanism to catch drivers that missed to set
> .can_sleep. The best idea I currently have for that is to disable
> preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while
> .apply() is running.
If we have pwm_apply_state_atomic(), then CONFIG_DEBUG_ATOMIC_SLEEP will
catch them, but only in that code path of course.
How about using non_block_start() and non_block_end() if can_sleep is
not set?
> > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> > index b7c6045c5d08..b8b9392844e9 100644
> > --- a/drivers/pwm/pwm-fsl-ftm.c
> > +++ b/drivers/pwm/pwm-fsl-ftm.c
> > @@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> >
> > fpc->soc = of_device_get_match_data(&pdev->dev);
> > fpc->chip.dev = &pdev->dev;
> > + fpc->chip.can_sleep = true;
>
> As .apply() being callable in non-sleepable context only depends on
> .apply() I think a better place for this property is in struct pwm_ops.
That makes sense.
> Also I wonder if the distinction between atomic and sleeping
> pwm_state_apply() should be more explicit. For GPIOs you have a sleeping
> variant gpiod_set_value_cansleep() that allows to immediately determine
> the intended context in the caller. This would allow that programming
> a PWM stays a preemption point (if possible/desired) even if the
> underlying hardware/driver is atomic. To not have to touch all consumer
> drivers, maybe the pair for pwm should better be
>
> pwm_apply_state()
> pwm_apply_state_atomic()
Do we need pwm_config_atomic(), pwm_enable_atomic(), and pwm_disable_atomic()
too? These are just convenience functions, so we can probably do without them.
> instead of a "cansleep" suffix for the sleeping variant? Or maybe it's
> better to accept touching all consumer drivers to get semantics similar
> to gpio? I couldn't decide quickly what I really like better here, so
> that's your chance to comment and influence the outcome :-)
If you expect to have more parameters for pwm_apply_state() then a flags
argument makes sense.
TBH I like the pwm_apply_state_atomic() option.
Sean
Hello Sean,
On Thu, Oct 05, 2023 at 09:30:32AM +0100, Sean Young wrote:
> On Wed, Oct 04, 2023 at 11:59:20AM +0200, Uwe Kleine-K?nig wrote:
> > On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index dc66e3405bf5..d9679ae5b2be 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> > > * is a bad idea. So make it explicit that calling this function might
> > > * sleep.
> > > */
> > > - might_sleep();
> > > + might_sleep_if(pwm_can_sleep(pwm));
> > >
> > > if (!pwm || !state || !state->period ||
> > > state->duty_cycle > state->period)
> >
> > I'd like to have a mechanism to catch drivers that missed to set
> > .can_sleep. The best idea I currently have for that is to disable
> > preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while
> > .apply() is running.
>
> If we have pwm_apply_state_atomic(), then CONFIG_DEBUG_ATOMIC_SLEEP will
> catch them, but only in that code path of course.
>
> How about using non_block_start() and non_block_end() if can_sleep is
> not set?
TIL, looks like it was created for that task.
> > > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> > > index b7c6045c5d08..b8b9392844e9 100644
> > > --- a/drivers/pwm/pwm-fsl-ftm.c
> > > +++ b/drivers/pwm/pwm-fsl-ftm.c
> > > @@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> > >
> > > fpc->soc = of_device_get_match_data(&pdev->dev);
> > > fpc->chip.dev = &pdev->dev;
> > > + fpc->chip.can_sleep = true;
> >
> > Also I wonder if the distinction between atomic and sleeping
> > pwm_state_apply() should be more explicit. For GPIOs you have a sleeping
> > variant gpiod_set_value_cansleep() that allows to immediately determine
> > the intended context in the caller. This would allow that programming
> > a PWM stays a preemption point (if possible/desired) even if the
> > underlying hardware/driver is atomic. To not have to touch all consumer
> > drivers, maybe the pair for pwm should better be
> >
> > pwm_apply_state()
> > pwm_apply_state_atomic()
>
> Do we need pwm_config_atomic(), pwm_enable_atomic(), and pwm_disable_atomic()
> too? These are just convenience functions, so we can probably do without them.
I'd like to get rid of these, so for now I'd keep them as is.
> > instead of a "cansleep" suffix for the sleeping variant? Or maybe it's
> > better to accept touching all consumer drivers to get semantics similar
> > to gpio? I couldn't decide quickly what I really like better here, so
> > that's your chance to comment and influence the outcome :-)
>
> If you expect to have more parameters for pwm_apply_state() then a flags
> argument makes sense.
Actually I don't want more parameters -- at least for this use case. I
could imagine another parameter for something like apply-immediately vs.
complete-current-period, but that's another topic.
> TBH I like the pwm_apply_state_atomic() option.
ok.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed, Oct 04, 2023 at 11:59:20AM +0200, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index dc66e3405bf5..d9679ae5b2be 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -505,7 +505,7 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> > * is a bad idea. So make it explicit that calling this function might
> > * sleep.
> > */
> > - might_sleep();
> > + might_sleep_if(pwm_can_sleep(pwm));
> >
> > if (!pwm || !state || !state->period ||
> > state->duty_cycle > state->period)
>
> I'd like to have a mechanism to catch drivers that missed to set
> .can_sleep. The best idea I currently have for that is to disable
> preemption if IS_ENABLED(CONFIG_PWM_DEBUG) && !pwm_can_sleep(pwm) while
> .apply() is running.
>
> > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> > index b7c6045c5d08..b8b9392844e9 100644
> > --- a/drivers/pwm/pwm-fsl-ftm.c
> > +++ b/drivers/pwm/pwm-fsl-ftm.c
> > @@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> >
> > fpc->soc = of_device_get_match_data(&pdev->dev);
> > fpc->chip.dev = &pdev->dev;
> > + fpc->chip.can_sleep = true;
>
> As .apply() being callable in non-sleepable context only depends on
> .apply() I think a better place for this property is in struct pwm_ops.
What about drivers for devices that can be either sleeping or not? There
are things like regmap which can abstract those differences away, so you
could have a driver that works on both types of devices, so setting this
in ops wouldn't be correct all the time. I think can_sleep needs to be
per-chip rather than per-driver.
Thierry
On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
[...]
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index d2f9f690a9c1..c94894ffa4c4 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -287,6 +287,7 @@ struct pwm_ops {
> * @ops: callbacks for this PWM controller
> * @base: number of first PWM controlled by this chip
> * @npwm: number of PWMs controlled by this chip
> + * @can_sleep: can the driver sleep in pwm_apply_state
> * @of_xlate: request a PWM device given a device tree PWM specifier
> * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier
> * @list: list node for internal use
> @@ -297,6 +298,7 @@ struct pwm_chip {
> const struct pwm_ops *ops;
> int base;
> unsigned int npwm;
> + bool can_sleep;
Can we please call this "might_sleep"?
>
> struct pwm_device * (*of_xlate)(struct pwm_chip *chip,
> const struct of_phandle_args *args);
> @@ -380,6 +382,18 @@ static inline void pwm_disable(struct pwm_device *pwm)
> pwm_apply_state(pwm, &state);
> }
>
> +/**
> + * pwm_can_sleep() - can a pwm driver sleep in pwm_apply_state()
> + * @pwm: PWM device
> + *
> + * Returns: true if the driver may sleep, false if pwm_apply_state()
> + * can be called from atomic context.
> + */
> +static inline bool pwm_can_sleep(struct pwm_device *pwm)
And this one pwm_might_sleep()? I don't see why we need to deviate from
the nomenclature that the core introduced.
Thierry
Hello Thierry,
On Fri, Oct 06, 2023 at 12:27:51PM +0200, Thierry Reding wrote:
> On Wed, Oct 04, 2023 at 11:59:20AM +0200, Uwe Kleine-K?nig wrote:
> > On Sun, Oct 01, 2023 at 11:40:29AM +0100, Sean Young wrote:
> > > diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> > > index b7c6045c5d08..b8b9392844e9 100644
> > > --- a/drivers/pwm/pwm-fsl-ftm.c
> > > +++ b/drivers/pwm/pwm-fsl-ftm.c
> > > @@ -405,6 +405,7 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> > >
> > > fpc->soc = of_device_get_match_data(&pdev->dev);
> > > fpc->chip.dev = &pdev->dev;
> > > + fpc->chip.can_sleep = true;
> >
> > As .apply() being callable in non-sleepable context only depends on
> > .apply() I think a better place for this property is in struct pwm_ops.
>
> What about drivers for devices that can be either sleeping or not? There
> are things like regmap which can abstract those differences away, so you
> could have a driver that works on both types of devices, so setting this
> in ops wouldn't be correct all the time. I think can_sleep needs to be
> per-chip rather than per-driver.
I would consider that a theoretic possibility. If there is a hardware
that has a (say) i2c and a memory-mapped register variant, I never
encountered such a thing. Hmm, the dwc driver seems to have a pci and a
memory-mapped variant, both would be "fast" though. (Wouldn't they?)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |