Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933973Ab3CVQVc (ORCPT ); Fri, 22 Mar 2013 12:21:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51871 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933862Ab3CVQVb (ORCPT ); Fri, 22 Mar 2013 12:21:31 -0400 Message-ID: <1363969243.24132.613.camel@bling.home> Subject: Re: [PATCH] pciehp: Add pciehp_surprise module option From: Alex Williamson To: Michal Marek Cc: Matthew Garrett , Takashi Iwai , Bjorn Helgaas , Oliver Neukum , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Date: Fri, 22 Mar 2013 10:20:43 -0600 In-Reply-To: <514C8388.3050200@suse.cz> References: <1363802953.24132.513.camel@bling.home> <514C8388.3050200@suse.cz> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5160 Lines: 104 On Fri, 2013-03-22 at 17:15 +0100, Michal Marek wrote: > On 20.3.2013 20:08, Takashi Iwai wrote: > > At Wed, 20 Mar 2013 12:09:13 -0600, > > Alex Williamson wrote: > >> > >> On Wed, 2013-03-20 at 15:02 +0100, Takashi Iwai wrote: > >>> We encountered a problem that on some HP machines the Realtek PCI-e > >>> card reader device appears only when you inserted a card before the > >>> cold boot. While debugging, it turned out that the device is actually > >>> handled via PCI-e hotplug in some level. The device sends a presence > >>> change notification, and pciehp receives it, but it's ignored because > >>> of lack of the hotplug surprise (PCI_EXP_SLTCAP_HPS) capability bit. > >>> Once when this check passes, everything starts working -- the device > >>> appears upon plugging the card properly. > >>> > >>> There are a few other bug reports indicating the similar problems > >>> (e.g. on recent Dell laptops), and I guess the culprit is same. > >>> > >>> This patch adds a new module option, pciehp_surprise, to pciehp as a > >>> workaround. When pciehp_surprise=1 is given, pciehp handles the > >>> presence change as the device on/off as if PCI_EXP_SLTCAP_HPS is set. > >>> Unless it's set explicitly, there is no impact on the existing > >>> behavior. > >>> > >>> Signed-off-by: Takashi Iwai > >>> --- > >>> drivers/pci/hotplug/pciehp.h | 3 ++- > >>> drivers/pci/hotplug/pciehp_core.c | 3 +++ > >>> 2 files changed, 5 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h > >>> index 2c113de..314f3be 100644 > >>> --- a/drivers/pci/hotplug/pciehp.h > >>> +++ b/drivers/pci/hotplug/pciehp.h > >>> @@ -44,6 +44,7 @@ extern bool pciehp_poll_mode; > >>> extern int pciehp_poll_time; > >>> extern bool pciehp_debug; > >>> extern bool pciehp_force; > >>> +extern bool pciehp_surprise; > >>> > >>> #define dbg(format, arg...) \ > >>> do { \ > >>> @@ -122,7 +123,7 @@ struct controller { > >>> #define MRL_SENS(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_MRLSP) > >>> #define ATTN_LED(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_AIP) > >>> #define PWR_LED(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_PIP) > >>> -#define HP_SUPR_RM(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPS) > >>> +#define HP_SUPR_RM(ctrl) (pciehp_surprise || ((ctrl)->slot_cap & PCI_EXP_SLTCAP_HPS)) > >>> #define EMI(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_EIP) > >>> #define NO_CMD_CMPL(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS) > >>> #define PSN(ctrl) ((ctrl)->slot_cap >> 19) > >>> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c > >>> index 7d72c5e..c3a574e 100644 > >>> --- a/drivers/pci/hotplug/pciehp_core.c > >>> +++ b/drivers/pci/hotplug/pciehp_core.c > >>> @@ -42,6 +42,7 @@ bool pciehp_debug; > >>> bool pciehp_poll_mode; > >>> int pciehp_poll_time; > >>> bool pciehp_force; > >>> +bool pciehp_surprise; > >>> > >>> #define DRIVER_VERSION "0.4" > >>> #define DRIVER_AUTHOR "Dan Zink , Greg Kroah-Hartman , Dely Sy " > >>> @@ -55,10 +56,12 @@ module_param(pciehp_debug, bool, 0644); > >>> module_param(pciehp_poll_mode, bool, 0644); > >>> module_param(pciehp_poll_time, int, 0644); > >>> module_param(pciehp_force, bool, 0644); > >>> +module_param(pciehp_surprise, bool, 0644); > >>> MODULE_PARM_DESC(pciehp_debug, "Debugging mode enabled or not"); > >>> MODULE_PARM_DESC(pciehp_poll_mode, "Using polling mechanism for hot-plug events or not"); > >>> MODULE_PARM_DESC(pciehp_poll_time, "Polling mechanism frequency, in seconds"); > >>> MODULE_PARM_DESC(pciehp_force, "Force pciehp, even if OSHP is missing"); > >>> +MODULE_PARM_DESC(pciehp_surprise, "Force to set hotplug-surprise capability"); > >>> > >>> #define PCIE_MODULE_NAME "pciehp" > >>> > >> > >> Please no. Can we quirk just the device with the problem? > > > > It'd be good to have a static quirk in the end, yes. > > Alex, Matthew, would it work for you to have this debug / band-aid > option _and_ have a list of either machines' dmi strings that have this > problem, or devices' PCI IDs (*), and enable the surprise event handling > for such machines / devices automatically? The option would be still > useful for debugging, to be able to easily find out if given machine has > this problem and needs to be added to the quirk list. > > (*) It would probably make more sense to have a list of dmi strings, > because it's the PCIe controller that does not set the capability bit > when it should. The device (Realtek card reader in this case) seems to > behave correctly. The machines in question have been laptops or > all-in-one PCs so far, so there should be no problem with dmi matches. Yep, I think that makes sense, leave the global option for debugging but fix individual known broken devices through dmi quirks. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/