linux-kernel - Re: [PATCH v3 1/1] soc: fujitsu: Add A64FX diagnostic interrupt driver

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>] [day] [month] [year] [list]

Message-ID: <OSZPR01MB7050ED8D47839EAC9B4FC5BFEBE49@OSZPR01MB7050.jpnprd01.prod.outlook.com>
Date:   Fri, 8 Apr 2022 10:32:15 +0000
From:   "hasegawa-hitomi@...itsu.com" <hasegawa-hitomi@...itsu.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "soc@...nel.org" <soc@...nel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "sumit.garg@...aro.org" <sumit.garg@...aro.org>,
        "arnd@...db.de" <arnd@...db.de>, "olof@...om.net" <olof@...om.net>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will@...nel.org" <will@...nel.org>,
        "jirislaby@...nel.org" <jirislaby@...nel.org>,
        "jason.wessel@...driver.com" <jason.wessel@...driver.com>,
        "daniel.thompson@...aro.org" <daniel.thompson@...aro.org>,
        "dianders@...omium.org" <dianders@...omium.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kgdb-bugreport@...ts.sourceforge.net" 
        <kgdb-bugreport@...ts.sourceforge.net>,
        "peterz@...radead.org" <peterz@...radead.org>
Subject: Re: [PATCH v3 1/1] soc: fujitsu: Add A64FX diagnostic interrupt
 driver

Hi Greg,

> > Enable diagnostic interrupts for the A64FX.
> > This is done using a pseudo-NMI.
> 
> I do not understand what this driver is, sorry.  Can you please provide more
> information in the changelog text for what this is, who would use it, and how it will
> be interacted with.

I understand. I will add a description in the next version.

> > +config A64FX_DIAG
> > +	bool "A64FX diag driver"
> > +	depends on ARM64
> 
> What about ACPI?  Shouldn't this code depend on that?

Okey. I will make it dependent on ACPI.

> > +	help
> > +	  Say Y here if you want to enable diag interrupt on A64FX.
> 
> What is A64FX?

A64FX is a processor designed by Fujitsu.
For the sake of clarity, I will describe it as "Fujitsu A64FX".

> > +	  This driver uses pseudo-NMI if available.
> 
> What does this mean?

This driver uses the pseudo-NMI feature to perform diagnostic interrupts
for A64FX. However, I felt that it was superfluous to give this explanation
here, so I will delete this sentence.

> > +	  If unsure, say N.
> 
> No module?  Why not?

request_nmi() is not EXPORT_SYMBOL. So this driver cannot be a module.

> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * A64FX diag driver.
> 
> No copyright information?  Are you sure?

I will add copyright information.

> > +#define BMC_DIAG_INTERRUPT_STATUS_OFFSET (0x0044) #define
> > +BMC_INTERRUPT_STATUS_MASK ((1U) << 31)
> 
> BIT()?
> 
> > +#define BMC_DIAG_INTERRUPT_ENABLE_OFFSET (0x0040) #define
> > +BMC_INTERRUPT_ENABLE_MASK ((1U) << 31)
> 
> BIT()?

Okey, I will use BIT().

> > +static irqreturn_t a64fx_diag_handler(int irq, void *dev_id) {
> > +	handle_sysrq('c');
> 
> 
> Why is this calling this sysrq call?  From an interrupt?  Why?
> 
> And you are hard-coding "c", are you sure?

As mentioned by Arnd, I only called panic () at first, but after receiving
his suggestion, I decided to call handle_sysrq ('c').
This driver only supports the function that causes a panic when receiving
a diagnostic interrupt from the outside, so "c" is coded.
Also, no data is added when a diagnostic interrupt is sent.

> > +	if (mmsc & BMC_INTERRUPT_STATUS_MASK)
> > +		writel(BMC_INTERRUPT_STATUS_MASK, (void
> *)diag_status_reg_addr);
> 
> No need to wait for the write to complete?
> 
> You shouldn't have to cast diag_status_reg_addr, right?

Due to the specifications of the machine, there is no problem even
if there is no write wait processing.

I will remove constant and (void *). Thank you for pointing out.

> > +		enable_irq(priv->irq);
> > +		priv->has_nmi = false;
> > +		dev_info(dev, "registered for IRQ %d\n", priv->irq);
> > +	} else {
> > +		enable_nmi(priv->irq);
> > +		priv->has_nmi = true;
> > +		dev_info(dev, "registered for NMI %d\n", priv->irq);
> 
> When drivers are successful, they are quiet.  No need for the noise here.

I will remove the message for a successful NMI/IRQ registration.

Thank you.
Hitomi Hasegawa