[PATCH 0/2] mailbox: improvments around registering controllers

[PATCH 0/2] mailbox: improvments around registering controllers

[Wolfram Sang]

While diving deeper into this subsystem, I manually found these two
issues which I wanted to fix right away. Please see patch descriptions
for details.


Wolfram Sang (2):
  mailbox: add sanity check for channel array
  mailbox: update kdoc for struct mbox_controller

 drivers/mailbox/mailbox.c          | 3 +--
 include/linux/mailbox_controller.h | 9 +++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.51.0

[PATCH 1/2] mailbox: add sanity check for channel array

[Wolfram Sang]

Fail gracefully if there is no channel array attached to the mailbox
controller. Otherwise the later dereference will cause an OOPS which
might not be seen because mailbox controllers might instantiate very
early. Remove the comment explaining the obvious while here.

Fixes: 2b6d83e2b8b7 ("mailbox: Introduce framework for mailbox")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

On my Renesas R-Car X5H based board, the OOPS was indeed silent. I
injected the error manually.

 drivers/mailbox/mailbox.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 14d9655d5424..0ed34f8f7556 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -533,8 +533,7 @@ int mbox_controller_register(struct mbox_controller *mbox)
 {
 	int i, txdone;
 
-	/* Sanity check */
-	if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
+	if (!mbox || !mbox->dev || !mbox->ops || !mbox->chans || !mbox->num_chans)
 		return -EINVAL;
 
 	if (mbox->txdone_irq)
-- 
2.51.0

Re: [PATCH 1/2] mailbox: add sanity check for channel array

[Geert Uytterhoeven]

On Mon, 13 Apr 2026 at 12:42, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Fail gracefully if there is no channel array attached to the mailbox
> controller. Otherwise the later dereference will cause an OOPS which
> might not be seen because mailbox controllers might instantiate very
> early. Remove the comment explaining the obvious while here.
>
> Fixes: 2b6d83e2b8b7 ("mailbox: Introduce framework for mailbox")
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] mailbox: add sanity check for channel array

[Jassi Brar]

On Mon, Apr 13, 2026 at 5:42 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Fail gracefully if there is no channel array attached to the mailbox
> controller. Otherwise the later dereference will cause an OOPS which
> might not be seen because mailbox controllers might instantiate very
> early. Remove the comment explaining the obvious while here.
>
> Fixes: 2b6d83e2b8b7 ("mailbox: Introduce framework for mailbox")
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> On my Renesas R-Car X5H based board, the OOPS was indeed silent. I
> injected the error manually.
>
>  drivers/mailbox/mailbox.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 14d9655d5424..0ed34f8f7556 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -533,8 +533,7 @@ int mbox_controller_register(struct mbox_controller *mbox)
>  {
>         int i, txdone;
>
> -       /* Sanity check */
> -       if (!mbox || !mbox->dev || !mbox->ops || !mbox->num_chans)
> +       if (!mbox || !mbox->dev || !mbox->ops || !mbox->chans || !mbox->num_chans)
>                 return -EINVAL;
>
This sounds like a quick suggestion from some AI tool -- looks nice
but doesn't mean much.
Is your controller driver written such that it calls
mbox_controller_register() even if it failed to allocate channels ?
The code already gates access to chans by looking at num_chans. If the
controller sets num_chans to be non-zero, the chans is supposed to
point to an array of channels. NULL is just another garbage value, why
not also check for 0xdeadbabe or 0xffff... ?
I can let loose a claude session and I will find 1000s of such "fixes"
in the kernel.
But since it is here, I am picking it purely as a cosmetic change.
Thanks

Re: [PATCH 1/2] mailbox: add sanity check for channel array

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]


> This sounds like a quick suggestion from some AI tool -- looks nice
> but doesn't mean much.

Nope, from the cover letter:

While diving deeper into this subsystem, I manually found these two
issues which I wanted to fix right away.

I just wanted to check which parameters are required and which are
optional. And the array is definitely required, so the core should check
for it IMHO.

> point to an array of channels. NULL is just another garbage value, why
> not also check for 0xdeadbabe or 0xffff... ?

Because NULL is what we get if we kzalloc a structure unlike the values
you mentioned.

> I can let loose a claude session and I will find 1000s of such "fixes"
> in the kernel.

No AI was involved in the generation of these patches. I think I marked
the patches which resulted from AI review correctly so far.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] mailbox: add sanity check for channel array

[Jassi Brar]

On Sat, Apr 18, 2026 at 6:57 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> > This sounds like a quick suggestion from some AI tool -- looks nice
> > but doesn't mean much.
>
> Nope, from the cover letter:
>
> While diving deeper into this subsystem, I manually found these two
> issues which I wanted to fix right away.
>
> I just wanted to check which parameters are required and which are
> optional. And the array is definitely required, so the core should check
> for it IMHO.
>
I am saying that it is still not sufficient if we get so paranoid and
start doing what are basically static checks. What if the array at
chans is not NULL but less than num_chans?
num_chans check acts as the "required" indicator already.

> > point to an array of channels. NULL is just another garbage value, why
> > not also check for 0xdeadbabe or 0xffff... ?
>
> Because NULL is what we get if we kzalloc a structure unlike the values
> you mentioned.
>
Why can't the mbox structure be reused or be at some offset of a
bigger allocation? See, there is no end to theoretical dangers.

> > I can let loose a claude session and I will find 1000s of such "fixes"
> > in the kernel.
>
> No AI was involved in the generation of these patches. I think I marked
> the patches which resulted from AI review correctly so far.
>
To be clear, I am not against using AI tools. I use them myself. A lot.
All I am saying is its a slippery slope of "this check is good to
have, at least it doesn't hurt", with the AI tools we can generate
1000s of such patches that are only to "educate" the developer rather
than making any runtime difference.

Regards,
Jassi

[PATCH 2/2] mailbox: update kdoc for struct mbox_controller

[Wolfram Sang]

Add field for missing lock around the hrtimer. Add 'Required' where
the core checks for valid entries.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 include/linux/mailbox_controller.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index a49ee687d4cf..dc93287a2a01 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -62,10 +62,10 @@ struct mbox_chan_ops {
 
 /**
  * struct mbox_controller - Controller of a class of communication channels
- * @dev:		Device backing this controller
- * @ops:		Operators that work on each communication chan
- * @chans:		Array of channels
- * @num_chans:		Number of channels in the 'chans' array.
+ * @dev:		Device backing this controller. Required.
+ * @ops:		Operators that work on each communication chan. Required.
+ * @chans:		Array of channels. Required.
+ * @num_chans:		Number of channels in the 'chans' array. Required.
  * @txdone_irq:		Indicates if the controller can report to API when
  *			the last transmitted data was read by the remote.
  *			Eg, if it has some TX ACK irq.
@@ -78,6 +78,7 @@ struct mbox_chan_ops {
  * @of_xlate:		Controller driver specific mapping of channel via DT
  * @poll_hrt:		API private. hrtimer used to poll for TXDONE on all
  *			channels.
+ * @poll_hrt_lock:	API private. Lock protecting access to poll_hrt.
  * @node:		API private. To hook into list of controllers.
  */
 struct mbox_controller {
-- 
2.51.0

Re: [PATCH 2/2] mailbox: update kdoc for struct mbox_controller

[Geert Uytterhoeven]

On Mon, 13 Apr 2026 at 12:42, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Add field for missing lock around the hrtimer. Add 'Required' where
> the core checks for valid entries.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] mailbox: update kdoc for struct mbox_controller

[Jassi Brar]

On Thu, Apr 16, 2026 at 8:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> On Mon, 13 Apr 2026 at 12:42, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > Add field for missing lock around the hrtimer. Add 'Required' where
> > the core checks for valid entries.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>

Applied to mailbox/for-next , this and the 1/2 patch
Thanks
Jassi