* [PATCH v2] mailbox: don't free the channel if the startup callback failed
@ 2026-04-20 11:41 Wolfram Sang
2026-04-21 15:07 ` kernel test robot
2026-04-22 10:43 ` kernel test robot
0 siblings, 2 replies; 3+ messages in thread
From: Wolfram Sang @ 2026-04-20 11:41 UTC (permalink / raw)
To: linux-renesas-soc; +Cc: linux-kernel, Wolfram Sang, Jassi Brar, Mark Brown
If the optional startup() callbacks fails, we need to clear some states.
Currently, this is done by freeing the channel. This does, however, more
than needed which creates problems. Namely, it is calling the shutdown()
callback. This is totally not intuitive. No user expects that shutdown()
is called when startup() fails, similar to remove() not being called
when probe() fails. Currently, quite some mailbox users register the IRQ
in startup() and free them in shutdown(). These drivers will get a WARN
about freeing an already free IRQ. Other subtle issues could arise from
this unexpected behaviour.
To solve this problem, introduce a helper which does the minimal cleanup
and use it in both, in free_channel() and after startup() failed.
Link: https://sashiko.dev/#/patchset/20260402112709.13002-1-wsa%2Brenesas%40sang-engineering.com # second issue
Fixes: 2b6d83e2b8b7 ("mailbox: Introduce framework for mailbox")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since RFC v1:
* use a helper instead of open coding the cleanup
* reword commit message to explain more and drop the wrong "issue" of
module_put imbalance
drivers/mailbox/mailbox.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index bbc9fd75a95f..2a83f83cf868 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -350,10 +350,9 @@ static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
if (chan->mbox->ops->startup) {
ret = chan->mbox->ops->startup(chan);
-
if (ret) {
dev_err(dev, "Unable to startup the chan (%d)\n", ret);
- mbox_free_channel(chan);
+ mbox_clean_and_put_channel(chan);
return ret;
}
}
@@ -482,6 +481,19 @@ struct mbox_chan *mbox_request_channel_byname(struct mbox_client *cl,
}
EXPORT_SYMBOL_GPL(mbox_request_channel_byname);
+void mbox_clean_and_put_channel(struct mbox_chan *chan)
+{
+ /* The queued TX requests are simply aborted, no callbacks are made */
+ scoped_guard(spinlock_irqsave, &chan->lock) {
+ chan->cl = NULL;
+ chan->active_req = MBOX_NO_MSG;
+ if (chan->txdone_method == MBOX_TXDONE_BY_ACK)
+ chan->txdone_method = MBOX_TXDONE_BY_POLL;
+ }
+
+ module_put(chan->mbox->dev->driver->owner);
+}
+
/**
* mbox_free_channel - The client relinquishes control of a mailbox
* channel by this call.
@@ -495,15 +507,7 @@ void mbox_free_channel(struct mbox_chan *chan)
if (chan->mbox->ops->shutdown)
chan->mbox->ops->shutdown(chan);
- /* The queued TX requests are simply aborted, no callbacks are made */
- scoped_guard(spinlock_irqsave, &chan->lock) {
- chan->cl = NULL;
- chan->active_req = MBOX_NO_MSG;
- if (chan->txdone_method == MBOX_TXDONE_BY_ACK)
- chan->txdone_method = MBOX_TXDONE_BY_POLL;
- }
-
- module_put(chan->mbox->dev->driver->owner);
+ mbox_clean_and_put_channel(chan);
}
EXPORT_SYMBOL_GPL(mbox_free_channel);
--
2.51.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] mailbox: don't free the channel if the startup callback failed
2026-04-20 11:41 [PATCH v2] mailbox: don't free the channel if the startup callback failed Wolfram Sang
@ 2026-04-21 15:07 ` kernel test robot
2026-04-22 10:43 ` kernel test robot
1 sibling, 0 replies; 3+ messages in thread
From: kernel test robot @ 2026-04-21 15:07 UTC (permalink / raw)
To: Wolfram Sang, linux-renesas-soc
Cc: oe-kbuild-all, linux-kernel, Wolfram Sang, Jassi Brar, Mark Brown
Hi Wolfram,
kernel test robot noticed the following build errors:
[auto build test ERROR on jassibrar-mailbox/for-next]
[also build test ERROR on next-20260420]
[cannot apply to linus/master v7.0]
[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/Wolfram-Sang/mailbox-don-t-free-the-channel-if-the-startup-callback-failed/20260420-234226
base: https://git.kernel.org/pub/scm/linux/kernel/git/jassibrar/mailbox.git for-next
patch link: https://lore.kernel.org/r/20260420114346.10586-2-wsa%2Brenesas%40sang-engineering.com
patch subject: [PATCH v2] mailbox: don't free the channel if the startup callback failed
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20260421/202604212338.ff2P1FQg-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260421/202604212338.ff2P1FQg-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202604212338.ff2P1FQg-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/mailbox/mailbox.c: In function '__mbox_bind_client':
>> drivers/mailbox/mailbox.c:355:25: error: implicit declaration of function 'mbox_clean_and_put_channel' [-Wimplicit-function-declaration]
355 | mbox_clean_and_put_channel(chan);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/mailbox/mailbox.c: At top level:
>> drivers/mailbox/mailbox.c:484:6: warning: no previous prototype for 'mbox_clean_and_put_channel' [-Wmissing-prototypes]
484 | void mbox_clean_and_put_channel(struct mbox_chan *chan)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/mailbox/mailbox.c:484:6: warning: conflicting types for 'mbox_clean_and_put_channel'; have 'void(struct mbox_chan *)'
drivers/mailbox/mailbox.c:355:25: note: previous implicit declaration of 'mbox_clean_and_put_channel' with type 'void(struct mbox_chan *)'
355 | mbox_clean_and_put_channel(chan);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/mbox_clean_and_put_channel +355 drivers/mailbox/mailbox.c
329
330 static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
331 {
332 struct device *dev = cl->dev;
333 int ret;
334
335 if (chan->cl || !try_module_get(chan->mbox->dev->driver->owner)) {
336 dev_err(dev, "%s: mailbox not free\n", __func__);
337 return -EBUSY;
338 }
339
340 scoped_guard(spinlock_irqsave, &chan->lock) {
341 chan->msg_free = 0;
342 chan->msg_count = 0;
343 chan->active_req = MBOX_NO_MSG;
344 chan->cl = cl;
345 init_completion(&chan->tx_complete);
346
347 if (chan->txdone_method == MBOX_TXDONE_BY_POLL && cl->knows_txdone)
348 chan->txdone_method = MBOX_TXDONE_BY_ACK;
349 }
350
351 if (chan->mbox->ops->startup) {
352 ret = chan->mbox->ops->startup(chan);
353 if (ret) {
354 dev_err(dev, "Unable to startup the chan (%d)\n", ret);
> 355 mbox_clean_and_put_channel(chan);
356 return ret;
357 }
358 }
359
360 return 0;
361 }
362
363 /**
364 * mbox_bind_client - Bind client to a mailbox channel.
365 * @chan: The mailbox channel to bind the client to.
366 * @cl: Identity of the client requesting the channel.
367 *
368 * The Client specifies its requirements and capabilities while asking for
369 * a mailbox channel. It can't be called from atomic context.
370 * The channel is exclusively allocated and can't be used by another
371 * client before the owner calls mbox_free_channel.
372 * After assignment, any packet received on this channel will be
373 * handed over to the client via the 'rx_callback'.
374 * The framework holds reference to the client, so the mbox_client
375 * structure shouldn't be modified until the mbox_free_channel returns.
376 *
377 * Return: 0 if the channel was assigned to the client successfully.
378 * <0 for request failure.
379 */
380 int mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
381 {
382 guard(mutex)(&con_mutex);
383
384 return __mbox_bind_client(chan, cl);
385 }
386 EXPORT_SYMBOL_GPL(mbox_bind_client);
387
388 /**
389 * mbox_request_channel - Request a mailbox channel.
390 * @cl: Identity of the client requesting the channel.
391 * @index: Index of mailbox specifier in 'mboxes' property.
392 *
393 * The Client specifies its requirements and capabilities while asking for
394 * a mailbox channel. It can't be called from atomic context.
395 * The channel is exclusively allocated and can't be used by another
396 * client before the owner calls mbox_free_channel.
397 * After assignment, any packet received on this channel will be
398 * handed over to the client via the 'rx_callback'.
399 * The framework holds reference to the client, so the mbox_client
400 * structure shouldn't be modified until the mbox_free_channel returns.
401 *
402 * Return: Pointer to the channel assigned to the client if successful.
403 * ERR_PTR for request failure.
404 */
405 struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
406 {
407 struct fwnode_reference_args fwspec;
408 struct fwnode_handle *fwnode;
409 struct mbox_controller *mbox;
410 struct of_phandle_args spec;
411 struct mbox_chan *chan;
412 struct device *dev;
413 unsigned int i;
414 int ret;
415
416 dev = cl->dev;
417 if (!dev) {
418 pr_debug("No owner device\n");
419 return ERR_PTR(-ENODEV);
420 }
421
422 fwnode = dev_fwnode(dev);
423 if (!fwnode) {
424 dev_dbg(dev, "No owner fwnode\n");
425 return ERR_PTR(-ENODEV);
426 }
427
428 ret = fwnode_property_get_reference_args(fwnode, "mboxes", "#mbox-cells",
429 0, index, &fwspec);
430 if (ret) {
431 dev_err(dev, "%s: can't parse \"%s\" property\n", __func__, "mboxes");
432 return ERR_PTR(ret);
433 }
434
435 spec.np = to_of_node(fwspec.fwnode);
436 spec.args_count = fwspec.nargs;
437 for (i = 0; i < spec.args_count; i++)
438 spec.args[i] = fwspec.args[i];
439
440 scoped_guard(mutex, &con_mutex) {
441 chan = ERR_PTR(-EPROBE_DEFER);
442 list_for_each_entry(mbox, &mbox_cons, node) {
443 if (device_match_fwnode(mbox->dev, fwspec.fwnode)) {
444 if (mbox->fw_xlate) {
445 chan = mbox->fw_xlate(mbox, &fwspec);
446 if (!IS_ERR(chan))
447 break;
448 } else if (mbox->of_xlate) {
449 chan = mbox->of_xlate(mbox, &spec);
450 if (!IS_ERR(chan))
451 break;
452 }
453 }
454 }
455
456 fwnode_handle_put(fwspec.fwnode);
457
458 if (IS_ERR(chan))
459 return chan;
460
461 ret = __mbox_bind_client(chan, cl);
462 if (ret)
463 chan = ERR_PTR(ret);
464 }
465
466 return chan;
467 }
468 EXPORT_SYMBOL_GPL(mbox_request_channel);
469
470 struct mbox_chan *mbox_request_channel_byname(struct mbox_client *cl,
471 const char *name)
472 {
473 int index = device_property_match_string(cl->dev, "mbox-names", name);
474
475 if (index < 0) {
476 dev_err(cl->dev, "%s() could not locate channel named \"%s\"\n",
477 __func__, name);
478 return ERR_PTR(index);
479 }
480 return mbox_request_channel(cl, index);
481 }
482 EXPORT_SYMBOL_GPL(mbox_request_channel_byname);
483
> 484 void mbox_clean_and_put_channel(struct mbox_chan *chan)
485 {
486 /* The queued TX requests are simply aborted, no callbacks are made */
487 scoped_guard(spinlock_irqsave, &chan->lock) {
488 chan->cl = NULL;
489 chan->active_req = MBOX_NO_MSG;
490 if (chan->txdone_method == MBOX_TXDONE_BY_ACK)
491 chan->txdone_method = MBOX_TXDONE_BY_POLL;
492 }
493
494 module_put(chan->mbox->dev->driver->owner);
495 }
496
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] mailbox: don't free the channel if the startup callback failed
2026-04-20 11:41 [PATCH v2] mailbox: don't free the channel if the startup callback failed Wolfram Sang
2026-04-21 15:07 ` kernel test robot
@ 2026-04-22 10:43 ` kernel test robot
1 sibling, 0 replies; 3+ messages in thread
From: kernel test robot @ 2026-04-22 10:43 UTC (permalink / raw)
To: Wolfram Sang, linux-renesas-soc
Cc: llvm, oe-kbuild-all, linux-kernel, Wolfram Sang, Jassi Brar,
Mark Brown
Hi Wolfram,
kernel test robot noticed the following build errors:
[auto build test ERROR on jassibrar-mailbox/for-next]
[also build test ERROR on next-20260421]
[cannot apply to linus/master v7.0]
[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/Wolfram-Sang/mailbox-don-t-free-the-channel-if-the-startup-callback-failed/20260420-234226
base: https://git.kernel.org/pub/scm/linux/kernel/git/jassibrar/mailbox.git for-next
patch link: https://lore.kernel.org/r/20260420114346.10586-2-wsa%2Brenesas%40sang-engineering.com
patch subject: [PATCH v2] mailbox: don't free the channel if the startup callback failed
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20260422/202604221846.9nkpd0oi-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260422/202604221846.9nkpd0oi-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202604221846.9nkpd0oi-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/mailbox/mailbox.c:355:4: error: call to undeclared function 'mbox_clean_and_put_channel'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
355 | mbox_clean_and_put_channel(chan);
| ^
>> drivers/mailbox/mailbox.c:484:6: error: conflicting types for 'mbox_clean_and_put_channel'
484 | void mbox_clean_and_put_channel(struct mbox_chan *chan)
| ^
drivers/mailbox/mailbox.c:355:4: note: previous implicit declaration is here
355 | mbox_clean_and_put_channel(chan);
| ^
2 errors generated.
vim +/mbox_clean_and_put_channel +355 drivers/mailbox/mailbox.c
329
330 static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
331 {
332 struct device *dev = cl->dev;
333 int ret;
334
335 if (chan->cl || !try_module_get(chan->mbox->dev->driver->owner)) {
336 dev_err(dev, "%s: mailbox not free\n", __func__);
337 return -EBUSY;
338 }
339
340 scoped_guard(spinlock_irqsave, &chan->lock) {
341 chan->msg_free = 0;
342 chan->msg_count = 0;
343 chan->active_req = MBOX_NO_MSG;
344 chan->cl = cl;
345 init_completion(&chan->tx_complete);
346
347 if (chan->txdone_method == MBOX_TXDONE_BY_POLL && cl->knows_txdone)
348 chan->txdone_method = MBOX_TXDONE_BY_ACK;
349 }
350
351 if (chan->mbox->ops->startup) {
352 ret = chan->mbox->ops->startup(chan);
353 if (ret) {
354 dev_err(dev, "Unable to startup the chan (%d)\n", ret);
> 355 mbox_clean_and_put_channel(chan);
356 return ret;
357 }
358 }
359
360 return 0;
361 }
362
363 /**
364 * mbox_bind_client - Bind client to a mailbox channel.
365 * @chan: The mailbox channel to bind the client to.
366 * @cl: Identity of the client requesting the channel.
367 *
368 * The Client specifies its requirements and capabilities while asking for
369 * a mailbox channel. It can't be called from atomic context.
370 * The channel is exclusively allocated and can't be used by another
371 * client before the owner calls mbox_free_channel.
372 * After assignment, any packet received on this channel will be
373 * handed over to the client via the 'rx_callback'.
374 * The framework holds reference to the client, so the mbox_client
375 * structure shouldn't be modified until the mbox_free_channel returns.
376 *
377 * Return: 0 if the channel was assigned to the client successfully.
378 * <0 for request failure.
379 */
380 int mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
381 {
382 guard(mutex)(&con_mutex);
383
384 return __mbox_bind_client(chan, cl);
385 }
386 EXPORT_SYMBOL_GPL(mbox_bind_client);
387
388 /**
389 * mbox_request_channel - Request a mailbox channel.
390 * @cl: Identity of the client requesting the channel.
391 * @index: Index of mailbox specifier in 'mboxes' property.
392 *
393 * The Client specifies its requirements and capabilities while asking for
394 * a mailbox channel. It can't be called from atomic context.
395 * The channel is exclusively allocated and can't be used by another
396 * client before the owner calls mbox_free_channel.
397 * After assignment, any packet received on this channel will be
398 * handed over to the client via the 'rx_callback'.
399 * The framework holds reference to the client, so the mbox_client
400 * structure shouldn't be modified until the mbox_free_channel returns.
401 *
402 * Return: Pointer to the channel assigned to the client if successful.
403 * ERR_PTR for request failure.
404 */
405 struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
406 {
407 struct fwnode_reference_args fwspec;
408 struct fwnode_handle *fwnode;
409 struct mbox_controller *mbox;
410 struct of_phandle_args spec;
411 struct mbox_chan *chan;
412 struct device *dev;
413 unsigned int i;
414 int ret;
415
416 dev = cl->dev;
417 if (!dev) {
418 pr_debug("No owner device\n");
419 return ERR_PTR(-ENODEV);
420 }
421
422 fwnode = dev_fwnode(dev);
423 if (!fwnode) {
424 dev_dbg(dev, "No owner fwnode\n");
425 return ERR_PTR(-ENODEV);
426 }
427
428 ret = fwnode_property_get_reference_args(fwnode, "mboxes", "#mbox-cells",
429 0, index, &fwspec);
430 if (ret) {
431 dev_err(dev, "%s: can't parse \"%s\" property\n", __func__, "mboxes");
432 return ERR_PTR(ret);
433 }
434
435 spec.np = to_of_node(fwspec.fwnode);
436 spec.args_count = fwspec.nargs;
437 for (i = 0; i < spec.args_count; i++)
438 spec.args[i] = fwspec.args[i];
439
440 scoped_guard(mutex, &con_mutex) {
441 chan = ERR_PTR(-EPROBE_DEFER);
442 list_for_each_entry(mbox, &mbox_cons, node) {
443 if (device_match_fwnode(mbox->dev, fwspec.fwnode)) {
444 if (mbox->fw_xlate) {
445 chan = mbox->fw_xlate(mbox, &fwspec);
446 if (!IS_ERR(chan))
447 break;
448 } else if (mbox->of_xlate) {
449 chan = mbox->of_xlate(mbox, &spec);
450 if (!IS_ERR(chan))
451 break;
452 }
453 }
454 }
455
456 fwnode_handle_put(fwspec.fwnode);
457
458 if (IS_ERR(chan))
459 return chan;
460
461 ret = __mbox_bind_client(chan, cl);
462 if (ret)
463 chan = ERR_PTR(ret);
464 }
465
466 return chan;
467 }
468 EXPORT_SYMBOL_GPL(mbox_request_channel);
469
470 struct mbox_chan *mbox_request_channel_byname(struct mbox_client *cl,
471 const char *name)
472 {
473 int index = device_property_match_string(cl->dev, "mbox-names", name);
474
475 if (index < 0) {
476 dev_err(cl->dev, "%s() could not locate channel named \"%s\"\n",
477 __func__, name);
478 return ERR_PTR(index);
479 }
480 return mbox_request_channel(cl, index);
481 }
482 EXPORT_SYMBOL_GPL(mbox_request_channel_byname);
483
> 484 void mbox_clean_and_put_channel(struct mbox_chan *chan)
485 {
486 /* The queued TX requests are simply aborted, no callbacks are made */
487 scoped_guard(spinlock_irqsave, &chan->lock) {
488 chan->cl = NULL;
489 chan->active_req = MBOX_NO_MSG;
490 if (chan->txdone_method == MBOX_TXDONE_BY_ACK)
491 chan->txdone_method = MBOX_TXDONE_BY_POLL;
492 }
493
494 module_put(chan->mbox->dev->driver->owner);
495 }
496
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-04-22 10:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-20 11:41 [PATCH v2] mailbox: don't free the channel if the startup callback failed Wolfram Sang
2026-04-21 15:07 ` kernel test robot
2026-04-22 10:43 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox