| Message ID | 1525675735-125547-1-git-send-email-vadimp@mellanox.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Mon, May 07, 2018 at 06:48:51AM +0000, Vadim Pasternak wrote: Hi Vadim, > Add extra cycle for hotplug work queue to handle the case when a signal is > It adds missed logic for signal acknowledge, by adding an extra run for > 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. The extra run will > allow to handler to acknowledge the missed signal. > ... > > Fixes: 07b89c2b2a5e ("platform/x86: Introduce support for Mellanox hotplug driver") This sha doesn't exist in mainline, I believe you are referring to: $ git l | grep "Introduce support for Mellanox hotplug" 304887041d95 platform/x86: Introduce support for Mellanox hotplug driver While this was introduced in 4.10. There have been many changes since then, with significant changes in the 4.17 cycle. I have not added the stable tag, please let me know if you think this should be pushed to stable, and which of the intervening patches you'd consider dependencies. I've fixed the sha in the commit message, and queued for testing. Thanks,
> -----Original Message----- > From: Darren Hart [mailto:dvhart@infradead.org] > Sent: Friday, May 25, 2018 2:52 AM > To: Vadim Pasternak <vadimp@mellanox.com> > Cc: andy.shevchenko@gmail.com; gregkh@linuxfoundation.org; linux- > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; jiri@resnulli.us; > Michael Shych <michaelsh@mellanox.com>; ivecera@redhat.com > Subject: Re: [PATCH v2 3/7] platform/mellanox: mlxreg-hotplug: add extra cycle > for hotplug work queue > > On Mon, May 07, 2018 at 06:48:51AM +0000, Vadim Pasternak wrote: > > Hi Vadim, > > > Add extra cycle for hotplug work queue to handle the case when a > > signal is It adds missed logic for signal acknowledge, by adding an > > extra run for 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. The extra run will allow to handler to acknowledge the missed > signal. > > > > ... > > > > > Fixes: 07b89c2b2a5e ("platform/x86: Introduce support for Mellanox > > hotplug driver") > > This sha doesn't exist in mainline, I believe you are referring to: > > $ git l | grep "Introduce support for Mellanox hotplug" > 304887041d95 platform/x86: Introduce support for Mellanox hotplug driver > > While this was introduced in 4.10. There have been many changes since then, > with significant changes in the 4.17 cycle. I have not added the stable tag, please > let me know if you think this should be pushed to stable, and which of the > intervening patches you'd consider dependencies. Hi Darren, Thank you for review. I think it's worth pushing to stable. It doesn't depend one previous patches from the patchset, but possibly this one, which just fixes few comments in mlxreg-hotplug: platform/mellanox: mlxreg-hotplug: Document fixes for hotplug private data (commit 321089a4da2f6b20bb8af96354f76d260a4ca2c6 in testing branch). Thanks, Vadim. > > I've fixed the sha in the commit message, and queued for testing. > > Thanks, > > -- > Darren Hart > VMware Open Source Technology Center
diff --git a/drivers/platform/mellanox/mlxreg-hotplug.c b/drivers/platform/mellanox/mlxreg-hotplug.c index b56953a..922913b 100644 --- a/drivers/platform/mellanox/mlxreg-hotplug.c +++ b/drivers/platform/mellanox/mlxreg-hotplug.c @@ -55,6 +55,7 @@ #define MLXREG_HOTPLUG_RST_CNTR 3 #define MLXREG_HOTPLUG_ATTRS_MAX 24 +#define MLXREG_HOTPLUG_NOT_ASSERT 3 /** * struct mlxreg_hotplug_priv_data - platform private data: @@ -74,6 +75,7 @@ * @mask: top aggregation interrupt common mask; * @aggr_cache: last value of aggregation register status; * @after_probe: flag indication probing completion; + * @not_asserted: number of entries in workqueue with no signal assertion; */ struct mlxreg_hotplug_priv_data { int irq; @@ -93,6 +95,7 @@ struct mlxreg_hotplug_priv_data { u32 mask; u32 aggr_cache; bool after_probe; + u8 not_asserted; }; 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) + goto unmask_event; + /* Handle topology and health configuration changes. */ for (i = 0; i < pdata->counter; i++, item++) { if (aggr_asserted & item->aggr_mask) { @@ -420,27 +435,26 @@ static void mlxreg_hotplug_work_handler(struct work_struct *work) } } - if (aggr_asserted) { - spin_lock_irqsave(&priv->lock, flags); + spin_lock_irqsave(&priv->lock, flags); - /* - * It is possible, that some signals have been inserted, while - * interrupt has been masked by mlxreg_hotplug_work_handler. - * In this case such signals will be missed. In order to handle - * these signals delayed work is canceled and work task - * re-scheduled for immediate execution. It allows to handle - * missed signals, if any. In other case work handler just - * validates that no new signals have been received during - * masking. - */ - cancel_delayed_work(&priv->dwork_irq); - schedule_delayed_work(&priv->dwork_irq, 0); + /* + * It is possible, that some signals have been inserted, while + * interrupt has been masked by mlxreg_hotplug_work_handler. In this + * case such signals will be missed. In order to handle these signals + * delayed work is canceled and work task re-scheduled for immediate + * execution. It allows to handle missed signals, if any. In other case + * work handler just validates that no new signals have been received + * during masking. + */ + cancel_delayed_work(&priv->dwork_irq); + schedule_delayed_work(&priv->dwork_irq, 0); - spin_unlock_irqrestore(&priv->lock, flags); + spin_unlock_irqrestore(&priv->lock, flags); - return; - } + return; +unmask_event: + priv->not_asserted++; /* Unmask aggregation event (no need acknowledge). */ ret = regmap_write(priv->regmap, pdata->cell + MLXREG_HOTPLUG_AGGR_MASK_OFF, pdata->mask);
Add extra cycle for hotplug work queue to handle the case when a signal is It adds missed logic for signal acknowledge, by adding an extra run for 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. The extra run will allow to handler to acknowledge the missed signal. The interrupt handling flow performs the next steps: (1) Enter mlxreg_hotplug_work_handler due to signal assertion. Aggregation status register is changed for example from 0xff to 0xfd (event signal group related to bit 1). (2) Mask aggregation interrupts, read aggregation status register and save it (0xfd) in aggr_cache, then traverse down to handle signal from groups related to the changed bit. (3) Read and mask group related signal. Acknowledge and unmask group related signal (acknowledge should clear aggregation status register from 0xfd back to 0xff). (4) Re-schedule work queue for the immediate execution. (5) Enter mlxreg_hotplug_work_handler due to re-scheduling. Aggregation status is changed from previous 0xfd to 0xff. Go over steps (2) - (5) and in case no new signal assertion is detected - unmask aggregation interrupts. The possible race could happen in case new signal from the same group is asserted after step (3) and prior step (5). In such case aggregation status will change back from 0xff to 0xfd and the value read from the aggregation status register will be the same as a value saved in aggr_cache. As a result the handler will not traverse down and signal will stay unhandled. Example of faulty 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 does not show change (same 0xfb) and after (a2) execution jumps to (a3) and signal leaved unhandled The fix will enforce handler to traverse down in case the signal is received, but signal assertion is not detected. Fixes: 07b89c2b2a5e ("platform/x86: Introduce support for Mellanox hotplug driver") Signed-off-by: Vadim Pasternak <vadimp@mellanox.com> --- V1->v2: Comments pointed out by Darren: - Modify commit message; - Remove redundant check of aggr_asserted; - Change bug fix reference from 1f976f6978bf, which just relocated driver to 07b89c2b2a5e, which was initial driver commit. --- drivers/platform/mellanox/mlxreg-hotplug.c | 48 +++++++++++++++++++----------- 1 file changed, 31 insertions(+), 17 deletions(-)