| 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
| ||
|
Message-ID: <aCv/ksNjpYcOMZCj@gchen>
Date: Tue, 20 May 2025 04:05:38 +0000
From: Guomin chen <guomin.chen@...tech.com>
To: Jassi Brar <jassisinghbrar@...il.com>
Cc: peter.chen@...tech.com, 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 <cix-kernel-upstream@...tech.com>,
maz@...nel.org, sudeep.holla@....com, kajetan.puchalski@....com,
eballetb@...hat.com, Gary Yang <gary.yang@...tech.com>,
Lihua Liu <Lihua.Liu@...tech.com>
Subject: Re: [PATCH v8 5/9] mailbox: add CIX mailbox driver
On Mon, May 19, 2025 at 12:46:54PM -0500, Jassi Brar wrote:
> [Some people who received this message don't often get email from jassisinghbrar@...il.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL
>
> Hi,
>
Hi Jassi
Thank you for your feedback.
> > 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.
> ....
Okay, I will move these two macros to the beginning and
remove the brackets around numbers.
> > +
> > +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.
>
Okay, I will make an overall adjustment according to this rule.
> > +
> > + 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.
Yes, In some cases it is not necessary.
I will move this under "FIFO waterMark interrupt is generated."
> > +
> > +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.
This cannot be done before the return, as it needs to be registered
before the interrupt enable register.
However, I do need to free this IRQ before the error return.
> Also please make sure you run scripts/checkpatch and have all warnings cleared.
I have already run scripts/checkpatch to perform the check.
The warning you mentioned is due to the need to update the MAINTAINERS file for
the newly added files, right?
Thanks
Guomin Chen
Powered by blists - more mailing lists