linux-kernel - Re: [PATCH v4 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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <OSZPR01MB7050C926426AF82B7ADE86B8EBCE9@OSZPR01MB7050.jpnprd01.prod.outlook.com>
Date:   Tue, 17 May 2022 09:53:57 +0000
From:   "hasegawa-hitomi@...itsu.com" <hasegawa-hitomi@...itsu.com>
To:     Jiri Slaby <jirislaby@...nel.org>
CC:     "arnd@...db.de" <arnd@...db.de>, "olof@...om.net" <olof@...om.net>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will@...nel.org" <will@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.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>,
        "sumit.garg@...aro.org" <sumit.garg@...aro.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "soc@...nel.org" <soc@...nel.org>
Subject: Re: [PATCH v4 1/1] soc: fujitsu: Add A64FX diagnostic interrupt
 driver

Hi Jiri,


> I'm not sure why you cc linux-serial, but anyway, comments below :).

I used sysrq until the last version, so I still included kernel-serial in
the destination. I am not planning to use sysrq now, so I will remove it
from the destination from the next version.
Thank you for your comment.


> > +struct a64fx_diag_priv {
> > +	int irq;
> > +	void __iomem *mmsc_reg_base;
> > +	bool has_nmi;
> 
> There are unnecessary holes in the struct. If you reorder it, you drop some
> alignment. Like: pointer, int, bool.

> > +	u32 mmsc;
> > +	void __iomem *diag_status_reg_addr;
> 
> I'm not sure what soc/ maintainers prefer, but inverted xmas tree would look/read
> better.

> > +	priv = devm_kzalloc(dev, sizeof(struct a64fx_diag_priv),
> > +GFP_KERNEL);
> 
> Don't we prefer sizeof(*priv)?

> > +		ret = request_irq(priv->irq, &a64fx_diag_handler_irq,
> > +				irq_flags, "a64fx_diag_irq", NULL);
> > +		if (ret) {
> > +			dev_err(dev, "cannot register IRQ %d\n", ret);
> 
> No a64fx_diag_interrupt_disable()?

> > +		priv->has_nmi = false;
> 
> No need to set zeroed priv member to zero.

I understand. I will fix it as per your comment. Thank you.


> > +		enable_nmi(priv->irq);
> 
> Provided the above, I don't immediatelly see, what's the purpose of
> IRQF_NO_AUTOEN then?

It seems that request_nmi() requires IRQF_NO_AUTOEN.


> > +static int __exit a64fx_diag_remove(struct platform_device *pdev)
> 
> Is __exit appropriate here at all -- I doubt that.

I will remove __exit as it seems unnecessary as you suggested.

Also, I will correct BMC_DIAG_INTERRUPT_STATUS_OFFSET
and BMC_DIAG_INTERRUPT_ENABLE_OFFSET.


Thank you.
Hitomi Hasegawa