linux-kernel - RE: [PATCH platform-next 2/2] platform/mellanox: mlxreg-hotplug: Add support for handling interrupt storm

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:
 <SN7PR12MB73247244E8D925D264EABA7AD611A@SN7PR12MB7324.namprd12.prod.outlook.com>
Date: Fri, 19 Sep 2025 12:46:53 +0000
From: Ciju Rajan K <crajank@...dia.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, Hans de
 Goede <hdegoede@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, LKML
	<linux-kernel@...r.kernel.org>, Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>
CC: "christophe.jaillet@...adoo.fr" <christophe.jaillet@...adoo.fr>,
	"platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>,
	Vadim Pasternak <vadimp@...dia.com>
Subject: RE: [PATCH platform-next 2/2] platform/mellanox: mlxreg-hotplug: Add
 support for handling interrupt storm

Hi Ilpo,

Thank you very much for the review.

> > +
> > +             /* Interrupt storm handling logic. */
> > +             if (data->wmark_low_cntr == 0)
> > +                     data->wmark_low_ts = jiffies;
> > +
> > +             if (data->wmark_low_cntr == MLXREG_HOTPLUG_WM_COUNTER -
> 1) {
> > +                     data->wmark_high_ts = jiffies;
> 
> Why does this timestamp have to be saved?

Good point, I will remove it in the next version.

> 
> > +                     wmark_low_ts_window = data->wmark_low_ts +
> > +
> msecs_to_jiffies(MLXREG_HOTPLUG_WM_WINDOW);
> 
> Why not just calculate the ending of the window right at the beginning?
> I'd call the member e.g. ->wmark_window (or ->wmark_window_end).

ACK. Will introduce a new variable in the next version.

> 
> > +                     if (time_after(data->wmark_high_ts, wmark_low_ts_window)) {
> > +                             dev_err(priv->dev, "Storming bit %d (label: %s) - interrupt
> masked permanently. Replace broken HW.",
> > +                                     bit, data->label);
> > +                             /* Mark bit as storming. */
> > +                             item->storming_bits |= BIT(bit);
> 
> Why not using continue here for consistency with the other skip above?
> 
> If you add the continue, you can remove the else and deindent the zero
> assignment by one level:

Yes. Agree. Will change it in the next version.

Thanks
Ciju