[PATCH] pciehp: Add pciehp_surprise module option

[PATCH] pciehp: Add pciehp_surprise module option

[Takashi Iwai]

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 <tiwai@suse.de>
---
 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 <dan.zink@compaq.com>, Greg Kroah-Hartman <greg@kroah.com>, Dely Sy <dely.l.sy@intel.com>"
@@ -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"
 
-- 
1.8.2

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Randy Dunlap]

On 03/20/13 07:02, 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 <tiwai@suse.de>
> ---
>  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_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
> @@ -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");


It appears that all of the pciehp options need to be documented,
preferably in Documentation/kernel-parameters.txt .


-- 
~Randy

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Takashi Iwai]

At Wed, 20 Mar 2013 09:33:18 -0700,
Randy Dunlap wrote:
> 
> On 03/20/13 07:02, 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 <tiwai@suse.de>
> > ---
> >  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_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
> > @@ -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");
> 
> 
> It appears that all of the pciehp options need to be documented,
> preferably in Documentation/kernel-parameters.txt .

Yeah, makes sense.

Off-topic: it would have been nicer if pciehp_ prefix was removed at
the first place, too.  But it's too late and we need to live with it.


Takashi

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Matthew Garrett]

On Wed, Mar 20, 2013 at 03:02:01PM +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.

Well that just sounds like a bug. What's the downside to just ignoring 
that capability bit?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Takashi Iwai]

At Wed, 20 Mar 2013 17:52:34 +0000,
Matthew Garrett wrote:
> 
> On Wed, Mar 20, 2013 at 03:02:01PM +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.
> 
> Well that just sounds like a bug. What's the downside to just ignoring 
> that capability bit?

I'm afraid that it's too radical to enable always.
Or, what about to check this bit only for disable path?
Enabling the device is usually not to worry much.  The problematic
part is rather the surprising device disablement.

Just my $0.02.


thanks,

Takashi

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Matthew Garrett]

On Wed, Mar 20, 2013 at 08:11:15PM +0100, Takashi Iwai wrote:
> At Wed, 20 Mar 2013 17:52:34 +0000,
> Matthew Garrett wrote:
> > Well that just sounds like a bug. What's the downside to just ignoring 
> > that capability bit?
> 
> I'm afraid that it's too radical to enable always.

Why?

> Or, what about to check this bit only for disable path?

What happens if you unplug the device on one of the affected machines?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Takashi Iwai]

At Wed, 20 Mar 2013 19:12:50 +0000,
Matthew Garrett wrote:
> 
> On Wed, Mar 20, 2013 at 08:11:15PM +0100, Takashi Iwai wrote:
> > At Wed, 20 Mar 2013 17:52:34 +0000,
> > Matthew Garrett wrote:
> > > Well that just sounds like a bug. What's the downside to just ignoring 
> > > that capability bit?
> > 
> > I'm afraid that it's too radical to enable always.
> 
> Why?

Because I'm conservative :)
Well, dunno.  It's just my feeling without deep thought.

> > Or, what about to check this bit only for disable path?
> 
> What happens if you unplug the device on one of the affected machines?

It continues working.  I mean, the PCI device is still there after
unplug, but the Realtek driver unmounts it smoothly.  Re-plugging also
works, too.  So, the handling via pciehp is needed only for the very
first time to register the PCI device.


Takashi

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Matthew Garrett]

On Wed, Mar 20, 2013 at 08:23:39PM +0100, Takashi Iwai wrote:
> At Wed, 20 Mar 2013 19:12:50 +0000,
> Matthew Garrett wrote:
> > On Wed, Mar 20, 2013 at 08:11:15PM +0100, Takashi Iwai wrote:
> > > I'm afraid that it's too radical to enable always.
> > 
> > Why?
> 
> Because I'm conservative :)
> Well, dunno.  It's just my feeling without deep thought.

Adding a parameter means it's broken until people discover that they 
need to pass a parameter to make it work, and many people will just be 
stuck with it not working. It really should be the last resort.

> > > Or, what about to check this bit only for disable path?
> > 
> > What happens if you unplug the device on one of the affected machines?
> 
> It continues working.  I mean, the PCI device is still there after
> unplug, but the Realtek driver unmounts it smoothly.  Re-plugging also
> works, too.  So, the handling via pciehp is needed only for the very
> first time to register the PCI device.

But if I plug in a different device, things will be broken, right?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Alex Williamson]

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 <tiwai@suse.de>
> ---
>  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 <dan.zink@compaq.com>, Greg Kroah-Hartman <greg@kroah.com>, Dely Sy <dely.l.sy@intel.com>"
> @@ -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?  My issue
with turning on surprise hotplug across the system is that secondary bus
resets will trigger a device hot unplug, hot re-plug.  I have a system
with an integrated broadcom NIC behind a root port that claims to
support surprise hotplug (even though this NIC is soldered to the
motherboard) and it's nearly impossible to use it with KVM device
assignment because the struct pci_dev goes away as we're trying to use
it.  This patch is only going to make that problem more widespread.
Thanks,

Alex

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Takashi Iwai]

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 <tiwai@suse.de>
> > ---
> >  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 <dan.zink@compaq.com>, Greg Kroah-Hartman <greg@kroah.com>, Dely Sy <dely.l.sy@intel.com>"
> > @@ -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.

> My issue
> with turning on surprise hotplug across the system is that secondary bus
> resets will trigger a device hot unplug, hot re-plug.  I have a system
> with an integrated broadcom NIC behind a root port that claims to
> support surprise hotplug (even though this NIC is soldered to the
> motherboard) and it's nearly impossible to use it with KVM device
> assignment because the struct pci_dev goes away as we're trying to use
> it.  This patch is only going to make that problem more widespread.

Well, the option is provided obviously for working around a buggy
hardware, or an easy way to investigate the bug.  The problem is that
you can't fix it easily.  The PCI device itself doesn't appear until
it's registered, so you can't run setpci or whatever.  (At the cold
boot, it doesn't exist.)  Hence without such an option, the only way
is to patch the kernel, and it's too inconvenient.

Of course, I can put a big warning when the option is enabled if you
prefer:
   "This option might break KVM enviornment.  If you still use KVM and
    send any bug report, Alex Williamson will curse you."


thanks,

Takashi

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Michal Marek]

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 <tiwai@suse.de>
>>> ---
>>>  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 <dan.zink@compaq.com>, Greg Kroah-Hartman <greg@kroah.com>, Dely Sy <dely.l.sy@intel.com>"
>>> @@ -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.

Thanks,
Michal

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Matthew Garrett]

On Fri, Mar 22, 2013 at 05:15:04PM +0100, Michal Marek wrote:

> 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.

That's be preferable, but if there's anything we've learned about dmi 
strings it's that they usually mean we're doing something wrong. Does 
this work with Windows without any quirking? Is there a machine-specific 
driver for the PCIe chipset?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Michal Marek]

On 22.3.2013 17:16, Matthew Garrett wrote:
> On Fri, Mar 22, 2013 at 05:15:04PM +0100, Michal Marek wrote:
> 
>> 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.
> 
> That's be preferable, but if there's anything we've learned about dmi 
> strings it's that they usually mean we're doing something wrong. Does 
> this work with Windows without any quirking? Is there a machine-specific 
> driver for the PCIe chipset?

I don't know. Oliver and Takashi have the machines, but I'm not sure if
they have a working Windows image for them.

Michal

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Alex Williamson]

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 <tiwai@suse.de>
> >>> ---
> >>>  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 <dan.zink@compaq.com>, Greg Kroah-Hartman <greg@kroah.com>, Dely Sy <dely.l.sy@intel.com>"
> >>> @@ -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

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Oliver Neukum]

On Friday 22 March 2013 10:20:43 Alex Williamson wrote:

> > (*) 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,

I checked with Win8 and it does ignore the surprise attribute.

	Regards
		Oliver

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Michal Marek]

On 27.3.2013 17:11, Oliver Neukum wrote:
> On Friday 22 March 2013 10:20:43 Alex Williamson wrote:
> 
>>> (*) 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,
> 
> I checked with Win8 and it does ignore the surprise attribute.

I.e., the device hotplugs / hotremoves even in a pristine Win8 install
that does not include any driver for the device.

Michal

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Martin Mokrejs]

Hi Takashi,
  would you please describe your test system in more detail? How
about 'lspci -tv'? And 'lsusb -v' of the broken device?

1. For me on Dell Vostro 3550 with a SandyBridge chip doing all SATA+USB2+ExpressCardSlot:

00:00.0 Host bridge: Intel Corporation 2nd Generation Core Processor Family DRAM Controller (rev 09)
00:02.0 VGA compatible controller: Intel Corporation 2nd Generation Core Processor Family Integrated Graphics Controller (rev 09) (prog-if 00 [VGA controller])
00:16.0 Communication controller: Intel Corporation 6 Series/C200 Series Chipset Family MEI Controller #1 (rev 04)
00:1a.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
00:1b.0 Audio device: Intel Corporation 6 Series/C200 Series Chipset Family High Definition Audio Controller (rev 05)
00:1c.0 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 1 (rev b5) (prog-if 00 [Normal decode])
00:1c.1 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 2 (rev b5) (prog-if 00 [Normal decode])
00:1c.3 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 4 (rev b5) (prog-if 00 [Normal decode])
00:1c.4 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 5 (rev b5) (prog-if 00 [Normal decode])
00:1c.7 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 8 (rev b5) (prog-if 00 [Normal decode])
00:1d.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
00:1f.0 ISA bridge: Intel Corporation HM67 Express Chipset Family LPC Controller (rev 05)
00:1f.2 SATA controller: Intel Corporation 6 Series/C200 Series Chipset Family 6 port SATA AHCI Controller (rev 05) (prog-if 01 [AHCI 1.0])
00:1f.3 SMBus: Intel Corporation 6 Series/C200 Series Chipset Family SMBus Controller (rev 05)
05:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168 PCI Express Gigabit Ethernet controller (rev 06)
09:00.0 Network controller: Intel Corporation Centrino Wireless-N 1030 [Rainbow Peak] (rev 34)
0b:00.0 USB controller: Texas Instruments TUSB73x0 SuperSpeed USB 3.0 xHCI Host Controller (rev 02) (prog-if 30 [XHCI])
11:00.0 Mass storage controller: Silicon Image, Inc. SiI 3132 Serial ATA Raid II Controller (rev 01)
#

If I have Realtek MediaCardReader enabled in BIOS, no card in it, coldboot, and hot
insert an ExpressCard into the slot, the Realtek MediaCardReader pops up in dmesg as
a new PCI device. How about you?

My card does NOT show in lspci (maybe because I never plugged in a data card into it) but does show in lsusb:

Bus 002 Device 005: ID 0bda:0138 Realtek Semiconductor Corp. RTS5138 Card Reader Controller
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.00
  bDeviceClass            0 (Defined at Interface level)
  bDeviceSubClass         0 
  bDeviceProtocol         0 
  bMaxPacketSize0        64
  idVendor           0x0bda Realtek Semiconductor Corp.
  idProduct          0x0138 RTS5138 Card Reader Controller
  bcdDevice           38.82
  iManufacturer           1 Generic
  iProduct                2 USB2.0-CRW
  iSerial                 3 20090516388200000


Can you try coldboot without a media card inserted before power up without
your patch and check whether the CardReader pops up after you plugin some
ExpressCard into an ExpressCardSlot (not the CardReader)? I presume it is
a laptop. ;-)

2. Is the hotplug broken also under acpiphp? And again, does it get detected
once you plugin some card into an ExpressCard slot?

3. Does the device appear under lsusb also in addition to lspci?

4. How does the 'lack of the hotplug surprise (PCI_EXP_SLTCAP_HPS) capability bit'
manifest in 'lspci -vvv' output? A diff before and after the patch?

5. Where is the *real* bug in the code that "linux" ignores the fact that one of
the PCIe Root Ports (or the whole PCI Bridge?) does not support 'hotplug surprise'?
Or is this about the hooked up "third-party" PCI devices? Why does it affect
other PCIe ports of the bridge?


Would be nice if you look into any of my previous emails to linux-pci and
with your current knowledge comment whether here or there I faced a same
problem. Looks like. Disabling the hotplug is a no go for me, I need hotplug
for my ExpressCards. So far am rather having disabled the MediaCardReader in
BIOS. But thank you, I did not know that inserting a data card into a CardReader
is supposed to give me a lspci entry for it. So far I saw only the one in lsusb.

Thank you,
Martin


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 <tiwai@suse.de>
> ---
>  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 <dan.zink@compaq.com>, Greg Kroah-Hartman <greg@kroah.com>, Dely Sy <dely.l.sy@intel.com>"
> @@ -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"
>  
> 

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Martin Mokrejs]

Martin Mokrejs wrote:
> Hi Takashi,
>   would you please describe your test system in more detail? How
> about 'lspci -tv'? And 'lsusb -v' of the broken device?
> 
> 1. For me on Dell Vostro 3550 with a SandyBridge chip doing all SATA+USB2+ExpressCardSlot:
> 
> 00:00.0 Host bridge: Intel Corporation 2nd Generation Core Processor Family DRAM Controller (rev 09)
> 00:02.0 VGA compatible controller: Intel Corporation 2nd Generation Core Processor Family Integrated Graphics Controller (rev 09) (prog-if 00 [VGA controller])
> 00:16.0 Communication controller: Intel Corporation 6 Series/C200 Series Chipset Family MEI Controller #1 (rev 04)
> 00:1a.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
> 00:1b.0 Audio device: Intel Corporation 6 Series/C200 Series Chipset Family High Definition Audio Controller (rev 05)
> 00:1c.0 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 1 (rev b5) (prog-if 00 [Normal decode])
> 00:1c.1 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 2 (rev b5) (prog-if 00 [Normal decode])
> 00:1c.3 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 4 (rev b5) (prog-if 00 [Normal decode])
> 00:1c.4 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 5 (rev b5) (prog-if 00 [Normal decode])
> 00:1c.7 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 8 (rev b5) (prog-if 00 [Normal decode])
> 00:1d.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
> 00:1f.0 ISA bridge: Intel Corporation HM67 Express Chipset Family LPC Controller (rev 05)
> 00:1f.2 SATA controller: Intel Corporation 6 Series/C200 Series Chipset Family 6 port SATA AHCI Controller (rev 05) (prog-if 01 [AHCI 1.0])
> 00:1f.3 SMBus: Intel Corporation 6 Series/C200 Series Chipset Family SMBus Controller (rev 05)
> 05:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168 PCI Express Gigabit Ethernet controller (rev 06)
> 09:00.0 Network controller: Intel Corporation Centrino Wireless-N 1030 [Rainbow Peak] (rev 34)
> 0b:00.0 USB controller: Texas Instruments TUSB73x0 SuperSpeed USB 3.0 xHCI Host Controller (rev 02) (prog-if 30 [XHCI])
> 11:00.0 Mass storage controller: Silicon Image, Inc. SiI 3132 Serial ATA Raid II Controller (rev 01)
> #
> 
> If I have Realtek MediaCardReader enabled in BIOS, no card in it, coldboot, and hot
> insert an ExpressCard into the slot, the Realtek MediaCardReader pops up in dmesg as
> a new PCI device. How about you?

Err, not PCI device as I said, sorry, but gets re-detected as a USB device:

[    4.220009] hub 2-1:1.0: port 6, status 0101, change 0000, 12 Mb/s
[    4.291831] usb 2-1.6: new high-speed USB device number 5 using ehci_hcd
[    4.409353] usb 2-1.6: default language 0x0409
[    4.414740] usb 2-1.6: udev 5, busnum 2, minor = 132
[    4.414745] usb 2-1.6: New USB device found, idVendor=0bda, idProduct=0138
[    4.414858] usb 2-1.6: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[    4.414967] usb 2-1.6: Product: USB2.0-CRW
[    4.415069] usb 2-1.6: Manufacturer: Generic
[    4.415172] usb 2-1.6: SerialNumber: 20090516388200000
[    4.416956] usb 2-1.6: usb_probe_device
[    4.416962] usb 2-1.6: configuration #1 chosen from 1 choice
[    4.419477] usb 2-1.6: adding 2-1.6:1.0 (config #1, interface 0)
[    4.424094] usb-storage 2-1.6:1.0: usb_probe_interface
[    4.424103] usb-storage 2-1.6:1.0: usb_probe_interface - got id
[    4.424276] ums-realtek 2-1.6:1.0: usb_probe_interface
[    4.424279] ums-realtek 2-1.6:1.0: usb_probe_interface - got id
[    4.440838] scsi6 : usb-storage 2-1.6:1.0

cut

[  222.748820] pci 0000:11:00.0: [1095:3132] type 00 class 0x018000
[  222.748865] pci 0000:11:00.0: reg 10: [mem 0x00000000-0x0000007f 64bit]
[  222.748898] pci 0000:11:00.0: reg 18: [mem 0x00000000-0x00003fff 64bit]
[  222.748919] pci 0000:11:00.0: reg 20: [io  0x0000-0x007f]
[  222.748960] pci 0000:11:00.0: reg 30: [mem 0x00000000-0x0007ffff pref]
[  222.749095] pci 0000:11:00.0: supports D1 D2
[  222.769438] pci 0000:11:00.0: BAR 6: assigned [mem 0xf0000000-0xf007ffff pref]
[  222.769442] pci 0000:11:00.0: BAR 2: assigned [mem 0xf6c00000-0xf6c03fff 64bit]
[  222.769464] pci 0000:11:00.0: BAR 2: set to [mem 0xf6c00000-0xf6c03fff 64bit] (PCI address [0xf6c00000-0xf6c03fff])
[  222.769466] pci 0000:11:00.0: BAR 0: assigned [mem 0xf6c04000-0xf6c0407f 64bit]
[  222.769487] pci 0000:11:00.0: BAR 0: set to [mem 0xf6c04000-0xf6c0407f 64bit] (PCI address [0xf6c04000-0xf6c0407f])
[  222.769489] pci 0000:11:00.0: BAR 4: assigned [io  0xc000-0xc07f]
[  222.769496] pci 0000:11:00.0: BAR 4: set to [io  0xc000-0xc07f] (PCI address [0xc000-0xc07f])
[  222.891588] sata_sil24 0000:11:00.0: version 1.1
[  222.891606] sata_sil24 0000:11:00.0: enabling device (0100 -> 0103)
[  222.891766] sata_sil24 0000:11:00.0: enabling bus mastering
[  222.894206] scsi7 : sata_sil24
[  222.894813] scsi8 : sata_sil24
[  222.895288] ata7: SATA max UDMA/100 host m128@0xf6c04000 port 0xf6c00000 irq 19
[  222.895291] ata8: SATA max UDMA/100 host m128@0xf6c04000 port 0xf6c02000 irq 19
[  223.337591] sata_sil24 0000:11:00.0: PME# disabled
[  225.323870] ata7: SATA link down (SStatus FFFFFFFF SControl FFFFFFFF)

[  225.326709] sata_sil24: IRQ status == 0xffffffff, PCI fault or device removal?
[  232.184580] pci 0000:11:00.0: [1095:3132] type 00 class 0x018000
[  232.184624] pci 0000:11:00.0: reg 10: [mem 0x00000000-0x0000007f 64bit]
[  232.184655] pci 0000:11:00.0: reg 18: [mem 0x00000000-0x00003fff 64bit]
[  232.184675] pci 0000:11:00.0: reg 20: [io  0x0000-0x007f]
[  232.184714] pci 0000:11:00.0: reg 30: [mem 0x00000000-0x0007ffff pref]
[  232.184828] pci 0000:11:00.0: supports D1 D2
[  232.206000] pci 0000:11:00.0: BAR 6: assigned [mem 0xf0000000-0xf007ffff pref]
[  232.206005] pci 0000:11:00.0: BAR 2: assigned [mem 0xf6c00000-0xf6c03fff 64bit]
[  232.206026] pci 0000:11:00.0: BAR 2: set to [mem 0xf6c00000-0xf6c03fff 64bit] (PCI address [0xf6c00000-0xf6c03fff])
[  232.206028] pci 0000:11:00.0: BAR 0: assigned [mem 0xf6c04000-0xf6c0407f 64bit]
[  232.206048] pci 0000:11:00.0: BAR 0: set to [mem 0xf6c04000-0xf6c0407f 64bit] (PCI address [0xf6c04000-0xf6c0407f])
[  232.206050] pci 0000:11:00.0: BAR 4: assigned [io  0xc000-0xc07f]
[  232.206057] pci 0000:11:00.0: BAR 4: set to [io  0xc000-0xc07f] (PCI address [0xc000-0xc07f])
[  232.207155] sata_sil24 0000:11:00.0: enabling device (0100 -> 0103)
[  232.207369] sata_sil24 0000:11:00.0: enabling bus mastering
[  232.210324] scsi9 : sata_sil24
[  232.211226] scsi10 : sata_sil24
[  232.211830] ata9: SATA max UDMA/100 host m128@0xf6c04000 port 0xf6c00000 irq 19
[  232.211834] ata10: SATA max UDMA/100 host m128@0xf6c04000 port 0xf6c02000 irq 19
[  232.855586] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 1
[  232.855803] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 2
[  232.855904] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 3
[  232.856028] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 4
[  232.856153] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 5
[  232.856278] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 6
[  232.856403] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 7
[  232.856528] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 8
[  232.856652] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 9
[  232.856742] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 10
[  232.856857] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 11
[  232.856981] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 12
[  232.857105] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 13
[  232.857230] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 14
[  232.857355] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 15
[  232.857480] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 16
[  232.857605] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 17
[  232.857729] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 18
[  232.857854] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 19
[  232.857979] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 20
[  232.858104] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 21
[  232.858229] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 22
[  232.858353] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 23
[  232.858478] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 24
[  232.858603] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 25
[  232.858728] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 26
[  232.858853] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 27
[  232.858977] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 28
[  232.859103] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 29
[  232.859227] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 30
[  232.859352] ehci_hcd 0000:00:1d.0: detected XactErr len 0/13 retry 31
[  232.859477] ehci_hcd 0000:00:1d.0: devpath 1.6 ep2in 3strikes
[  232.873452] hub 2-1:1.0: logical disconnect on port 6
[  232.873747] hub 2-1:1.0: state 7 ports 8 chg 0040 evt 0000
[  232.874190] hub 2-1:1.0: port 6, status 0100, change 0001, 12 Mb/s
[  232.874201] usb 2-1.6: USB disconnect, device number 5
[  232.874207] usb 2-1.6: unregistering device
[  232.874213] usb 2-1.6: unregistering interface 2-1.6:1.0
[  232.895911] usb 2-1.6: usb_disable_device nuking all URBs
[  233.052586] hub 2-1:1.0: debounce: port 6: total 100ms stable 100ms status 0x100
[  234.290521] ata9: SATA link down (SStatus 0 SControl 0)
[  279.413768] hub 2-1:1.0: state 7 ports 8 chg 0000 evt 0040
[  279.414305] hub 2-1:1.0: port 6, status 0101, change 0001, 12 Mb/s
[  279.573458] hub 2-1:1.0: debounce: port 6: total 100ms stable 100ms status 0x101
[  279.653215] usb 2-1.6: new high-speed USB device number 8 using ehci_hcd
[  279.850803] usb 2-1.6: default language 0x0409
[  279.856648] usb 2-1.6: udev 8, busnum 2, minor = 135
[  279.856651] usb 2-1.6: New USB device found, idVendor=0bda, idProduct=0138
[  279.856653] usb 2-1.6: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[  279.856654] usb 2-1.6: Product: USB2.0-CRW
[  279.856656] usb 2-1.6: Manufacturer: Generic
[  279.856657] usb 2-1.6: SerialNumber: 20090516388200000
[  279.857242] usb 2-1.6: usb_probe_device
[  279.857244] usb 2-1.6: configuration #1 chosen from 1 choice
[  279.859632] usb 2-1.6: adding 2-1.6:1.0 (config #1, interface 0)
[  279.863384] usb-storage 2-1.6:1.0: usb_probe_interface
[  279.863387] usb-storage 2-1.6:1.0: usb_probe_interface - got id
[  279.863442] ums-realtek 2-1.6:1.0: usb_probe_interface
[  279.863443] ums-realtek 2-1.6:1.0: usb_probe_interface - got id
[  279.879716] scsi11 : usb-storage 2-1.6:1.0


The trigger is the eSATA card, unlike two other (USB3 and Firewire) express cards I have. Probably
the culprit is the SATA thing or a broken sata_sil24 driver.

> 
> My card does NOT show in lspci (maybe because I never plugged in a data card into it) but does show in lsusb:
> 
> Bus 002 Device 005: ID 0bda:0138 Realtek Semiconductor Corp. RTS5138 Card Reader Controller
> Device Descriptor:
>   bLength                18
>   bDescriptorType         1
>   bcdUSB               2.00
>   bDeviceClass            0 (Defined at Interface level)
>   bDeviceSubClass         0 
>   bDeviceProtocol         0 
>   bMaxPacketSize0        64
>   idVendor           0x0bda Realtek Semiconductor Corp.
>   idProduct          0x0138 RTS5138 Card Reader Controller
>   bcdDevice           38.82
>   iManufacturer           1 Generic
>   iProduct                2 USB2.0-CRW
>   iSerial                 3 20090516388200000
> 

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Takashi Iwai]

At Wed, 20 Mar 2013 19:41:38 +0100,
Martin Mokrejs wrote:
> 
> Hi Takashi,
>   would you please describe your test system in more detail? How
> about 'lspci -tv'? And 'lsusb -v' of the broken device?

I left the machine in my office, so I'll give details tomorrow.
It's a Realtek 5249 PCI-e card reader, and this appears as a PCI
device once when registered by pciehp.  At cold boot, it doesn't
appear in lspci.  It appears only when you insert the card.  Also,
this device is no USB.  It's supported by mfd/rtsx_pci driver in 3.9.

> 1. For me on Dell Vostro 3550 with a SandyBridge chip doing all SATA+USB2+ExpressCardSlot:
> 
> 00:00.0 Host bridge: Intel Corporation 2nd Generation Core Processor Family DRAM Controller (rev 09)
> 00:02.0 VGA compatible controller: Intel Corporation 2nd Generation Core Processor Family Integrated Graphics Controller (rev 09) (prog-if 00 [VGA controller])
> 00:16.0 Communication controller: Intel Corporation 6 Series/C200 Series Chipset Family MEI Controller #1 (rev 04)
> 00:1a.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset Family USB Enhanced Host Controller #2 (rev 05) (prog-if 20 [EHCI])
> 00:1b.0 Audio device: Intel Corporation 6 Series/C200 Series Chipset Family High Definition Audio Controller (rev 05)
> 00:1c.0 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 1 (rev b5) (prog-if 00 [Normal decode])
> 00:1c.1 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 2 (rev b5) (prog-if 00 [Normal decode])
> 00:1c.3 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 4 (rev b5) (prog-if 00 [Normal decode])
> 00:1c.4 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 5 (rev b5) (prog-if 00 [Normal decode])
> 00:1c.7 PCI bridge: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 8 (rev b5) (prog-if 00 [Normal decode])
> 00:1d.0 USB controller: Intel Corporation 6 Series/C200 Series Chipset Family USB Enhanced Host Controller #1 (rev 05) (prog-if 20 [EHCI])
> 00:1f.0 ISA bridge: Intel Corporation HM67 Express Chipset Family LPC Controller (rev 05)
> 00:1f.2 SATA controller: Intel Corporation 6 Series/C200 Series Chipset Family 6 port SATA AHCI Controller (rev 05) (prog-if 01 [AHCI 1.0])
> 00:1f.3 SMBus: Intel Corporation 6 Series/C200 Series Chipset Family SMBus Controller (rev 05)
> 05:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168 PCI Express Gigabit Ethernet controller (rev 06)
> 09:00.0 Network controller: Intel Corporation Centrino Wireless-N 1030 [Rainbow Peak] (rev 34)
> 0b:00.0 USB controller: Texas Instruments TUSB73x0 SuperSpeed USB 3.0 xHCI Host Controller (rev 02) (prog-if 30 [XHCI])
> 11:00.0 Mass storage controller: Silicon Image, Inc. SiI 3132 Serial ATA Raid II Controller (rev 01)
> #
> 
> If I have Realtek MediaCardReader enabled in BIOS, no card in it, coldboot, and hot
> insert an ExpressCard into the slot, the Realtek MediaCardReader pops up in dmesg as
> a new PCI device. How about you?

The device is hotplugged only when the option of my patch is enabled,
i.e. overriding the surprise capability check.

> My card does NOT show in lspci (maybe because I never plugged in a data card into it) but does show in lsusb:

So, it's a completely different case...

> 
> Bus 002 Device 005: ID 0bda:0138 Realtek Semiconductor Corp. RTS5138 Card Reader Controller
> Device Descriptor:
>   bLength                18
>   bDescriptorType         1
>   bcdUSB               2.00
>   bDeviceClass            0 (Defined at Interface level)
>   bDeviceSubClass         0 
>   bDeviceProtocol         0 
>   bMaxPacketSize0        64
>   idVendor           0x0bda Realtek Semiconductor Corp.
>   idProduct          0x0138 RTS5138 Card Reader Controller
>   bcdDevice           38.82
>   iManufacturer           1 Generic
>   iProduct                2 USB2.0-CRW
>   iSerial                 3 20090516388200000
> 
> 
> Can you try coldboot without a media card inserted before power up without
> your patch and check whether the CardReader pops up after you plugin some
> ExpressCard into an ExpressCardSlot (not the CardReader)? I presume it is
> a laptop. ;-)

When you boot without the card, there is no PCI device.  Triggering
PCI bus rescan also doesn't expose it.  But, when you insert the card,
you'll get the notification in pciehp (seeing "Card present on Slot"
message), but pciehp doesn't do anything right now unless the
surprising bit is set.  The device may appear if you trigger the PCI
bus rescan at this moment, too, though.

> 2. Is the hotplug broken also under acpiphp? And again, does it get detected
> once you plugin some card into an ExpressCard slot?

acpiphp doesn't load on this machine.

> 3. Does the device appear under lsusb also in addition to lspci?

No.

> 4. How does the 'lack of the hotplug surprise (PCI_EXP_SLTCAP_HPS) capability bit'
> manifest in 'lspci -vvv' output? A diff before and after the patch?

The patch doesn't change the PCI capability itself at all.  It just
skips the check in pciehp driver.

> 5. Where is the *real* bug in the code that "linux" ignores the fact that one of
> the PCIe Root Ports (or the whole PCI Bridge?) does not support 'hotplug surprise'?
> Or is this about the hooked up "third-party" PCI devices? Why does it affect
> other PCIe ports of the bridge?

It's hard to say at this moment.  Can be a bug of BIOS, or can be
something we missed in the PCI discovery or pciehp code...

> Would be nice if you look into any of my previous emails to linux-pci and
> with your current knowledge comment whether here or there I faced a same
> problem. Looks like. Disabling the hotplug is a no go for me, I need hotplug
> for my ExpressCards. So far am rather having disabled the MediaCardReader in
> BIOS. But thank you, I did not know that inserting a data card into a CardReader
> is supposed to give me a lspci entry for it. So far I saw only the one in lsusb.

I'm going to check tomorrow.


Takashi

> 
> Thank you,
> Martin
> 
> 
> 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 <tiwai@suse.de>
> > ---
> >  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 <dan.zink@compaq.com>, Greg Kroah-Hartman <greg@kroah.com>, Dely Sy <dely.l.sy@intel.com>"
> > @@ -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"
> >  
> > 
> 

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Martin Mokrejs]

Takashi Iwai wrote:
> At Wed, 20 Mar 2013 19:41:38 +0100,
> Martin Mokrejs wrote:
>>
>> Hi Takashi,
>>   would you please describe your test system in more detail? How
>> about 'lspci -tv'? And 'lsusb -v' of the broken device?
> 
> I left the machine in my office, so I'll give details tomorrow.
> It's a Realtek 5249 PCI-e card reader, and this appears as a PCI
> device once when registered by pciehp.  At cold boot, it doesn't
> appear in lspci.  It appears only when you insert the card.  Also,
> this device is no USB.  It's supported by mfd/rtsx_pci driver in 3.9.
> 

>> If I have Realtek MediaCardReader enabled in BIOS, no card in it, coldboot, and hot
>> insert an ExpressCard into the slot, the Realtek MediaCardReader pops up in dmesg as
>> a new PCI device. How about you?
> 
> The device is hotplugged only when the option of my patch is enabled,
> i.e. overriding the surprise capability check.
> 
>> My card does NOT show in lspci (maybe because I never plugged in a data card into it) but does show in lsusb:
> 
> So, it's a completely different case...
> 
>>
>> Bus 002 Device 005: ID 0bda:0138 Realtek Semiconductor Corp. RTS5138 Card Reader Controller
>> Device Descriptor:
>>   bLength                18
>>   bDescriptorType         1
>>   bcdUSB               2.00
>>   bDeviceClass            0 (Defined at Interface level)
>>   bDeviceSubClass         0 
>>   bDeviceProtocol         0 
>>   bMaxPacketSize0        64
>>   idVendor           0x0bda Realtek Semiconductor Corp.
>>   idProduct          0x0138 RTS5138 Card Reader Controller
>>   bcdDevice           38.82
>>   iManufacturer           1 Generic
>>   iProduct                2 USB2.0-CRW
>>   iSerial                 3 20090516388200000
>>
>>
>> Can you try coldboot without a media card inserted before power up without
>> your patch and check whether the CardReader pops up after you plugin some
>> ExpressCard into an ExpressCardSlot (not the CardReader)? I presume it is
>> a laptop. ;-)
> 
> When you boot without the card, there is no PCI device.  Triggering
> PCI bus rescan also doesn't expose it.  But, when you insert the card,
> you'll get the notification in pciehp (seeing "Card present on Slot"
> message), but pciehp doesn't do anything right now unless the
> surprising bit is set.  The device may appear if you trigger the PCI
> bus rescan at this moment, too, though.
> 
>> 2. Is the hotplug broken also under acpiphp? And again, does it get detected
>> once you plugin some card into an ExpressCard slot?
> 
> acpiphp doesn't load on this machine.

While we concluded above that I have a different card (USB-hooked Realtek card)
I need since about 3.5 kernel pcie_aspm=off to get acpiphp working. It does not
work for all express cards but maybe this will help you to get *acpiphp* recognize
the slot? Note: the same kernel command line pcie_aspm=off breaks *pciehp* on
my laptop, so don't forget to delete it from grub.conf if you want to stick
with *pciehp* (if your hardware is prone to hit same bug like me:
https://bugzilla.kernel.org/show_bug.cgi?id=59391 .

Just in case you could switch to acpiphp. ;)
Martin

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Bjorn Helgaas]

On Wed, Mar 20, 2013 at 8:02 AM, Takashi Iwai <tiwai@suse.de> 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.

Can you point us at these bug reports, e.g., with URLs?  Hopefully
they will contain complete dmesg logs and "lspci -vv" outputs so we
can debug this a bit more.

I'm strongly opposed to adding a module option to work around this
issue because the user experience is unacceptable.  We can't expect
users to debug the problem and discover the option.

I'm also opposed to a DMI quirk system because I think it's very
likely that this device works correctly under Windows, and I doubt
very much that Windows has to include the equivalent of DMI quirks.
So we should, at least in principle, be able to figure out how to make
it work, too.

> 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 <tiwai@suse.de>
> ---
>  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 <dan.zink@compaq.com>, Greg Kroah-Hartman <greg@kroah.com>, Dely Sy <dely.l.sy@intel.com>"
> @@ -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"
>
> --
> 1.8.2
>

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Takashi Iwai]

[-- Attachment #1: Type: text/plain, Size: 3769 bytes --]

Hi Bjorn,

sorry for the late follow up as I was on vacation and has been busy
for other tasks.  Since this topic went to nirvana, I try to whip
again...

At Mon, 25 Mar 2013 10:58:40 -0600,
Bjorn Helgaas wrote:
> 
> On Wed, Mar 20, 2013 at 8:02 AM, Takashi Iwai <tiwai@suse.de> 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.
> 
> Can you point us at these bug reports, e.g., with URLs?  Hopefully
> they will contain complete dmesg logs and "lspci -vv" outputs so we
> can debug this a bit more.

The machine isn't in market yet, so we cannot expose all things, but I
attach the lspci snippet of the relevant parts, pci-e ports and the
card reader, at least.  If you need anything else, let me know.

As Oliver and Michal already replied, Windows (both 7/8) identifies
the device without modification.  This implies that Windows handles
the hotplug no matter whether the surprise bit is set or not, either
globally or device-specifically.  But, since this is pretty new
hardware, we highly doubt it's done in a white-list basis.

> I'm strongly opposed to adding a module option to work around this
> issue because the user experience is unacceptable.  We can't expect
> users to debug the problem and discover the option.
> 
> I'm also opposed to a DMI quirk system because I think it's very
> likely that this device works correctly under Windows, and I doubt
> very much that Windows has to include the equivalent of DMI quirks.
> So we should, at least in principle, be able to figure out how to make
> it work, too.

In order to get things a bit straight, let me list up the things we
found again:

- The Realtek card reader devices doesn't appear in lspci at the
  fresh boot in multiple kernel versions from 3.0 to 3.9.

- Once when the card is inserted, it issues the hotplug IRQ event.

- pciehp receives and handles the event but it doesn't add/remove the
  device actually because the corresponding controller has no surprise
  bit.

  When forcibly enabling the hotplug device addition by my patch, it
  starts working.  The device is added at card insert.  The removal
  doesn't trigger on our system, but the event itself seems
  generated.

- The surprise bit can't be changed as it's supposed to be read-only
  register bits.  Thus no PCI quirk seems possible, and it has to be
  fixed in pciehp.

- Another way to detect the PCI card reader device is to perform
     echo 1 > /sys/bus/pci/rescan
  with a memory card inserted.  It doesn't work without the card,
  and it is less sophisticated than pciehp, of course.

Right now, we applied a patch for pciehp to ignore the surprise bit
per basis of DMI string match.  This works, but doesn't scale; if the
same problem happens on a similar model, the driver must be compiled
again.  A module option would be really convenient for that, although
I understand your concern, too.

Of course, an alternative (and more radical) solution is to remove the
surprise bit check completely from pciehp, as Matthew suggested in the
thread.  What risk would it bring?


If you have any other solution in mind, please let me know.


thanks,

Takashi

---

[-- Attachment #2: lspci.out --]
[-- Type: application/octet-stream, Size: 15097 bytes --]

00:04.0 PCI bridge: Advanced Micro Devices [AMD] Device 1414 (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
	I/O behind bridge: 00004000-00004fff
	Memory behind bridge: d5000000-d5ffffff
	Prefetchable memory behind bridge: 00000000d0000000-00000000d0ffffff
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [50] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [58] Express (v2) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
			ExtTag+ RBE+ FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
		LnkCap:	Port #1, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <64ns, L1 <1us
			ClockPM- Surprise- LLActRep+ BwNot+
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk-
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt-
		SltCap:	AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise-
			Slot #0, PowerLimit 0.000W; Interlock- NoCompl+
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet+ CmdCplt- HPIrq+ LinkChg-
			Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock-
			Changed: MRL- PresDet- LinkState+
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible-
		RootCap: CRSVisible-
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
		DevCap2: Completion Timeout: Range ABCD, TimeoutDis+ ARIFwd-
		DevCtl2: Completion Timeout: 65ms to 210ms, TimeoutDis- ARIFwd-
		LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis+, Selectable De-emphasis: -6dB
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
			 Compliance De-emphasis: -6dB
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-, EqualizationPhase1-
			 EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
	Capabilities: [a0] MSI: Enable+ Count=1/1 Maskable- 64bit+
		Address: 00000000fee0f00c  Data: 4161
	Capabilities: [b0] Subsystem: Advanced Micro Devices [AMD] Device 1234
	Capabilities: [b8] HyperTransport: MSI Mapping Enable+ Fixed+
	Capabilities: [100 v1] Vendor Specific Information: ID=0001 Rev=1 Len=010 <?>
	Kernel driver in use: pcieport
	Kernel modules: shpchp
00: 22 10 14 14 07 04 10 00 00 00 04 06 10 00 01 00
10: 00 00 00 00 00 00 00 00 00 01 01 00 41 41 00 00
20: 00 d5 f0 d5 01 d0 f1 d0 00 00 00 00 00 00 00 00
30: 00 00 00 00 50 00 00 00 00 00 00 00 ff 01 00 00
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
50: 01 58 03 c8 00 00 00 00 10 a0 42 01 21 80 00 00
60: 10 28 00 00 11 0c 70 01 00 00 11 30 40 00 04 00
70: 28 00 40 01 08 00 01 00 00 00 00 00 1f 00 00 00
80: 06 00 00 00 06 00 00 00 21 00 00 00 00 00 00 00
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
a0: 05 b0 81 00 0c f0 e0 fe 00 00 00 00 61 41 00 00
b0: 0d b8 00 00 22 10 34 12 08 00 03 a8 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 50 00 00 00 02 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

00:05.0 PCI bridge: Advanced Micro Devices [AMD] Device 1415 (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Bus: primary=00, secondary=02, subordinate=02, sec-latency=0
	I/O behind bridge: 00003000-00003fff
	Memory behind bridge: d4000000-d4ffffff
	Prefetchable memory behind bridge: 00000000d1000000-00000000d1ffffff
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [50] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [58] Express (v2) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
			ExtTag+ RBE+ FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
		LnkCap:	Port #2, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <64ns, L1 <1us
			ClockPM- Surprise- LLActRep+ BwNot+
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk-
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt-
		SltCap:	AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise-
			Slot #0, PowerLimit 0.000W; Interlock- NoCompl+
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet+ CmdCplt- HPIrq+ LinkChg-
			Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock-
			Changed: MRL- PresDet- LinkState+
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible-
		RootCap: CRSVisible-
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
		DevCap2: Completion Timeout: Range ABCD, TimeoutDis+ ARIFwd-
		DevCtl2: Completion Timeout: 65ms to 210ms, TimeoutDis- ARIFwd-
		LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis+, Selectable De-emphasis: -6dB
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
			 Compliance De-emphasis: -6dB
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-, EqualizationPhase1-
			 EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
	Capabilities: [a0] MSI: Enable+ Count=1/1 Maskable- 64bit+
		Address: 00000000fee0f00c  Data: 4169
	Capabilities: [b0] Subsystem: Advanced Micro Devices [AMD] Device 1234
	Capabilities: [b8] HyperTransport: MSI Mapping Enable+ Fixed+
	Capabilities: [100 v1] Vendor Specific Information: ID=0001 Rev=1 Len=010 <?>
	Kernel driver in use: pcieport
	Kernel modules: shpchp
00: 22 10 15 14 07 04 10 00 00 00 04 06 10 00 01 00
10: 00 00 00 00 00 00 00 00 00 02 02 00 31 31 00 00
20: 00 d4 f0 d4 01 d1 f1 d1 00 00 00 00 00 00 00 00
30: 00 00 00 00 50 00 00 00 00 00 00 00 ff 01 00 00
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
50: 01 58 03 c8 00 00 00 00 10 a0 42 01 21 80 00 00
60: 10 28 00 00 11 0c 70 02 00 00 11 30 40 00 04 00
70: 28 00 40 01 08 00 01 00 00 00 00 00 1f 00 00 00
80: 06 00 00 00 06 00 00 00 21 00 00 00 00 00 00 00
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
a0: 05 b0 81 00 0c f0 e0 fe 00 00 00 00 69 41 00 00
b0: 0d b8 00 00 22 10 34 12 08 00 03 a8 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 50 00 00 00 02 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

00:07.0 PCI bridge: Advanced Micro Devices [AMD] Device 1417 (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Bus: primary=00, secondary=03, subordinate=05, sec-latency=0
	I/O behind bridge: 00002000-00002fff
	Memory behind bridge: d3000000-d3ffffff
	Prefetchable memory behind bridge: 00000000d2000000-00000000d2ffffff
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ <SERR- <PERR-
	BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [50] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [58] Express (v2) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 256 bytes, PhantFunc 0, Latency L0s <64ns, L1 <1us
			ExtTag+ RBE+ FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
		LnkCap:	Port #4, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <64ns, L1 <1us
			ClockPM- Surprise- LLActRep+ BwNot+
		LnkCtl:	ASPM Disabled; RCB 64 bytes Disabled- Retrain- CommClk-
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt-
		SltCap:	AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug+ Surprise-
			Slot #0, PowerLimit 0.000W; Interlock- NoCompl+
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet+ CmdCplt- HPIrq+ LinkChg-
			Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock-
			Changed: MRL- PresDet- LinkState+
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible-
		RootCap: CRSVisible-
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
		DevCap2: Completion Timeout: Range ABCD, TimeoutDis+ ARIFwd-
		DevCtl2: Completion Timeout: 65ms to 210ms, TimeoutDis- ARIFwd-
		LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis+, Selectable De-emphasis: -6dB
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
			 Compliance De-emphasis: -6dB
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-, EqualizationPhase1-
			 EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
	Capabilities: [a0] MSI: Enable+ Count=1/1 Maskable- 64bit+
		Address: 00000000fee0f00c  Data: 4171
	Capabilities: [b0] Subsystem: Advanced Micro Devices [AMD] Device 1234
	Capabilities: [b8] HyperTransport: MSI Mapping Enable+ Fixed+
	Capabilities: [100 v1] Vendor Specific Information: ID=0001 Rev=1 Len=010 <?>
	Kernel driver in use: pcieport
	Kernel modules: shpchp
00: 22 10 17 14 07 04 10 00 00 00 04 06 10 00 01 00
10: 00 00 00 00 00 00 00 00 00 03 05 00 21 21 00 20
20: 00 d3 f0 d3 01 d2 f1 d2 00 00 00 00 00 00 00 00
30: 00 00 00 00 50 00 00 00 00 00 00 00 ff 01 00 00
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
50: 01 58 03 c8 00 00 00 00 10 a0 42 01 21 80 00 00
60: 10 28 00 00 11 0c 70 04 00 00 11 30 40 00 04 00
70: 28 00 40 01 08 00 01 00 00 00 00 00 1f 00 00 00
80: 06 00 00 00 06 00 00 00 21 00 00 00 00 00 00 00
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
a0: 05 b0 81 00 0c f0 e0 fe 00 00 00 00 71 41 00 00
b0: 0d b8 00 00 22 10 34 12 08 00 03 a8 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 50 00 00 00 02 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

02:00.0 Unassigned class [ff00]: Realtek Semiconductor Co., Ltd. Device 5229 (rev 01)
	Subsystem: Hewlett-Packard Company Device 194e
	Physical Slot: 0-1
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 17
	Region 0: Memory at d4000000 (32-bit, non-prefetchable) [size=4K]
	Capabilities: [40] Power Management version 3
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0-,D1+,D2+,D3hot+,D3cold-)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+
		Address: 0000000000000000  Data: 0000
	Capabilities: [70] Express (v2) Endpoint, MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0, Latency L0s unlimited, L1 unlimited
			ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
		LnkCap:	Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 unlimited, L1 <64us
			ClockPM+ Surprise- LLActRep- BwNot-
		LnkCtl:	ASPM L0s L1 Enabled; RCB 64 bytes Disabled- Retrain- CommClk-
			ExtSynch- ClockPM+ AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
		DevCap2: Completion Timeout: Not Supported, TimeoutDis+
		DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-
		LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance- SpeedDis-, Selectable De-emphasis: -6dB
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
			 Compliance De-emphasis: -6dB
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-, EqualizationPhase1-
			 EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest-
	Capabilities: [100 v1] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+ MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr+
		AERCap:	First Error Pointer: 00, GenCap+ CGenEn- ChkCap+ ChkEn-
	Capabilities: [140 v1] Device Serial Number 00-00-00-01-00-4c-e0-00
	Kernel driver in use: rts5229
	Kernel modules: rts5229
00: ec 10 29 52 06 00 10 00 01 00 00 ff 00 00 00 00
10: 00 00 00 d4 00 00 00 00 00 00 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 3c 10 4e 19
30: 00 00 00 00 40 00 00 00 00 00 00 00 ff 01 00 00
40: 01 50 c3 77 00 00 00 00 00 00 00 00 00 00 00 00
50: 05 70 80 00 00 00 00 00 00 00 00 00 00 00 00 00
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
70: 10 00 02 00 c0 8f 00 00 10 20 00 00 11 7c 07 00
80: 03 01 11 10 00 00 00 00 00 00 00 00 00 00 00 00
90: 00 00 00 00 10 00 00 00 00 00 00 00 00 00 00 00
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Bjorn Helgaas]

On Wed, Apr 10, 2013 at 10:34 AM, Takashi Iwai <tiwai@suse.de> wrote:
> Hi Bjorn,
>
> sorry for the late follow up as I was on vacation and has been busy
> for other tasks.  Since this topic went to nirvana, I try to whip
> again...
>
> At Mon, 25 Mar 2013 10:58:40 -0600,
> Bjorn Helgaas wrote:
>>
>> On Wed, Mar 20, 2013 at 8:02 AM, Takashi Iwai <tiwai@suse.de> 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.
>>
>> Can you point us at these bug reports, e.g., with URLs?  Hopefully
>> they will contain complete dmesg logs and "lspci -vv" outputs so we
>> can debug this a bit more.
>
> The machine isn't in market yet, so we cannot expose all things, but I
> attach the lspci snippet of the relevant parts, pci-e ports and the
> card reader, at least.  If you need anything else, let me know.
>
> As Oliver and Michal already replied, Windows (both 7/8) identifies
> the device without modification.  This implies that Windows handles
> the hotplug no matter whether the surprise bit is set or not, either
> globally or device-specifically.  But, since this is pretty new
> hardware, we highly doubt it's done in a white-list basis.
>
>> I'm strongly opposed to adding a module option to work around this
>> issue because the user experience is unacceptable.  We can't expect
>> users to debug the problem and discover the option.
>>
>> I'm also opposed to a DMI quirk system because I think it's very
>> likely that this device works correctly under Windows, and I doubt
>> very much that Windows has to include the equivalent of DMI quirks.
>> So we should, at least in principle, be able to figure out how to make
>> it work, too.
>
> In order to get things a bit straight, let me list up the things we
> found again:
>
> - The Realtek card reader devices doesn't appear in lspci at the
>   fresh boot in multiple kernel versions from 3.0 to 3.9.
>
> - Once when the card is inserted, it issues the hotplug IRQ event.
>
> - pciehp receives and handles the event but it doesn't add/remove the
>   device actually because the corresponding controller has no surprise
>   bit.
>
>   When forcibly enabling the hotplug device addition by my patch, it
>   starts working.  The device is added at card insert.  The removal
>   doesn't trigger on our system, but the event itself seems
>   generated.
>
> - The surprise bit can't be changed as it's supposed to be read-only
>   register bits.  Thus no PCI quirk seems possible, and it has to be
>   fixed in pciehp.
>
> - Another way to detect the PCI card reader device is to perform
>      echo 1 > /sys/bus/pci/rescan
>   with a memory card inserted.  It doesn't work without the card,
>   and it is less sophisticated than pciehp, of course.
>
> Right now, we applied a patch for pciehp to ignore the surprise bit
> per basis of DMI string match.  This works, but doesn't scale; if the
> same problem happens on a similar model, the driver must be compiled
> again.  A module option would be really convenient for that, although
> I understand your concern, too.
>
> Of course, an alternative (and more radical) solution is to remove the
> surprise bit check completely from pciehp, as Matthew suggested in the
> thread.  What risk would it bring?

I think we need to ignore the surprise bit as Matthew suggested.

Alex raised an issue with this (secondary bus reset causes a device
remove followed by device add), so we'd have to work through that
somehow.  I think doing the remove/add would actually be more correct
because we would be doing the whole device initialization after the
reset.  We currently save/restore some device state around the reset,
but that's a piecemeal approach that is certain to miss internal
hidden state.  But I don't know how to deal with the KVM implications
of remove/add.

Bjorn

Re: [PATCH] pciehp: Add pciehp_surprise module option

[Takashi Iwai]

At Wed, 10 Apr 2013 11:19:48 -0600,
Bjorn Helgaas wrote:
> 
> On Wed, Apr 10, 2013 at 10:34 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > Hi Bjorn,
> >
> > sorry for the late follow up as I was on vacation and has been busy
> > for other tasks.  Since this topic went to nirvana, I try to whip
> > again...
> >
> > At Mon, 25 Mar 2013 10:58:40 -0600,
> > Bjorn Helgaas wrote:
> >>
> >> On Wed, Mar 20, 2013 at 8:02 AM, Takashi Iwai <tiwai@suse.de> 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.
> >>
> >> Can you point us at these bug reports, e.g., with URLs?  Hopefully
> >> they will contain complete dmesg logs and "lspci -vv" outputs so we
> >> can debug this a bit more.
> >
> > The machine isn't in market yet, so we cannot expose all things, but I
> > attach the lspci snippet of the relevant parts, pci-e ports and the
> > card reader, at least.  If you need anything else, let me know.
> >
> > As Oliver and Michal already replied, Windows (both 7/8) identifies
> > the device without modification.  This implies that Windows handles
> > the hotplug no matter whether the surprise bit is set or not, either
> > globally or device-specifically.  But, since this is pretty new
> > hardware, we highly doubt it's done in a white-list basis.
> >
> >> I'm strongly opposed to adding a module option to work around this
> >> issue because the user experience is unacceptable.  We can't expect
> >> users to debug the problem and discover the option.
> >>
> >> I'm also opposed to a DMI quirk system because I think it's very
> >> likely that this device works correctly under Windows, and I doubt
> >> very much that Windows has to include the equivalent of DMI quirks.
> >> So we should, at least in principle, be able to figure out how to make
> >> it work, too.
> >
> > In order to get things a bit straight, let me list up the things we
> > found again:
> >
> > - The Realtek card reader devices doesn't appear in lspci at the
> >   fresh boot in multiple kernel versions from 3.0 to 3.9.
> >
> > - Once when the card is inserted, it issues the hotplug IRQ event.
> >
> > - pciehp receives and handles the event but it doesn't add/remove the
> >   device actually because the corresponding controller has no surprise
> >   bit.
> >
> >   When forcibly enabling the hotplug device addition by my patch, it
> >   starts working.  The device is added at card insert.  The removal
> >   doesn't trigger on our system, but the event itself seems
> >   generated.
> >
> > - The surprise bit can't be changed as it's supposed to be read-only
> >   register bits.  Thus no PCI quirk seems possible, and it has to be
> >   fixed in pciehp.
> >
> > - Another way to detect the PCI card reader device is to perform
> >      echo 1 > /sys/bus/pci/rescan
> >   with a memory card inserted.  It doesn't work without the card,
> >   and it is less sophisticated than pciehp, of course.
> >
> > Right now, we applied a patch for pciehp to ignore the surprise bit
> > per basis of DMI string match.  This works, but doesn't scale; if the
> > same problem happens on a similar model, the driver must be compiled
> > again.  A module option would be really convenient for that, although
> > I understand your concern, too.
> >
> > Of course, an alternative (and more radical) solution is to remove the
> > surprise bit check completely from pciehp, as Matthew suggested in the
> > thread.  What risk would it bring?
> 
> I think we need to ignore the surprise bit as Matthew suggested.

OK, this is certainly a cleaner way.

> Alex raised an issue with this (secondary bus reset causes a device
> remove followed by device add), so we'd have to work through that
> somehow.  I think doing the remove/add would actually be more correct
> because we would be doing the whole device initialization after the
> reset.  We currently save/restore some device state around the reset,
> but that's a piecemeal approach that is certain to miss internal
> hidden state.  But I don't know how to deal with the KVM implications
> of remove/add.

So is this specific to KVM?


In anyway, if any test patch is available, let me (or Oliver, Michal)
know.  We'll happily try on the machine showing the bug.


thanks,

Takashi