Le 03/02/2017 à 07:57, Vaibhav Jain a écrit :
During an eeh event when the cxl card is fenced and card sysfs attr perst_reloads_same_image is set following warning message is seen in the kernel logs: [ 60.622727] Adapter context unlocked with 0 active contexts [ 60.622762] ------------[ cut here ]------------ [ 60.622771] WARNING: CPU: 12 PID: 627 at ../drivers/misc/cxl/main.c:325 cxl_adapter_context_unlock+0x60/0x80 [cxl] Even though this warning is harmless, it clutters the kernel log during an eeh event. This warning is triggered as the EEH callback cxl_pci_error_detected doesn't obtain a context-lock before forcibly detaching all active context and when context-lock is released during call to cxl_configure_adapter from cxl_pci_slot_reset, a warning in cxl_adapter_context_unlock is triggered. To fix this warning and also prevent activation of any context during eeh the patch introduces a new function cxl_adapter_context_force_lock that would forcefully acquire the context_lock and warn if any active contexts exists when this happens. After the EEH flow concludes with call to cxl_pci_resume the context-lock is released with a call to cxl_adapter_context_unlock. Cc: stable@xxxxxxxxxxxxxxx Fixes: 70b565bbdb91("cxl: Prevent adapter reset if an active context exists") Reported-by: Andrew Donnellan <andrew.donnellan@xxxxxxxxxxx> Signed-off-by: Vaibhav Jain <vaibhav@xxxxxxxxxxxxxxxxxx> Reviewed-by: Andrew Donnellan <andrew.donnellan@xxxxxxxxxxx> --- drivers/misc/cxl/cxl.h | 3 +++ drivers/misc/cxl/main.c | 10 ++++++++++ drivers/misc/cxl/pci.c | 18 ++++++++++++++++-- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h index b24d767..34d2ca0 100644 --- a/drivers/misc/cxl/cxl.h +++ b/drivers/misc/cxl/cxl.h @@ -970,4 +970,7 @@ int cxl_adapter_context_lock(struct cxl *adapter); /* Unlock the contexts-lock if taken. Warn and force unlock otherwise */ void cxl_adapter_context_unlock(struct cxl *adapter); +/* Force contexts-lock to be taken */ +void cxl_adapter_context_force_lock(struct cxl *adapter); + #endif diff --git a/drivers/misc/cxl/main.c b/drivers/misc/cxl/main.c index 62e0dfb..1754a0c 100644 --- a/drivers/misc/cxl/main.c +++ b/drivers/misc/cxl/main.c @@ -301,6 +301,16 @@ void cxl_adapter_context_put(struct cxl *adapter) atomic_dec_if_positive(&adapter->contexts_num); } +void cxl_adapter_context_force_lock(struct cxl *adapter) +{ + int count = atomic_read(&adapter->contexts_num); + + if (count > 0) + pr_warn("Forcing context lock with %d active contexts\n", + count); + atomic_set(&adapter->contexts_num, -1); +} +
int cxl_adapter_context_lock(struct cxl *adapter) { int rc; diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c index 80a87ab..d76fd4d 100644 --- a/drivers/misc/cxl/pci.c +++ b/drivers/misc/cxl/pci.c @@ -1487,8 +1487,6 @@ static int cxl_configure_adapter(struct cxl *adapter, struct pci_dev *dev) if ((rc = cxl_native_register_psl_err_irq(adapter))) goto err; - /* Release the context lock as adapter is configured */ - cxl_adapter_context_unlock(adapter); return 0; err: @@ -1587,6 +1585,9 @@ static struct cxl *cxl_pci_init_adapter(struct pci_dev *dev) if ((rc = cxl_sysfs_adapter_add(adapter))) goto err_put1; + /* Release the context lock as adapter is configured */ + cxl_adapter_context_unlock(adapter); + return adapter; err_put1: @@ -1607,6 +1608,9 @@ static void cxl_pci_remove_adapter(struct cxl *adapter) { pr_devel("cxl_remove_adapter\n"); + /* Forcibly take the adapter context lock */ + cxl_adapter_context_force_lock(adapter); + cxl_sysfs_adapter_remove(adapter); cxl_debugfs_adapter_remove(adapter); @@ -1778,6 +1782,9 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev, */ schedule(); + /* forcibly take the context lock to prevent new context activation */ + cxl_adapter_context_force_lock(adapter); +
I'm not a fan of the "force lock" call and I'm not convinced it's needed. When an EEH is triggered, we'll force-detach all the contexts. So that in itself should bring the counter of active contexts to 0. So why not just add a cxl_adapter_context_lock() call at the end of cxl_pci_error_detected()? The lock would be released in cxl_configure_adapter when cxl_pci_slot_reset() is called, as is already the case with the current code. (as a side note, I don't believe the code is racy and that we need to add extra protection to avoid context activation while cxl_pci_error_detected() is called. We can talk about it if needed)
Fred
/* If we're permanently dead, give up. */ if (state == pci_channel_io_perm_failure) { /* Tell the AFU drivers; but we don't care what they @@ -1879,11 +1886,15 @@ static pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev, /* Only continue if everyone agrees on NEED_RESET */ if (result != PCI_ERS_RESULT_NEED_RESET) return result; + } + for (i = 0; i < adapter->slices; i++) { + afu = adapter->afu[i]; cxl_context_detach_all(afu); cxl_ops->afu_deactivate_mode(afu, afu->current_mode); pci_deconfigure_afu(afu); } + cxl_deconfigure_adapter(adapter); return result; @@ -1979,6 +1990,9 @@ static void cxl_pci_resume(struct pci_dev *pdev) afu_dev->driver->err_handler->resume(afu_dev); } } + + /* Unlock context activation for the adapter */ + cxl_adapter_context_unlock(adapter); } static const struct pci_error_handlers cxl_err_handler = {
-- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html