RE: [PATCH v1 3/7] platform/mellanox: mlxreg-hotplug: add extra cycle for hotplug work queue

[Vadim Pasternak]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@xxxxxxxxxxxxx]
> Sent: Friday, April 13, 2018 7:47 PM
> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> Cc: andy.shevchenko@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; jiri@xxxxxxxxxxx;
> Michael Shych <michaelsh@xxxxxxxxxxxx>; ivecera@xxxxxxxxxx
> Subject: Re: [PATCH v1 3/7] platform/mellanox: mlxreg-hotplug: add extra cycle
> for hotplug work queue
> 
> On Tue, Mar 27, 2018 at 10:02:03AM +0000, Vadim Pasternak wrote:
> > It adds missed logic for signal acknowledge, by adding an extra run
> > for work queue in case a signal is received, but no specific signal
> > assertion is detected. Such case theoretically can happen for example
> > in case several units are removed or inserted at the same time. In
> > this situation acknowledge for some signal can be missed at signal top
> > aggreagation status level.
> 
> Why can they be missed? What does "signal top aggregation status level"
> mean? I'm asking to confirm that we are fixing this at the right place, and not
> just applying a suboptimal bandaid by running the workqueue more.
> 

Hi Darren,

Thank for review.

It could happen within the next flow:

The signal routing flow is as following (f.e. for of FANi removing):
 - FAN status and event registers related bit is changed;
 -- intermediate aggregation status register is changed;
 --- top aggregation status register is changed;
 ---- interrupt routed to CPU and interrupt handler is invoked.

When interrupt handler is invoked it follows the next simple logic (f.e
FAN3 is removed):
 (a1) mask top aggregation interrupt mask register;
 (a2) read top aggregation interrupt status register and test to which
      underling group belongs a signal (FANs in this case and is changed
	  from 0xff to 0xfb and 0xfb is saved as a last status value);
   (b1) mask FANs interrupt mask register;
   (b2) read FANs status register and test which FAN has been changed (FAN3 in
        this example);
     (c1) perform relevant action;
              <--------------- (FAN2 is removed at this point)
   (b3) clear FANs interrupt event register to acknowledge FAN3 signal;
   (b4) unmask FANs interrupt mask register
 (a3) unmask top aggregation interrupt mask register;
 
 An interrupt handler is invoked, since FAN2 interrupt is not acknowledge.
 It should set top aggregation interrupt status register bit 6 (0xfb).
 In step (a2)
 (a2) read top aggregation interrupt and comparing it with saved value doesn't
      show change (same 0xfb) and after (a2) execution jumps to (a3) and
	  signal leaved unhandled.

> ...
> 
> >
> > Fixes: 1f976f6978bf ("platform/x86: Move Mellanox platform hotplug
> > driver to platform/mellanox")
> 
> You didn't mention above how this commit caused this - how did moving the
> driver create this problem? 

Actually I should reference to 
07b89c2b2a5e ("platform/x86: Introduce support for Mellanox hotplug driver")
which was initial driver commit, before it has been relocated. 

Does this need to go to stable? I'm assuming not as
> you've called it theoretical - not something you've observed in practice?
> 

It's not necessary to go to stable.

> ...
> 
> >  static int mlxreg_hotplug_device_create(struct
> > mlxreg_hotplug_priv_data *priv, @@ -410,6 +413,18 @@ static void
> mlxreg_hotplug_work_handler(struct work_struct *work)
> >  	aggr_asserted = priv->aggr_cache ^ regval;
> >  	priv->aggr_cache = regval;
> >
> > +	/*
> > +	 * Handler is invoked, but no assertion is detected at top aggregation
> > +	 * status level. Set aggr_asserted to mask value to allow handler extra
> > +	 * run over all relevant signals to recover any missed signal.
> > +	 */
> > +	if (priv->not_asserted == MLXREG_HOTPLUG_NOT_ASSERT) {
> > +		priv->not_asserted = 0;
> > +		aggr_asserted = pdata->mask;
> > +	}
> > +	if (!aggr_asserted)
> 
> We seem to check aggr_asserted in several places in this routine now.
> Looks like something we could simplify. If you check it here, can you drop the
> check lower in the routine? Can you remove it from the for loop if conditional
> entirely? Please consider how to simplify.

OK, will review this code.

> 
> --
> Darren Hart
> VMware Open Source Technology Center