Re: [PATCH v3 13/23] irqchip: Add driver for the RPMI system MSI service group

Anup Patel
2025-05-21T11:37:20Z

On Mon, May 12, 2025 at 12:20 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Sun, May 11, 2025 at 07:09:29PM +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.
>
> ...
>
> > +/*
> > + * Copyright (C) 2025 Ventana Micro Systems Inc.
> > + */
>
> It can occupy a single line instead of 3 LoCs.

Okay, I will update.

>
> ...
>
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/cpu.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irqchip.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/printk.h>
> > +#include <linux/smp.h>
>
> + types.h
>
> Actually this one is most clean, the rest of the patches where the new code
> is introduced has semi-random list of the inclusions, please, follow the IWYU
> principle.

Sure, I will simplify the #includes

>
> ...
>
> > +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_set_msi_state(priv, d->hwirq, 0);
>
> Please, use the respective getter and the type:

Okay

>
> irq_hw_number_t hwirq = irqd_to_hwirq(d);
>
> Ditto for all other similar cases.
>
> > + if (ret) {
> > + dev_warn(priv->dev, "Failed to mask hwirq %d (error %d)\n",
> > + (u32)d->hwirq, ret);
>
> No, this is wrong in two ways: usage of specified for signed value and
> passing the unsigned; using explicit casting to something unsigned.
> Instead ofa the explicit casting, find the best formatting specifier
> and use it.
>
> Ditto for all your code.

Okay, I will update.

>
> > + }
> > + irq_chip_mask_parent(d);
> > +}
>
> ...
>
> > +static int rpmi_sysmsi_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct rpmi_sysmsi_priv *priv;
> > + int rc;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > + priv->dev = dev;
> > + platform_set_drvdata(pdev, priv);
> > +
> > + /* 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);
> > + return dev_err_probe(dev, -ENODEV, "No system MSIs found\n");
>
> Can rc be negative holding an error code? If so, why does the code shadow that?

Ahh yes, we should print a different error message when rc < 0. I will update.

>
> > + }
> > + 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_get_msi_domain(dev))
> > + return -EPROBE_DEFER;
> > + }
> > +
> > + if (!msi_create_device_irq_domain(dev, MSI_DEFAULT_DOMAIN,
> > + &rpmi_sysmsi_template,
> > + priv->nr_irqs, priv, priv))
> > + return dev_err_probe(dev, -ENOMEM, "failed to create MSI irq domain\n");
> > +
> > + dev_info(dev, "%d system MSIs registered\n", priv->nr_irqs);
> > + return 0;
> > +}
>
> ...
>
> > +/** RPMI system MSI service IDs */
>
> Why does this have a kernel-doc marker?

Okay, I will use "/* */".

>
> > +enum rpmi_sysmsi_service_id {
> > + RPMI_SYSMSI_SRV_ENABLE_NOTIFICATION = 0x01,
> > + RPMI_SYSMSI_SRV_GET_ATTRIBUTES = 0x2,
> > + RPMI_SYSMSI_SRV_GET_MSI_ATTRIBUTES = 0x3,
> > + RPMI_SYSMSI_SRV_SET_MSI_STATE = 0x4,
> > + RPMI_SYSMSI_SRV_GET_MSI_STATE = 0x5,
> > + RPMI_SYSMSI_SRV_SET_MSI_TARGET = 0x6,
> > + RPMI_SYSMSI_SRV_GET_MSI_TARGET = 0x7,
>
> Please, be consistent in the style of values.

Okay, I will update.

>
> > + RPMI_SYSMSI_SRV_ID_MAX_COUNT,
>
> No comma in the terminator entry.

Okay.

>
> > +};
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Regards,
Anup