linux-kernel - Re: [PATCH v2 3/3] mailbox: add Microchip IPC support

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+yY3cDD-E-P1MPKQjdX7R2XVVKjwXUW-BANWcz-9aR6kskA@mail.gmail.com>
Date: Sat, 2 Nov 2024 19:23:01 -0500
From: Jassi Brar <jassisinghbrar@...il.com>
To: Valentina Fernandez <valentina.fernandezalanis@...rochip.com>
Cc: paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu, 
	peterlin@...estech.com, conor.dooley@...rochip.com, conor+dt@...nel.org, 
	ycliang@...estech.com, dminus@...estech.com, 
	prabhakar.mahadev-lad.rj@...renesas.com, robh@...nel.org, krzk+dt@...nel.org, 
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH v2 3/3] mailbox: add Microchip IPC support

On Fri, Oct 25, 2024 at 7:36 AM Valentina Fernandez
<valentina.fernandezalanis@...rochip.com> wrote:

....
> +
> +enum ipc_irq_type {
> +       IPC_OPS_NOT_SUPPORTED   = 1,
> +       IPC_MP_IRQ              = 2,
> +       IPC_MC_IRQ              = 4,
> +};
totally unused.

> +
> +/**
> + * struct mchp_ipc_probe - IPC probe message format
> + *
> + * @hw_type:           IPC implementation available in the hardware
> + * @num_channels:      number of IPC channels available in the hardware
> + *
> + * Used to retrieve information on the IPC implementation
> + * using the SBI_EXT_IPC_PROBE SBI function id.
> + */
> +struct mchp_ipc_probe {
 same as the driver.probe(), so maybe call this microchip_mbox_info

......

> +struct mchp_ipc_cluster_cfg {
> +       void *buf_base;
> +       unsigned long buf_base_addr;
> +       int irq;
> +};
> +
> +struct ipc_chan_info {
 I suggest s/ipc_chan_info/microchip_sbi_chan and hooking it to
mbox_chan.con_priv

....

> +       unsigned long buf_base_tx_addr;
> +       unsigned long buf_base_rx_addr;
> +       unsigned long msg_buf_tx_addr;
> +       unsigned long msg_buf_rx_addr;
If these are __pa(), then phys_addr_t please.

> +       int chan_aggregated_irq;
> +       int mp_irq;
> +       int mc_irq;
> +       u32 id;
> +       u32 max_msg_size;
> +};
> +
> +struct microchip_ipc {
 Maybe s/microchip_ipc/microchip_sbi_mbox ?


> +       struct device *dev;
> +       struct mbox_chan *chans;
> +       struct mchp_ipc_cluster_cfg *cluster_cfg;
> +       struct ipc_chan_info *priv;
  replace this with 'struct mbox_chan *chan' and hook
     chan[i].con_priv = priv[i]
  this will help avoid having to EXPORT mchp_ipc_get_chan_id


> +       void *buf_base;
> +       unsigned long buf_base_addr;
phys_addr_t buf_base_addr ?

> +       struct mbox_controller controller;
> +       u8 num_channels;
this could be dropped by directly using 'controller.num_chans'

......

> +static int mchp_ipc_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct ipc_chan_info *chan_info = (struct ipc_chan_info *)chan->con_priv;
> +       const struct mchp_ipc_msg *msg = data;
> +       struct mchp_ipc_sbi_msg sbi_payload;
> +
> +       memcpy(chan_info->msg_buf_tx, msg->buf, msg->size);
> +       sbi_payload.buf_addr = chan_info->msg_buf_tx_addr;
> +       sbi_payload.size = msg->size;
> +       memcpy(chan_info->buf_base_tx, &sbi_payload, sizeof(sbi_payload));
How does this work? sizeof(sbi_payload) is more than
sizeof(*chan_info->buf_base_tx)
I think buf_base_tx needs to be u8 array of max{sizeof(struct
mchp_ipc_init), sizeof(struct mchp_ipc_sbi_msg)}, if there are
alignment requirements then maybe kmalloc that size.
Similarly for buf_base_rx.

...

> +static struct platform_driver mchp_ipc_driver = {
> +       .driver = {
> +               .name = "microchip_ipc",
> +               .of_match_table = mchp_ipc_of_match,
> +       },
> +       .probe = mchp_ipc_probe,
The driver could be built as a module, so please provide .remove()
even if you never intend to unload it.

cheers.