linux-kernel - Re: [RFC PATCH v2 11/17] irqchip: Add driver for the RISC-V RPMI system MSI service group

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: <CAK9=C2U537LvffPLGad=LsFsN27uW2UB2X=Oxrub9v2U5vneYQ@mail.gmail.com>
Date: Thu, 6 Feb 2025 17:47:34 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jassi Brar <jassisinghbrar@...il.com>, "Rafael J . Wysocki" <rafael@...nel.org>, 
	Mika Westerberg <mika.westerberg@...ux.intel.com>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, 
	Uwe Kleine-König <ukleinek@...nel.org>, 
	Palmer Dabbelt <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Len Brown <lenb@...nel.org>, Sunil V L <sunilvl@...tanamicro.com>, 
	Rahul Pathak <rpathak@...tanamicro.com>, Leyfoon Tan <leyfoon.tan@...rfivetech.com>, 
	Atish Patra <atishp@...shpatra.org>, Andrew Jones <ajones@...tanamicro.com>, 
	Samuel Holland <samuel.holland@...ive.com>, Anup Patel <anup@...infault.org>, 
	linux-clk@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 11/17] irqchip: Add driver for the RISC-V RPMI
 system MSI service group

Hi Thomas,

On Mon, Feb 3, 2025 at 7:20 PM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> On Mon, Feb 03 2025 at 14:19, Anup Patel wrote:
> > +
> > +struct rpmi_sysmsi_priv {
> > +     struct device *dev;
> > +     struct mbox_client client;
> > +     struct mbox_chan *chan;
> > +     u32 nr_irqs;
> > +     u32 gsi_base;
> > +};
>
> AS requested before please use tabular layout for structs:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
>
> > +static int rpmi_sysmsi_set_msi_state(struct rpmi_sysmsi_priv *priv,
> > +                                  u32 sys_msi_index, u32 sys_msi_state)
> > +{
> > +     struct rpmi_sysmsi_set_msi_state_tx tx;
> > +     struct rpmi_sysmsi_set_msi_state_rx rx;
> > +     struct rpmi_mbox_message msg;
> > +     int ret;
> > +
> > +     tx.sys_msi_index = sys_msi_index;
> > +     tx.sys_msi_state = sys_msi_state;
> > +     rpmi_mbox_init_send_with_response(&msg, RPMI_SYSMSI_SRV_SET_MSI_STATE,
> > +                                       &tx, sizeof(tx), &rx, sizeof(rx));
> > +     ret = rpmi_mbox_send_message(priv->chan, &msg);
> > +     if (ret)
> > +             return ret;
> > +     if (rx.status)
> > +             return rpmi_to_linux_error(rx.status);
> > +
> > +     return 0;
> > +}
> > +
> > +#define rpmi_sysmsi_mask(__priv, __msi_index)                \
> > +     rpmi_sysmsi_set_msi_state(__priv, __msi_index, 0)
> > +#define rpmi_sysmsi_unmask(__priv, __msi_index)              \
> > +     rpmi_sysmsi_set_msi_state(__priv, __msi_index, RPMI_SYSMSI_MSI_STATE_ENABLE)
>
> These macros are not really providing any value.
>
> > +static void rpmi_sysmsi_irq_mask(struct irq_data *d)
> > +{
> > +     struct rpmi_sysmsi_priv *priv = irq_data_get_irq_chip_data(d);
> > +     int ret;
> > +
> > +     ret = rpmi_sysmsi_mask(priv, d->hwirq);
> > +     if (ret)
> > +             dev_warn(priv->dev, "Failed to mask hwirq %d (error %d)\n",
> > +                      (u32)d->hwirq, ret);
>
>         if (ret) {
>                 ....
>         }
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules
>
> > +     irq_chip_mask_parent(d);
> > +}
>
> Other than those nits, this looks reasonable.

I will address all above comments in the next revision.

Thanks,
Anup