linux-kernel - Re: [PATCH v9 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+yY2BmqiQ18hU+7C234UnY8n-8PH5VEoS7nH5Xq5O1krGhQ@mail.gmail.com>
Date: Sun, 13 Jul 2025 12:00:06 -0500
From: Jassi Brar <jassisinghbrar@...il.com>
To: Guomin chen <guomin.chen@...tech.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	catalin.marinas@....com, will@...nel.org, arnd@...db.de, 
	Peter Chen <peter.chen@...tech.com>, 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, 
	Gary Yang <gary.yang@...tech.com>, Lihua Liu <Lihua.Liu@...tech.com>
Subject: Re: [PATCH v9 5/9] mailbox: add CIX mailbox driver

On Tue, Jul 8, 2025 at 8:54 AM Guomin chen <guomin.chen@...tech.com> wrote:
....
> > > +/* [0~7] Fast channel
> > > + * [8] doorbell base channel
> > > + * [9]fifo base channel
> > > + * [10] register base channel
> > > + */
> > > +#define MBOX_FAST_IDX          7
> > > +#define MBOX_DB_IDX            8
> > > +#define MBOX_FIFO_IDX          9
> > > +#define MBOX_REG_IDX           10
> > > +#define CIX_MBOX_CHANS         11
> > > +
> > if it is not really a single controller owning different channels,
> > maybe implement only what you currently use.
> >
> As mentioned in the previous email, a single controller can support
> multiple different channels.
>
OK. I am not too worried about having all variants in one driver esp
when it is manageable and share the code.
Unless I am overlooking something. Arnd?


> > > +static u32 cix_mbox_read(struct cix_mbox_priv *priv, u32 offset)
> > > +{
> > > +       if (priv->use_shmem)
> > > +               return ioread32(priv->base + offset - SHMEM_OFFSET);
> > > +       else
> > > +               return ioread32(priv->base + offset);
> > > +}
> > > +
> > use_shmem is set for only CIX_MBOX_TYPE_DB, but it affects every read/write.
> > Maybe instead adjust the base for TYPE_DB?
> >
> The reason we introduced use_shmem here is that we had to adjust the base
> address of TYPE_DB to resolve the reg conflict in the DTS.
> This change has virtually no impact on performance.
>
Yes, I can see it should have no impact on performance and I think
adjusting the base once
during init is cleaner than checking the flag every read/write.
But wait... use_shmem is a controller wide flag, and isn't
priv->use_shmem always set to true  in cix_mbox_init() ?
Is the driver even tested?
....

> > > +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 index = cp->index, ret;
> > > +       u32 val_32;
> > > +
> > > +       ret = request_irq(priv->irq, cix_mbox_isr, 0,
> > > +                         dev_name(priv->dev), chan);
> > The same irq is requested for each channel. How do you expect it to
> > work? Maybe request it just once in probe and pass the 'priv' instead
> > of 'chan' , and in the cix_mbox_isr handle according to INT_STATUS
> >
> For the same mailbox controller, there won't be multiple channels
> simultaneously requesting the same IRQ, so there won't be an issue
> here. As you mentioned, if we need to handle multiple channels working
> concurrently in the future, we would need to modify cix_mbox_isr.
> However, that is not required at the moment.
>
Is it too hard to do it right already?

....
> > > +MODULE_AUTHOR("Cix Technology Group Co., Ltd.");
> > > +MODULE_DESCRIPTION("CIX mailbox driver");
> > > +MODULE_LICENSE("GPL");
> >
> > GPL v2 ? according to the SPDX-License-Identifier
> >
> > And please make sure you run scripts/checkpatch.pl
>
> Yes, I'm also confused here. I was previously using GPL v2,
> but when I ran checkpatch.pl, it triggered a warning due to
> commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE 'GPL'
> vs. 'GPL v2' bogosity").  So I changed it to GPL.
>
Sorry, I was wrong. It should simply be GPL

Thanks.