[PATCH] add chan->cl check in mbox_chan_received_data()

[PATCH] add chan->cl check in mbox_chan_received_data()

[Haidong Yao]

From: Haidong Yao <haidong.yao@unisoc.com>

mailbox outbox irq is coming, but mbox_request_channel
is not be registered, so cl->rx_callback is NULL.

panic log:
[    9.852090]c0  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
[    9.954634]c0  pstate: 60400089 (nZCv daIf +PAN -UAO)
[    9.954651]c0  pc : mbox_chan_received_data+0x1c/0x88
[    9.954666]c0  lr : sprd_mbox_outbox_isr+0x1d0/0x204 [sprd_mailbox]
[    9.967439]c0  sp : ffffffc010003e10
[    9.967443]c0  x29: ffffffc010003e20 x28: ffffffc011c2f6c0-
[    9.984918]c0  x27: ffffffc010e92e08 x26: 0000000000000001-
[   10.140344]c0  x25: 0000000000000378 x24: ffffff80f4064130-
[   10.145880]c0  x23: 0000000000000001 x22: ffffffc0091072c7-
[   10.151418]c0  x21: ffffffc009107212 x20: 0000000000000005-
[   10.156957]c0  x19: ffffff80f4064080 x18: ffffffc010005038-
[   10.162494]c0  x17: 0000000000000000 x16: ffffffc010e6f844-
[   10.168033]c0  x15: ffffffc0117abac7 x14: 000000000000003f-
[   10.173571]c0  x13: ffff0000ffffff00 x12: ffff0a01ffffff10-
[   10.179110]c0  x11: 0000000000000001 x10: 00000000ffffffff-
[   10.184649]c0  x9 : ffffff80f40644a8 x8 : c366877097809900-
[   10.190187]c0  x7 : 207273695f786f62 x6 : ffffffc011d62231-
[   10.195726]c0  x5 : 0000000000000034 x4 : 000000000000000c-
[   10.201265]c0  x3 : ffffffc010e9842c x2 : 0000000000000001-
[   10.206803]c0  x1 : ffffffc010003e40 x0 : 0000000000000000-
[   10.212343]c0  Call trace:
[   10.215029]c0   mbox_chan_received_data+0x1c/0x88
[   10.219705]c0   sprd_mbox_outbox_isr+0x1d0/0x204 [sprd_mailbox]
[   10.225590]c0   __handle_irq_event_percpu+0x164/0x358
[   10.230604]c0   handle_irq_event+0x60/0xd8
[   10.234675]c0   handle_fasteoi_irq+0x128/0x32c
[   10.239086]c0   __handle_domain_irq+0xa0/0x100
[   10.243502]c0   efi_header_end+0xb8/0x15c
[   10.247478]c0   el1_irq+0x104/0x200
[   10.250945]c0   cpuidle_enter_state+0x158/0x2d8
[   10.255440]c0   cpuidle_enter+0x38/0x50
[   10.259253]c0   do_idle.llvm.10091284334483161164+0x1a4/0x294
[   10.264963]c0   cpu_startup_entry+0x24/0x28
[   10.269120]c0   kernel_init+0x0/0x29c
[   10.272752]c0   start_kernel+0x0/0x418
[   10.276468]c0   start_kernel+0x3a0/0x418
[   10.280371]c0  Code: f90013f3 910043fd aa0003e9 f9400800 (f9401008)-
[   10.286684]c0  ---[ end trace b868997a960c667a ]---

Signed-off-by: Haidong Yao <haidong.yao@unisoc.com>
---
 drivers/mailbox/mailbox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 3e7d4b20ab34..58697298a95f 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -152,7 +152,7 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
 void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
 {
 	/* No buffering the received data */
-	if (chan->cl->rx_callback)
+	if (chan->cl && chan->cl->rx_callback)
 		chan->cl->rx_callback(chan->cl, mssg);
 }
 EXPORT_SYMBOL_GPL(mbox_chan_received_data);
-- 
2.28.0

Re: [PATCH] add chan->cl check in mbox_chan_received_data()

[Jassi Brar]

On Tue, Dec 15, 2020 at 2:48 AM Haidong Yao <yaohaidong369@gmail.com> wrote:

> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -152,7 +152,7 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
>  void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
>  {
>         /* No buffering the received data */
> -       if (chan->cl->rx_callback)
> +       if (chan->cl && chan->cl->rx_callback)
>                 chan->cl->rx_callback(chan->cl, mssg);
>  }
The proper fix is in the controller driver. Which should stop tx/rx
when the channel is freed.

thnx.

Re: [PATCH] add chan->cl check in mbox_chan_received_data()

[Orson Zhai]

On Tue, Dec 15, 2020 at 4:48 PM Haidong Yao <yaohaidong369@gmail.com> wrote:
>
> From: Haidong Yao <haidong.yao@unisoc.com>
>
> mailbox outbox irq is coming, but mbox_request_channel
> is not be registered, so cl->rx_callback is NULL.

Both "chan->cl" and "chan->cl->rx_callback" should be checked for being NULL.


>
> panic log:
> [    9.852090]c0  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
> [    9.954634]c0  pstate: 60400089 (nZCv daIf +PAN -UAO)
> [    9.954651]c0  pc : mbox_chan_received_data+0x1c/0x88
> [    9.954666]c0  lr : sprd_mbox_outbox_isr+0x1d0/0x204 [sprd_mailbox]
> [    9.967439]c0  sp : ffffffc010003e10
> [    9.967443]c0  x29: ffffffc010003e20 x28: ffffffc011c2f6c0-
> [    9.984918]c0  x27: ffffffc010e92e08 x26: 0000000000000001-
> [   10.140344]c0  x25: 0000000000000378 x24: ffffff80f4064130-
> [   10.145880]c0  x23: 0000000000000001 x22: ffffffc0091072c7-
> [   10.151418]c0  x21: ffffffc009107212 x20: 0000000000000005-
> [   10.156957]c0  x19: ffffff80f4064080 x18: ffffffc010005038-
> [   10.162494]c0  x17: 0000000000000000 x16: ffffffc010e6f844-
> [   10.168033]c0  x15: ffffffc0117abac7 x14: 000000000000003f-
> [   10.173571]c0  x13: ffff0000ffffff00 x12: ffff0a01ffffff10-
> [   10.179110]c0  x11: 0000000000000001 x10: 00000000ffffffff-
> [   10.184649]c0  x9 : ffffff80f40644a8 x8 : c366877097809900-
> [   10.190187]c0  x7 : 207273695f786f62 x6 : ffffffc011d62231-
> [   10.195726]c0  x5 : 0000000000000034 x4 : 000000000000000c-
> [   10.201265]c0  x3 : ffffffc010e9842c x2 : 0000000000000001-
> [   10.206803]c0  x1 : ffffffc010003e40 x0 : 0000000000000000-
> [   10.212343]c0  Call trace:
> [   10.215029]c0   mbox_chan_received_data+0x1c/0x88
> [   10.219705]c0   sprd_mbox_outbox_isr+0x1d0/0x204 [sprd_mailbox]
> [   10.225590]c0   __handle_irq_event_percpu+0x164/0x358
> [   10.230604]c0   handle_irq_event+0x60/0xd8
> [   10.234675]c0   handle_fasteoi_irq+0x128/0x32c
> [   10.239086]c0   __handle_domain_irq+0xa0/0x100
> [   10.243502]c0   efi_header_end+0xb8/0x15c
> [   10.247478]c0   el1_irq+0x104/0x200
> [   10.250945]c0   cpuidle_enter_state+0x158/0x2d8
> [   10.255440]c0   cpuidle_enter+0x38/0x50
> [   10.259253]c0   do_idle.llvm.10091284334483161164+0x1a4/0x294
> [   10.264963]c0   cpu_startup_entry+0x24/0x28
> [   10.269120]c0   kernel_init+0x0/0x29c
> [   10.272752]c0   start_kernel+0x0/0x418
> [   10.276468]c0   start_kernel+0x3a0/0x418
> [   10.280371]c0  Code: f90013f3 910043fd aa0003e9 f9400800 (f9401008)-
> [   10.286684]c0  ---[ end trace b868997a960c667a ]---
>
> Signed-off-by: Haidong Yao <haidong.yao@unisoc.com>
> ---
>  drivers/mailbox/mailbox.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 3e7d4b20ab34..58697298a95f 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -152,7 +152,7 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
>  void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
>  {
>         /* No buffering the received data */
> -       if (chan->cl->rx_callback)
> +       if (chan->cl && chan->cl->rx_callback)
>                 chan->cl->rx_callback(chan->cl, mssg);
>  }
>  EXPORT_SYMBOL_GPL(mbox_chan_received_data);

For the code changes,
Acked-by: Orson Zhai <orsonzhai@gmail.com>

> --
> 2.28.0
>

Re: [PATCH] add chan->cl check in mbox_chan_received_data()

[haidong yao]

Hi Jassi Brar

Thank you very much for your reply.

Look at the function sprd_mbox_outbox_isr .

Chan is !NULL.

chan->cl is NULL when the client driver not loaded, the controller
driver don't know the client driver loaded successfully, so, I do not
use mbox_free_channel.

Here,How do you know chan->cl is ok?

chan = &priv->chan[id];
mbox_chan_received_data(chan, (void *)msg);

static irqreturn_t sprd_mbox_outbox_isr(int irq, void *data)
{
        struct sprd_mbox_priv *priv = data;
        struct mbox_chan *chan;
        u32 fifo_sts, fifo_len, msg[2];
        int i, id;

        fifo_sts = readl(priv->outbox_base + SPRD_MBOX_FIFO_STS);

        fifo_len = sprd_mbox_get_fifo_len(priv, fifo_sts);
        if (!fifo_len) {
                dev_warn_ratelimited(priv->dev, "spurious outbox interrupt\n");
                return IRQ_NONE;
        }

        for (i = 0; i < fifo_len; i++) {
                msg[0] = readl(priv->outbox_base + SPRD_MBOX_MSG_LOW);
                msg[1] = readl(priv->outbox_base + SPRD_MBOX_MSG_HIGH);
                id = readl(priv->outbox_base + SPRD_MBOX_ID);

                chan = &priv->chan[id];
                mbox_chan_received_data(chan, (void *)msg);

                /* Trigger to update outbox FIFO pointer */
                writel(0x1, priv->outbox_base + SPRD_MBOX_TRIGGER);
        }

        /* Clear irq status after reading all message. */
        writel(SPRD_MBOX_IRQ_CLR, priv->outbox_base + SPRD_MBOX_IRQ_STS);

        return IRQ_HANDLED;
}


On Tue, Dec 15, 2020 at 2:48 AM Haidong Yao <yaohaidong369@gmail.com> wrote:

> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -152,7 +152,7 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
>  void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
>  {
>         /* No buffering the received data */
> -       if (chan->cl->rx_callback)
> +       if (chan->cl && chan->cl->rx_callback)
>                 chan->cl->rx_callback(chan->cl, mssg);
>  }
The proper fix is in the controller driver. Which should stop tx/rx
when the channel is freed.

thnx.



2020-12-15 16:48 GMT+08:00, Haidong Yao <yaohaidong369@gmail.com>:
> From: Haidong Yao <haidong.yao@unisoc.com>
>
> mailbox outbox irq is coming, but mbox_request_channel
> is not be registered, so cl->rx_callback is NULL.
>
> panic log:
> [    9.852090]c0  Unable to handle kernel NULL pointer dereference at
> virtual address 0000000000000020
> [    9.954634]c0  pstate: 60400089 (nZCv daIf +PAN -UAO)
> [    9.954651]c0  pc : mbox_chan_received_data+0x1c/0x88
> [    9.954666]c0  lr : sprd_mbox_outbox_isr+0x1d0/0x204 [sprd_mailbox]
> [    9.967439]c0  sp : ffffffc010003e10
> [    9.967443]c0  x29: ffffffc010003e20 x28: ffffffc011c2f6c0-
> [    9.984918]c0  x27: ffffffc010e92e08 x26: 0000000000000001-
> [   10.140344]c0  x25: 0000000000000378 x24: ffffff80f4064130-
> [   10.145880]c0  x23: 0000000000000001 x22: ffffffc0091072c7-
> [   10.151418]c0  x21: ffffffc009107212 x20: 0000000000000005-
> [   10.156957]c0  x19: ffffff80f4064080 x18: ffffffc010005038-
> [   10.162494]c0  x17: 0000000000000000 x16: ffffffc010e6f844-
> [   10.168033]c0  x15: ffffffc0117abac7 x14: 000000000000003f-
> [   10.173571]c0  x13: ffff0000ffffff00 x12: ffff0a01ffffff10-
> [   10.179110]c0  x11: 0000000000000001 x10: 00000000ffffffff-
> [   10.184649]c0  x9 : ffffff80f40644a8 x8 : c366877097809900-
> [   10.190187]c0  x7 : 207273695f786f62 x6 : ffffffc011d62231-
> [   10.195726]c0  x5 : 0000000000000034 x4 : 000000000000000c-
> [   10.201265]c0  x3 : ffffffc010e9842c x2 : 0000000000000001-
> [   10.206803]c0  x1 : ffffffc010003e40 x0 : 0000000000000000-
> [   10.212343]c0  Call trace:
> [   10.215029]c0   mbox_chan_received_data+0x1c/0x88
> [   10.219705]c0   sprd_mbox_outbox_isr+0x1d0/0x204 [sprd_mailbox]
> [   10.225590]c0   __handle_irq_event_percpu+0x164/0x358
> [   10.230604]c0   handle_irq_event+0x60/0xd8
> [   10.234675]c0   handle_fasteoi_irq+0x128/0x32c
> [   10.239086]c0   __handle_domain_irq+0xa0/0x100
> [   10.243502]c0   efi_header_end+0xb8/0x15c
> [   10.247478]c0   el1_irq+0x104/0x200
> [   10.250945]c0   cpuidle_enter_state+0x158/0x2d8
> [   10.255440]c0   cpuidle_enter+0x38/0x50
> [   10.259253]c0   do_idle.llvm.10091284334483161164+0x1a4/0x294
> [   10.264963]c0   cpu_startup_entry+0x24/0x28
> [   10.269120]c0   kernel_init+0x0/0x29c
> [   10.272752]c0   start_kernel+0x0/0x418
> [   10.276468]c0   start_kernel+0x3a0/0x418
> [   10.280371]c0  Code: f90013f3 910043fd aa0003e9 f9400800 (f9401008)-
> [   10.286684]c0  ---[ end trace b868997a960c667a ]---
>
> Signed-off-by: Haidong Yao <haidong.yao@unisoc.com>
> ---
>  drivers/mailbox/mailbox.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 3e7d4b20ab34..58697298a95f 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -152,7 +152,7 @@ static enum hrtimer_restart txdone_hrtimer(struct
> hrtimer *hrtimer)
>  void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
>  {
>  	/* No buffering the received data */
> -	if (chan->cl->rx_callback)
> +	if (chan->cl && chan->cl->rx_callback)
>  		chan->cl->rx_callback(chan->cl, mssg);
>  }
>  EXPORT_SYMBOL_GPL(mbox_chan_received_data);
> --
> 2.28.0
>
>

Re: [PATCH] add chan->cl check in mbox_chan_received_data()

[Jassi Brar]

On Thu, Jan 7, 2021 at 5:53 AM haidong yao <yaohaidong369@gmail.com> wrote:
>
> Hi Jassi Brar
>
> Thank you very much for your reply.
>
> Look at the function sprd_mbox_outbox_isr .
>
> Chan is !NULL.
>
> chan->cl is NULL when the client driver not loaded, the controller
> driver don't know the client driver loaded successfully, so, I do not
> use mbox_free_channel.
>
> Here,How do you know chan->cl is ok?
>
The channel is supposed to get/send data _only_ if it is being used by a client.
Which you can mark in .startup() and .shutdown().

Checking for chan->cl will make your symptoms disappear but that is
not the right fix for the issue.
The right fix is to EITHER not cause Rx/Tx interrupt on a channel not
being used, OR not send it to upper layers.

thanks.

Re: [PATCH] add chan->cl check in mbox_chan_received_data()

[Yao Haidong]

 Oh, I see, thank you

2021-02-01 14:28 GMT+08:00, Jassi Brar <jassisinghbrar@gmail.com>:
> On Thu, Jan 7, 2021 at 5:53 AM haidong yao <yaohaidong369@gmail.com> wrote:
>>
>> Hi Jassi Brar
>>
>> Thank you very much for your reply.
>>
>> Look at the function sprd_mbox_outbox_isr .
>>
>> Chan is !NULL.
>>
>> chan->cl is NULL when the client driver not loaded, the controller
>> driver don't know the client driver loaded successfully, so, I do not
>> use mbox_free_channel.
>>
>> Here,How do you know chan->cl is ok?
>>
> The channel is supposed to get/send data _only_ if it is being used by a
> client.
> Which you can mark in .startup() and .shutdown().
>
> Checking for chan->cl will make your symptoms disappear but that is
> not the right fix for the issue.
> The right fix is to EITHER not cause Rx/Tx interrupt on a channel not
> being used, OR not send it to upper layers.
>
> thanks.
>