| 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: <CAK9=C2VYCSkMV5zoyVEKHTY30Y0KpUr_M8FELm37M8=WYi+4HQ@mail.gmail.com>
Date: Tue, 10 Jun 2025 16:33:46 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
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>, Thomas Gleixner <tglx@...utronix.de>,
"Rafael J . Wysocki" <rafael@...nel.org>, Mika Westerberg <mika.westerberg@...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 <atish.patra@...ux.dev>, 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: [PATCH v4 13/23] irqchip: Add driver for the RPMI system MSI
service group
On Tue, May 27, 2025 at 5:03 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Sun, May 25, 2025 at 02:17:00PM +0530, Anup Patel wrote:
> > The RPMI specification defines a system MSI service group which
> > allows application processors to receive MSIs upon system events
> > such as graceful shutdown/reboot request, CPU hotplug event, memory
> > hotplug event, etc.
> >
> > Add an irqchip driver for the RISC-V RPMI system MSI service group
> > to directly receive system MSIs in Linux kernel.
>
> ...
>
> > +#include <linux/device.h>
>
> Perhaps I missed something, but devm_kzalloc() is in device/devres.h. Do you
> need it for something else?
dev_get_msi_domain() is part of linux/device.h hence it should
be included.
I will add linux/device/devres.h as well.
>
> > +#include <linux/dev_printk.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/mailbox_client.h>
> > +#include <linux/mailbox/riscv-rpmi-message.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/types.h>
>
> > +#include <vdso/bits.h>
>
> Just make it linux/bits.h as vdso is for user space libvdso and related.
>
> But
>
> + asm/byteorder.h
>
> ...
>
> > +static void rpmi_sysmsi_irq_mask(struct irq_data *d)
> > +{
> > + struct rpmi_sysmsi_priv *priv = irq_data_get_irq_chip_data(d);
>
> Declare temporary variable for hwirq and do irqd_to_hwirq() only once.
Okay, I will update.
>
> > + int ret;
> > +
> > + ret = rpmi_sysmsi_set_msi_state(priv, irqd_to_hwirq(d), 0);
> > + if (ret) {
> > + dev_warn(priv->dev, "Failed to mask hwirq %lu (error %d)\n",
> > + irqd_to_hwirq(d), ret);
> > + }
> > + irq_chip_mask_parent(d);
> > +}
>
> ...
>
> > +static void rpmi_sysmsi_irq_unmask(struct irq_data *d)
>
> Ditto.
>
> ...
>
> > +static void rpmi_sysmsi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
> > +{
> > + arg->desc = desc;
> > + arg->hwirq = (u32)desc->data.icookie.value;
>
> Hmm... Why do you need an explicit casting?
Not needed, I will drop the casting in the next revision.
>
> > +}
>
> ...
>
> > + if (WARN_ON(fwspec->param_count < 1))
>
> + bug.h
Okay, I will update.
>
> > + return -EINVAL;
>
> + errno.h (but actually you need err.h due to PTR_ERR() et al.)
>
> ...
>
> > +static int rpmi_sysmsi_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct rpmi_sysmsi_priv *priv;
> > + int rc;
>
> Be consistent with variable naming for the same (semantically) stuff.
>
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > + priv->dev = dev;
>
> > + platform_set_drvdata(pdev, priv);
>
> How is being used?
Typically, it's for driver_remove() but since we don't have
driver_remove() we can drop it.
>
> > +
> > + /* Setup mailbox client */
> > + priv->client.dev = priv->dev;
> > + priv->client.rx_callback = NULL;
> > + priv->client.tx_block = false;
> > + priv->client.knows_txdone = true;
> > + priv->client.tx_tout = 0;
> > +
> > + /* Request mailbox channel */
> > + priv->chan = mbox_request_channel(&priv->client, 0);
> > + if (IS_ERR(priv->chan))
> > + return PTR_ERR(priv->chan);
> > +
> > + /* Get number of system MSIs */
> > + rc = rpmi_sysmsi_get_num_msi(priv);
> > + if (rc < 1) {
> > + mbox_free_channel(priv->chan);
> > + if (rc)
> > + return dev_err_probe(dev, rc, "Failed to get number of system MSIs\n");
> > + else
> > + return dev_err_probe(dev, -ENODEV, "No system MSIs found\n");
> > + }
> > + priv->nr_irqs = rc;
> > +
> > + /* Set the device MSI domain if not available */
> > + if (!dev_get_msi_domain(dev)) {
> > + /*
> > + * The device MSI domain for OF devices is only set at the
> > + * time of populating/creating OF device. If the device MSI
> > + * domain is discovered later after the OF device is created
> > + * then we need to set it explicitly before using any platform
> > + * MSI functions.
> > + */
>
> > + if (is_of_node(dev_fwnode(dev)))
> > + of_msi_configure(dev, to_of_node(dev_fwnode(dev)));
>
> if (dev_of_node(dev))
> of_msi_configure(dev, dev_of_node(dev));
Okay, I will update.
>
> > + if (!dev_get_msi_domain(dev)) {
> > + mbox_free_channel(priv->chan);
> > + return -EPROBE_DEFER;
> > + }
> > + }
> > +
> > + if (!msi_create_device_irq_domain(dev, MSI_DEFAULT_DOMAIN,
> > + &rpmi_sysmsi_template,
> > + priv->nr_irqs, priv, priv)) {
> > + mbox_free_channel(priv->chan);
> > + return dev_err_probe(dev, -ENOMEM, "failed to create MSI irq domain\n");
> > + }
> > +
> > + dev_info(dev, "%u system MSIs registered\n", priv->nr_irqs);
> > + return 0;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Regards,
Anup
Powered by blists - more mailing lists