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

[Ciju Rajan K]

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