We found that there is a spin_lock_irqsave protection in msg_submit()
of mailbox.c and it is in the atomic context.
So when the mailbox controller driver calls pm_runtime_get_sync() in
mbox_chan_ops->send_data(), it will get this BUG report.
"BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:1164"
Add API to mbox_chan_ops to make mailbox takes into account that a driver
may need PM done.
Jason-JH.Lin (2):
mailbox: Add power_get/power_put API to mbox_chan_ops
mailbox: mtk-cmdq: Move pm_runimte_get and put to mbox_chan_ops API
drivers/mailbox/mailbox.c | 55 ++++++++++++++++++++++++++++++
drivers/mailbox/mtk-cmdq-mailbox.c | 37 ++++++++++----------
include/linux/mailbox_controller.h | 11 ++++++
3 files changed, 84 insertions(+), 19 deletions(-)
--
2.18.0
To avoid pm_runtime APIs calling sleep in all the atomic context APIs
in mbox_chan_ops, we add power_get/power_put API to let the controllers
implement them with pm_runtime APIs outside the other atomic context APIs
in mbox_chan_ops.
Signed-off-by: Jason-JH.Lin <[email protected]>
---
drivers/mailbox/mailbox.c | 55 ++++++++++++++++++++++++++++++
include/linux/mailbox_controller.h | 11 ++++++
2 files changed, 66 insertions(+)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index ebff3baf3045..bafcc7b0c0b8 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -58,6 +58,12 @@ static void msg_submit(struct mbox_chan *chan)
void *data;
int err = -EBUSY;
+ if (chan->mbox->ops->power_get) {
+ err = chan->mbox->ops->power_get(chan);
+ if (err < 0)
+ return;
+ }
+
spin_lock_irqsave(&chan->lock, flags);
if (!chan->msg_count || chan->active_req)
@@ -89,12 +95,16 @@ static void msg_submit(struct mbox_chan *chan)
hrtimer_start(&chan->mbox->poll_hrt, 0, HRTIMER_MODE_REL);
spin_unlock_irqrestore(&chan->mbox->poll_hrt_lock, flags);
}
+
+ if (chan->mbox->ops->power_put)
+ chan->mbox->ops->power_put(chan);
}
static void tx_tick(struct mbox_chan *chan, int r)
{
unsigned long flags;
void *mssg;
+ int ret;
spin_lock_irqsave(&chan->lock, flags);
mssg = chan->active_req;
@@ -107,10 +117,19 @@ static void tx_tick(struct mbox_chan *chan, int r)
if (!mssg)
return;
+ if (chan->mbox->ops->power_get) {
+ ret = chan->mbox->ops->power_get(chan);
+ if (ret < 0)
+ return;
+ }
+
/* Notify the client */
if (chan->cl->tx_done)
chan->cl->tx_done(chan->cl, mssg, r);
+ if (chan->mbox->ops->power_put)
+ chan->mbox->ops->power_put(chan);
+
if (r != -ETIME && chan->cl->tx_block)
complete(&chan->tx_complete);
}
@@ -223,9 +242,16 @@ EXPORT_SYMBOL_GPL(mbox_client_txdone);
*/
bool mbox_client_peek_data(struct mbox_chan *chan)
{
+ if (chan->mbox->ops->power_get)
+ if (chan->mbox->ops->power_get(chan) < 0)
+ return false;
+
if (chan->mbox->ops->peek_data)
return chan->mbox->ops->peek_data(chan);
+ if (chan->mbox->ops->power_put)
+ chan->mbox->ops->power_put(chan);
+
return false;
}
EXPORT_SYMBOL_GPL(mbox_client_peek_data);
@@ -310,10 +336,19 @@ int mbox_flush(struct mbox_chan *chan, unsigned long timeout)
if (!chan->mbox->ops->flush)
return -ENOTSUPP;
+ if (chan->mbox->ops->power_get) {
+ ret = chan->mbox->ops->power_get(chan);
+ if (ret < 0)
+ return ret;
+ }
+
ret = chan->mbox->ops->flush(chan, timeout);
if (ret < 0)
tx_tick(chan, ret);
+ if (chan->mbox->ops->power_put)
+ chan->mbox->ops->power_put(chan);
+
return ret;
}
EXPORT_SYMBOL_GPL(mbox_flush);
@@ -341,6 +376,12 @@ static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
spin_unlock_irqrestore(&chan->lock, flags);
+ if (chan->mbox->ops->power_get) {
+ ret = chan->mbox->ops->power_get(chan);
+ if (ret < 0)
+ return ERR_PTR(ret);
+ }
+
if (chan->mbox->ops->startup) {
ret = chan->mbox->ops->startup(chan);
@@ -441,7 +482,11 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
if (ret)
chan = ERR_PTR(ret);
+ if (chan->mbox->ops->power_put)
+ chan->mbox->ops->power_put(chan);
+
mutex_unlock(&con_mutex);
+
return chan;
}
EXPORT_SYMBOL_GPL(mbox_request_channel);
@@ -485,13 +530,23 @@ EXPORT_SYMBOL_GPL(mbox_request_channel_byname);
void mbox_free_channel(struct mbox_chan *chan)
{
unsigned long flags;
+ int ret;
if (!chan || !chan->cl)
return;
+ if (chan->mbox->ops->power_get) {
+ ret = chan->mbox->ops->power_get(chan);
+ if (ret < 0)
+ return;
+ }
+
if (chan->mbox->ops->shutdown)
chan->mbox->ops->shutdown(chan);
+ if (chan->mbox->ops->power_put)
+ chan->mbox->ops->power_put(chan);
+
/* The queued TX requests are simply aborted, no callbacks are made */
spin_lock_irqsave(&chan->lock, flags);
chan->cl = NULL;
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 6fee33cb52f5..e8f26e7dabfd 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -42,6 +42,15 @@ struct mbox_chan;
* Used only if txdone_poll:=true && txdone_irq:=false
* @peek_data: Atomic check for any received data. Return true if controller
* has some data to push to the client. False otherwise.
+ * @power_get: Called when the controller need to get the reference to keep
+ * the power on for the device of mailbox controller. It is
+ * optional to implement this function with pm_runtime APIs or
+ * more complicated operation.
+ * Return 0 is success, otherwise are fail.
+ * @power_put: Called when the controller need to put the reference to release
+ * the power for the device of mailbox controller. It is optional
+ * to implement this function with pm_runtime APIs or more
+ * complicated operation.
*/
struct mbox_chan_ops {
int (*send_data)(struct mbox_chan *chan, void *data);
@@ -50,6 +59,8 @@ struct mbox_chan_ops {
void (*shutdown)(struct mbox_chan *chan);
bool (*last_tx_done)(struct mbox_chan *chan);
bool (*peek_data)(struct mbox_chan *chan);
+ int (*power_get)(struct mbox_chan *chan);
+ void (*power_put)(struct mbox_chan *chan);
};
/**
--
2.18.0
> To avoid pm_runtime APIs calling sleep in all the atomic context APIs
PM-runtime?
> in mbox_chan_ops, we add power_get/power_put API to let the controllers
> implement them with pm_runtime APIs outside the other atomic context APIs
> in mbox_chan_ops.
* How do you think about to omit the word “we” from such a change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc3#n94
* Would you like to improve any indications for adjustments of data structures?
> Signed-off-by: Jason-JH.Lin <[email protected]>
Does a dot really belong to the personal name
(according to the Developer's Certificate of Origin)?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc3#n438
Regards,
Markus
Hi Jason-JH.Lin,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.10-rc3 next-20240613]
[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/Jason-JH-Lin/mailbox-Add-power_get-power_put-API-to-mbox_chan_ops/20240614-120412
base: linus/master
patch link: https://lore.kernel.org/r/20240614040133.24967-2-jason-jh.lin%40mediatek.com
patch subject: [PATCH 1/2] mailbox: Add power_get/power_put API to mbox_chan_ops
config: riscv-defconfig (https://download.01.org/0day-ci/archive/20240615/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 78ee473784e5ef6f0b19ce4cb111fb6e4d23c6b2)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240615/[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 drivers/mailbox/mailbox.c:9:
In file included from include/linux/interrupt.h:21:
In file included from arch/riscv/include/asm/sections.h:9:
In file included from include/linux/mm.h:2253:
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/mailbox/mailbox.c:382:11: error: incompatible pointer to integer conversion returning 'void *' from a function with result type 'int' [-Wint-conversion]
382 | return ERR_PTR(ret);
| ^~~~~~~~~~~~
1 warning and 1 error generated.
vim +382 drivers/mailbox/mailbox.c
355
356 static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
357 {
358 struct device *dev = cl->dev;
359 unsigned long flags;
360 int ret;
361
362 if (chan->cl || !try_module_get(chan->mbox->dev->driver->owner)) {
363 dev_dbg(dev, "%s: mailbox not free\n", __func__);
364 return -EBUSY;
365 }
366
367 spin_lock_irqsave(&chan->lock, flags);
368 chan->msg_free = 0;
369 chan->msg_count = 0;
370 chan->active_req = NULL;
371 chan->cl = cl;
372 init_completion(&chan->tx_complete);
373
374 if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
375 chan->txdone_method = TXDONE_BY_ACK;
376
377 spin_unlock_irqrestore(&chan->lock, flags);
378
379 if (chan->mbox->ops->power_get) {
380 ret = chan->mbox->ops->power_get(chan);
381 if (ret < 0)
> 382 return ERR_PTR(ret);
383 }
384
385 if (chan->mbox->ops->startup) {
386 ret = chan->mbox->ops->startup(chan);
387
388 if (ret) {
389 dev_err(dev, "Unable to startup the chan (%d)\n", ret);
390 mbox_free_channel(chan);
391 return ret;
392 }
393 }
394
395 return 0;
396 }
397
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Jason-JH.Lin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on fujitsu-integration/mailbox-for-next v6.10-rc3 next-20240613]
[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/Jason-JH-Lin/mailbox-Add-power_get-power_put-API-to-mbox_chan_ops/20240614-120412
base: linus/master
patch link: https://lore.kernel.org/r/20240614040133.24967-2-jason-jh.lin%40mediatek.com
patch subject: [PATCH 1/2] mailbox: Add power_get/power_put API to mbox_chan_ops
config: arc-randconfig-002-20240614 (https://download.01.org/0day-ci/archive/20240615/[email protected]/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240615/[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 warnings (new ones prefixed by >>):
drivers/mailbox/mailbox.c: In function '__mbox_bind_client':
>> drivers/mailbox/mailbox.c:382:32: warning: returning 'void *' from a function with return type 'int' makes integer from pointer without a cast [-Wint-conversion]
382 | return ERR_PTR(ret);
| ^~~~~~~~~~~~
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for REGMAP_SPI
Depends on [n]: SPI [=n]
Selected by [y]:
- AD9739A [=y] && IIO [=y] && (SPI [=n] || COMPILE_TEST [=y])
vim +382 drivers/mailbox/mailbox.c
355
356 static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
357 {
358 struct device *dev = cl->dev;
359 unsigned long flags;
360 int ret;
361
362 if (chan->cl || !try_module_get(chan->mbox->dev->driver->owner)) {
363 dev_dbg(dev, "%s: mailbox not free\n", __func__);
364 return -EBUSY;
365 }
366
367 spin_lock_irqsave(&chan->lock, flags);
368 chan->msg_free = 0;
369 chan->msg_count = 0;
370 chan->active_req = NULL;
371 chan->cl = cl;
372 init_completion(&chan->tx_complete);
373
374 if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
375 chan->txdone_method = TXDONE_BY_ACK;
376
377 spin_unlock_irqrestore(&chan->lock, flags);
378
379 if (chan->mbox->ops->power_get) {
380 ret = chan->mbox->ops->power_get(chan);
381 if (ret < 0)
> 382 return ERR_PTR(ret);
383 }
384
385 if (chan->mbox->ops->startup) {
386 ret = chan->mbox->ops->startup(chan);
387
388 if (ret) {
389 dev_err(dev, "Unable to startup the chan (%d)\n", ret);
390 mbox_free_channel(chan);
391 return ret;
392 }
393 }
394
395 return 0;
396 }
397
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Jason-JH.Lin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on fujitsu-integration/mailbox-for-next v6.10-rc3 next-20240613]
[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/Jason-JH-Lin/mailbox-Add-power_get-power_put-API-to-mbox_chan_ops/20240614-120412
base: linus/master
patch link: https://lore.kernel.org/r/20240614040133.24967-2-jason-jh.lin%40mediatek.com
patch subject: [PATCH 1/2] mailbox: Add power_get/power_put API to mbox_chan_ops
config: i386-randconfig-062-20240614 (https://download.01.org/0day-ci/archive/20240615/[email protected]/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240615/[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]/
sparse warnings: (new ones prefixed by >>)
>> drivers/mailbox/mailbox.c:382:39: sparse: sparse: incorrect type in return expression (different base types) @@ expected int @@ got void * @@
drivers/mailbox/mailbox.c:382:39: sparse: expected int
drivers/mailbox/mailbox.c:382:39: sparse: got void *
drivers/mailbox/mailbox.c: note: in included file (through include/linux/smp.h, include/linux/alloc_tag.h, include/linux/percpu.h, ...):
include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
vim +382 drivers/mailbox/mailbox.c
355
356 static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
357 {
358 struct device *dev = cl->dev;
359 unsigned long flags;
360 int ret;
361
362 if (chan->cl || !try_module_get(chan->mbox->dev->driver->owner)) {
363 dev_dbg(dev, "%s: mailbox not free\n", __func__);
364 return -EBUSY;
365 }
366
367 spin_lock_irqsave(&chan->lock, flags);
368 chan->msg_free = 0;
369 chan->msg_count = 0;
370 chan->active_req = NULL;
371 chan->cl = cl;
372 init_completion(&chan->tx_complete);
373
374 if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
375 chan->txdone_method = TXDONE_BY_ACK;
376
377 spin_unlock_irqrestore(&chan->lock, flags);
378
379 if (chan->mbox->ops->power_get) {
380 ret = chan->mbox->ops->power_get(chan);
381 if (ret < 0)
> 382 return ERR_PTR(ret);
383 }
384
385 if (chan->mbox->ops->startup) {
386 ret = chan->mbox->ops->startup(chan);
387
388 if (ret) {
389 dev_err(dev, "Unable to startup the chan (%d)\n", ret);
390 mbox_free_channel(chan);
391 return ret;
392 }
393 }
394
395 return 0;
396 }
397
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki