| 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: <aCGaKXOOWyM4JQMg@smile.fi.intel.com>
Date: Mon, 12 May 2025 09:50:17 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Anup Patel <apatel@...tanamicro.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 v3 13/23] irqchip: Add driver for the RPMI system MSI
service group
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.
...
> +#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.
...
> +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:
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.
> + }
> + 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?
> + }
> + 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?
> +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.
> + RPMI_SYSMSI_SRV_ID_MAX_COUNT,
No comma in the terminator entry.
> +};
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists