linux-kernel - Re: [PATCH v8 5/9] mailbox: add CIX mailbox driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABb+yY2fj13YDCYD9B-Hwta47=+CLy6eGSOOc_ez2HrR4-xbjg@mail.gmail.com>
Date: Mon, 19 May 2025 12:46:54 -0500
From: Jassi Brar <jassisinghbrar@...il.com>
To: Peter Chen <peter.chen@...tech.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	catalin.marinas@....com, will@...nel.org, arnd@...db.de, 
	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, cix-kernel-upstream@...tech.com, maz@...nel.org, 
	sudeep.holla@....com, kajetan.puchalski@....com, eballetb@...hat.com, 
	Guomin Chen <Guomin.Chen@...tech.com>, Gary Yang <gary.yang@...tech.com>, 
	Lihua Liu <Lihua.Liu@...tech.com>
Subject: Re: [PATCH v8 5/9] mailbox: add CIX mailbox driver

Hi,


> diff --git a/drivers/mailbox/cix-mailbox.c b/drivers/mailbox/cix-mailbox.c
> new file mode 100644
> index 000000000000..c2783dd7d145
> --- /dev/null
> +++ b/drivers/mailbox/cix-mailbox.c
> @@ -0,0 +1,632 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025 Cix Technology Group Co., Ltd.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include "mailbox.h"
> +
> +/* Register define */
> +#define REG_MSG(n)     (0x0 + 0x4*(n))                 /* 0x0~0x7c */
> +#define REG_DB_ACK     REG_MSG(CIX_MBOX_MSG_LEN)       /* 0x80 */
> +#define ERR_COMP       (REG_DB_ACK + 0x4)              /* 0x84 */
> +#define ERR_COMP_CLR   (REG_DB_ACK + 0x8)              /* 0x88 */
> +#define REG_F_INT(IDX) (ERR_COMP_CLR + 0x4*(IDX+1))    /* 0x8c~0xa8 */
> +#define FIFO_WR                (REG_F_INT(MBOX_FAST_IDX+1))    /* 0xac */
> +#define FIFO_RD                (FIFO_WR + 0x4)                 /* 0xb0 */
> +#define FIFO_STAS      (FIFO_WR + 0x8)                 /* 0xb4 */
> +#define FIFO_WM                (FIFO_WR + 0xc)                 /* 0xb8 */
> +#define INT_ENABLE     (FIFO_WR + 0x10)                /* 0xbc */
> +#define INT_ENABLE_SIDE_B      (FIFO_WR + 0x14)        /* 0xc0 */
> +#define INT_CLEAR      (FIFO_WR + 0x18)                /* 0xc4 */
> +#define INT_STATUS     (FIFO_WR + 0x1c)                /* 0xc8 */
> +#define FIFO_RST       (FIFO_WR + 0x20)                /* 0xcc */
> +
> +/* [0~7] Fast channel
> + * [8] doorbell base channel
> + * [9]fifo base channel
> + * [10] register base channel
> + */
> +#define CIX_MBOX_CHANS         (11)
> +
> +/*
> + * The maximum transmission size is 32 words or 128 bytes.
> + */
> +#define CIX_MBOX_MSG_LEN       (32)    /* Max length = 32 words */
> +#define MBOX_MSG_LEN_MASK      (0x7fL) /* Max length = 128 bytes */
> +
>
Move these above register defines where these are used.
Also, no need for brackets around numbers. Here and elsewhere.
....

> +
> +static void cix_mbox_isr_reg(struct mbox_chan *chan)
> +{
> +       struct cix_mbox_priv *priv = to_cix_mbox_priv(chan->mbox);
> +       u32 int_status;
> +       u32 data[CIX_MBOX_MSG_LEN];
> +       int i;
> +       u32 len;
>
cosmetic: tidy these up by merging and sorting in reverse christmas tree.


> +
> +       int_status = cix_mbox_read(priv, INT_STATUS);
> +
> +       if (priv->dir == MBOX_RX) {
> +               /* rx interrupt is triggered */
> +               if (int_status & DB_INT) {
> +                       cix_mbox_write(priv, DB_INT, INT_CLEAR);
> +                       data[0] = cix_mbox_read(priv, REG_MSG(0));
> +                       len = mbox_get_msg_size(data);
> +                       for (i = 0; i < len; i++)
> +                               data[i] = cix_mbox_read(priv, REG_MSG(i));
> +
> +                       /* trigger ack interrupt */
> +                       cix_mbox_write(priv, DB_ACK_INT_BIT, REG_DB_ACK);
> +                       mbox_chan_received_data(chan, data);
> +               }
> +       } else {
> +               /* tx ack interrupt is triggered */
> +               if (int_status & ACK_INT) {
> +                       cix_mbox_write(priv, ACK_INT, INT_CLEAR);
> +                       mbox_chan_txdone(chan, 0);
> +               }
> +       }
> +}
> +
> +static void cix_mbox_isr_fifo(struct mbox_chan *chan)
> +{
> +       struct cix_mbox_priv *priv = to_cix_mbox_priv(chan->mbox);
> +       u32 data[CIX_MBOX_MSG_LEN] = { 0 };
>
Is it really needed? Can we do with just zeroing the byte after valid data?
At least move it under "FIFO waterMark interrupt is generated", so it
is only done when needed.

....
> +
> +static int cix_mbox_startup(struct mbox_chan *chan)
> +{
> +       struct cix_mbox_priv *priv = to_cix_mbox_priv(chan->mbox);
> +       struct cix_mbox_con_priv *cp = chan->con_priv;
> +       int ret;
> +       int index = cp->index;
> +       u32 val_32;
> +
> +       ret = request_irq(priv->irq, cix_mbox_isr, 0,
> +                         dev_name(priv->dev), chan);
>
Can we do this later just before returning from the function? Or
atleast free the irq before error returns.

Also please make sure you run scripts/checkpatch and have all warnings cleared.

Thanks
Jassi