[PATCH] pciehp: Handle interrupts that happen during initialization.

[PATCH] pciehp: Handle interrupts that happen during initialization.

[Eric W. Biederman]

Move the enabling of interrupts after all of the data structures
are setup so that we can safely run the interrupt handler as
soon as it is registered.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/pci/hotplug/pciehp.h      |    2 ++
 drivers/pci/hotplug/pciehp_core.c |    7 +++++++
 drivers/pci/hotplug/pciehp_hpc.c  |   15 +++++++--------
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index db85284..39ae375 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -111,6 +111,7 @@ struct controller {
 	int cmd_busy;
 	unsigned int no_cmd_complete:1;
 	unsigned int link_active_reporting:1;
+	unsigned int notification_enabled:1;
 };
 
 #define INT_BUTTON_IGNORE		0
@@ -170,6 +171,7 @@ extern int pciehp_configure_device(struct slot *p_slot);
 extern int pciehp_unconfigure_device(struct slot *p_slot);
 extern void pciehp_queue_pushbutton_work(struct work_struct *work);
 struct controller *pcie_init(struct pcie_device *dev);
+int pcie_init_notification(struct controller *ctrl);
 int pciehp_enable_slot(struct slot *p_slot);
 int pciehp_disable_slot(struct slot *p_slot);
 int pcie_enable_notification(struct controller *ctrl);
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index c248554..681e391 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -434,6 +434,13 @@ static int pciehp_probe(struct pcie_device *dev, const struct pcie_port_service_
 		goto err_out_release_ctlr;
 	}
 
+	/* Enable events after we have setup the data structures */
+	rc = pcie_init_notification(ctrl);
+	if (rc) {
+		ctrl_err(ctrl, "Notification initialization failed\n");
+		goto err_out_release_ctlr;
+	}
+
 	/* Check if slot is occupied */
 	t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
 	t_slot->hpc_ops->get_adapter_status(t_slot, &value);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 71a8012..7a16c68 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -934,7 +934,7 @@ static void pcie_disable_notification(struct controller *ctrl)
 		ctrl_warn(ctrl, "Cannot disable software notification\n");
 }
 
-static int pcie_init_notification(struct controller *ctrl)
+int pcie_init_notification(struct controller *ctrl)
 {
 	if (pciehp_request_irq(ctrl))
 		return -1;
@@ -942,13 +942,17 @@ static int pcie_init_notification(struct controller *ctrl)
 		pciehp_free_irq(ctrl);
 		return -1;
 	}
+	ctrl->notification_enabled = 1;
 	return 0;
 }
 
 static void pcie_shutdown_notification(struct controller *ctrl)
 {
-	pcie_disable_notification(ctrl);
-	pciehp_free_irq(ctrl);
+	if (ctrl->notification_enabled) {
+		pcie_disable_notification(ctrl);
+		pciehp_free_irq(ctrl);
+		ctrl->notification_enabled = 0;
+	}
 }
 
 static int pcie_init_slot(struct controller *ctrl)
@@ -1110,13 +1114,8 @@ struct controller *pcie_init(struct pcie_device *dev)
 	if (pcie_init_slot(ctrl))
 		goto abort_ctrl;
 
-	if (pcie_init_notification(ctrl))
-		goto abort_slot;
-
 	return ctrl;
 
-abort_slot:
-	pcie_cleanup_slot(ctrl);
 abort_ctrl:
 	kfree(ctrl);
 abort:
-- 
1.5.6.6

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Kenji Kaneshige]

Hi,

I don't understand what this patch is trying to fix. Could you
please tell me more about the problem?

And I have a worry. With this patch, the hotplug operation files
under /sys/bus/pci/slots/*/ directory will be created before the
interrupt is initialized. If hotplug operation is initiated (power
on/off, change attention indicator and so on) by user before
interrupt is initialized (though I think it's rare), command
completion event could not be handled. 

Thanks,
Kenji Kaneshige


Eric W. Biederman wrote:
> Move the enabling of interrupts after all of the data structures
> are setup so that we can safely run the interrupt handler as
> soon as it is registered.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> ---
>  drivers/pci/hotplug/pciehp.h      |    2 ++
>  drivers/pci/hotplug/pciehp_core.c |    7 +++++++
>  drivers/pci/hotplug/pciehp_hpc.c  |   15 +++++++--------
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index db85284..39ae375 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -111,6 +111,7 @@ struct controller {
>  	int cmd_busy;
>  	unsigned int no_cmd_complete:1;
>  	unsigned int link_active_reporting:1;
> +	unsigned int notification_enabled:1;
>  };
>  
>  #define INT_BUTTON_IGNORE		0
> @@ -170,6 +171,7 @@ extern int pciehp_configure_device(struct slot *p_slot);
>  extern int pciehp_unconfigure_device(struct slot *p_slot);
>  extern void pciehp_queue_pushbutton_work(struct work_struct *work);
>  struct controller *pcie_init(struct pcie_device *dev);
> +int pcie_init_notification(struct controller *ctrl);
>  int pciehp_enable_slot(struct slot *p_slot);
>  int pciehp_disable_slot(struct slot *p_slot);
>  int pcie_enable_notification(struct controller *ctrl);
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index c248554..681e391 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -434,6 +434,13 @@ static int pciehp_probe(struct pcie_device *dev, const struct pcie_port_service_
>  		goto err_out_release_ctlr;
>  	}
>  
> +	/* Enable events after we have setup the data structures */
> +	rc = pcie_init_notification(ctrl);
> +	if (rc) {
> +		ctrl_err(ctrl, "Notification initialization failed\n");
> +		goto err_out_release_ctlr;
> +	}
> +
>  	/* Check if slot is occupied */
>  	t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
>  	t_slot->hpc_ops->get_adapter_status(t_slot, &value);
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 71a8012..7a16c68 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -934,7 +934,7 @@ static void pcie_disable_notification(struct controller *ctrl)
>  		ctrl_warn(ctrl, "Cannot disable software notification\n");
>  }
>  
> -static int pcie_init_notification(struct controller *ctrl)
> +int pcie_init_notification(struct controller *ctrl)
>  {
>  	if (pciehp_request_irq(ctrl))
>  		return -1;
> @@ -942,13 +942,17 @@ static int pcie_init_notification(struct controller *ctrl)
>  		pciehp_free_irq(ctrl);
>  		return -1;
>  	}
> +	ctrl->notification_enabled = 1;
>  	return 0;
>  }
>  
>  static void pcie_shutdown_notification(struct controller *ctrl)
>  {
> -	pcie_disable_notification(ctrl);
> -	pciehp_free_irq(ctrl);
> +	if (ctrl->notification_enabled) {
> +		pcie_disable_notification(ctrl);
> +		pciehp_free_irq(ctrl);
> +		ctrl->notification_enabled = 0;
> +	}
>  }
>  
>  static int pcie_init_slot(struct controller *ctrl)
> @@ -1110,13 +1114,8 @@ struct controller *pcie_init(struct pcie_device *dev)
>  	if (pcie_init_slot(ctrl))
>  		goto abort_ctrl;
>  
> -	if (pcie_init_notification(ctrl))
> -		goto abort_slot;
> -
>  	return ctrl;
>  
> -abort_slot:
> -	pcie_cleanup_slot(ctrl);
>  abort_ctrl:
>  	kfree(ctrl);
>  abort:

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Jesse Barnes]

Any update here, Eric?  Sounds like you're using hotplug in real environments 
with complex topologies (based on your earlier messages), so we're interested 
in what you're seeing here...

Thanks,
Jesse

On Wednesday, January 28, 2009 11:34 pm Kenji Kaneshige wrote:
> Hi,
>
> I don't understand what this patch is trying to fix. Could you
> please tell me more about the problem?
>
> And I have a worry. With this patch, the hotplug operation files
> under /sys/bus/pci/slots/*/ directory will be created before the
> interrupt is initialized. If hotplug operation is initiated (power
> on/off, change attention indicator and so on) by user before
> interrupt is initialized (though I think it's rare), command
> completion event could not be handled.
>
> Thanks,
> Kenji Kaneshige
>
> Eric W. Biederman wrote:
> > Move the enabling of interrupts after all of the data structures
> > are setup so that we can safely run the interrupt handler as
> > soon as it is registered.
> >
> > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> > ---
> >  drivers/pci/hotplug/pciehp.h      |    2 ++
> >  drivers/pci/hotplug/pciehp_core.c |    7 +++++++
> >  drivers/pci/hotplug/pciehp_hpc.c  |   15 +++++++--------
> >  3 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> > index db85284..39ae375 100644
> > --- a/drivers/pci/hotplug/pciehp.h
> > +++ b/drivers/pci/hotplug/pciehp.h
> > @@ -111,6 +111,7 @@ struct controller {
> >  	int cmd_busy;
> >  	unsigned int no_cmd_complete:1;
> >  	unsigned int link_active_reporting:1;
> > +	unsigned int notification_enabled:1;
> >  };
> >
> >  #define INT_BUTTON_IGNORE		0
> > @@ -170,6 +171,7 @@ extern int pciehp_configure_device(struct slot
> > *p_slot); extern int pciehp_unconfigure_device(struct slot *p_slot);
> >  extern void pciehp_queue_pushbutton_work(struct work_struct *work);
> >  struct controller *pcie_init(struct pcie_device *dev);
> > +int pcie_init_notification(struct controller *ctrl);
> >  int pciehp_enable_slot(struct slot *p_slot);
> >  int pciehp_disable_slot(struct slot *p_slot);
> >  int pcie_enable_notification(struct controller *ctrl);
> > diff --git a/drivers/pci/hotplug/pciehp_core.c
> > b/drivers/pci/hotplug/pciehp_core.c index c248554..681e391 100644
> > --- a/drivers/pci/hotplug/pciehp_core.c
> > +++ b/drivers/pci/hotplug/pciehp_core.c
> > @@ -434,6 +434,13 @@ static int pciehp_probe(struct pcie_device *dev,
> > const struct pcie_port_service_ goto err_out_release_ctlr;
> >  	}
> >
> > +	/* Enable events after we have setup the data structures */
> > +	rc = pcie_init_notification(ctrl);
> > +	if (rc) {
> > +		ctrl_err(ctrl, "Notification initialization failed\n");
> > +		goto err_out_release_ctlr;
> > +	}
> > +
> >  	/* Check if slot is occupied */
> >  	t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
> >  	t_slot->hpc_ops->get_adapter_status(t_slot, &value);
> > diff --git a/drivers/pci/hotplug/pciehp_hpc.c
> > b/drivers/pci/hotplug/pciehp_hpc.c index 71a8012..7a16c68 100644
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -934,7 +934,7 @@ static void pcie_disable_notification(struct
> > controller *ctrl) ctrl_warn(ctrl, "Cannot disable software
> > notification\n");
> >  }
> >
> > -static int pcie_init_notification(struct controller *ctrl)
> > +int pcie_init_notification(struct controller *ctrl)
> >  {
> >  	if (pciehp_request_irq(ctrl))
> >  		return -1;
> > @@ -942,13 +942,17 @@ static int pcie_init_notification(struct controller
> > *ctrl) pciehp_free_irq(ctrl);
> >  		return -1;
> >  	}
> > +	ctrl->notification_enabled = 1;
> >  	return 0;
> >  }
> >
> >  static void pcie_shutdown_notification(struct controller *ctrl)
> >  {
> > -	pcie_disable_notification(ctrl);
> > -	pciehp_free_irq(ctrl);
> > +	if (ctrl->notification_enabled) {
> > +		pcie_disable_notification(ctrl);
> > +		pciehp_free_irq(ctrl);
> > +		ctrl->notification_enabled = 0;
> > +	}
> >  }
> >
> >  static int pcie_init_slot(struct controller *ctrl)
> > @@ -1110,13 +1114,8 @@ struct controller *pcie_init(struct pcie_device
> > *dev) if (pcie_init_slot(ctrl))
> >  		goto abort_ctrl;
> >
> > -	if (pcie_init_notification(ctrl))
> > -		goto abort_slot;
> > -
> >  	return ctrl;
> >
> > -abort_slot:
> > -	pcie_cleanup_slot(ctrl);
> >  abort_ctrl:
> >  	kfree(ctrl);
> >  abort:



-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Eric W. Biederman]

Jesse Barnes <jbarnes@virtuousgeek.org> writes:

> Any update here, Eric?  Sounds like you're using hotplug in real environments 
> with complex topologies (based on your earlier messages), so we're interested 
> in what you're seeing here...

Yes.

Currently I have a test system that is a subset of what I'm worried
about and will shortly have the real hardware, so my immediate goal is
to get things working well enough so my internal users won't get
blocked by bugs.  Currently I only have the pcie hotplug and pcie
hotplug surprise case.  My basic topology is 16 hotplug slots into
which I will be plugging in pci express switches with a couple of
additional hotplug slots.  As for the firmware, I will have it reserving
bus numbers and mmio space on each of the first 16 slots and the rest
is going to be up to the linux kernel.  This is an embedded design
so no ACPI is appears more pain than it is worth to implement.

I am also looking at the case of pcie switches with two upstream
ports, and switching which cpu they are connected to at runtime.  So
in some cases I will have devices whose presence is detected but will
not get link for hours or days, as opposed to the 20ms time limit in
the pci express specification.  Call it a necessary extension.

I need to revisit the pciehp driver but my first pass through it
looked like every corner case appeared to get something wrong. So I
have written myself a little 430 line replaces that handles the case
that I currently care about.  Part of what I was seeing before is that
we don't clear pending events in the pciehp driver before we enable
interrupts.  So if booting the system has left some pending and you
have CONFIG_DEBUG_SHIRQ enabled you get a nice oops because p_slot has
not been initialized and so the interrupts can't be handled.

My little driver is at least good enough to let me start looking at
other things.  I have just found yesterday that if you mmap a resource
in sysfs hot-remove doesn't complete.  Sysfs issues seem to be the
bane of my existence and I am currently working on a patch for that.

Currently the pcie port driver calls the remove methods of child
drivers twice if it removed (say because you have hot unplugged a
bridge).  Which is one of the truly nasty bugs I saw when I was trying
to bring up my test system, as things start access freed memory and
all kinds of silly things happen.

After I get the worst of the problems handled I intend to do a
thorough review and fix everything that I can see.

Eric

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Eric W. Biederman]

And on the big gotcha's I have found one more I am tracking.

I am seeing pci bridges with a NULL pointer for the subordinate bus.
Earlier I had thought that this was a symptom of the double remove
but I have been able to reproduce it without that.

On just a little bit deeper investigation it looks like the cases
are dying are all coming when the nested bridge reappears.

Which is wrong on so many levels as I am toggle power to the outer
slot, so the nested bridge should not even exist at that time.  Ugh.
More tracing to for me on that one.

watch -n1 lspci -t seems like a good way to exacerbate races, when performing
hotplug tests.

Eric

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> And on the big gotcha's I have found one more I am tracking.
>
> I am seeing pci bridges with a NULL pointer for the subordinate bus.
> Earlier I had thought that this was a symptom of the double remove
> but I have been able to reproduce it without that.
>
> On just a little bit deeper investigation it looks like the cases
> are dying are all coming when the nested bridge reappears.
>
> Which is wrong on so many levels as I am toggle power to the outer
> slot, so the nested bridge should not even exist at that time.  Ugh.
> More tracing to for me on that one.

Ok. Got it.  I was processing the interrupt for a device after it had
been hot removed but before the device state had disappeared.

pcie_isr looks like it would be even worse in that situation.  Looping forever
if pciehp_readw(ctrl, PCIE_EXPSLTA) always succeed sand returns 0xffff.

That loop in there appears impossibly misguided.  If the pending interrupt
values change after you have received the interrupt another instance
of the same interrupt should be pending so the loop should be completely
unnecessary.

Eric

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Kenji Kaneshige]

Eric W. Biederman wrote:
> ebiederm@xmission.com (Eric W. Biederman) writes:
> 
>> And on the big gotcha's I have found one more I am tracking.
>>
>> I am seeing pci bridges with a NULL pointer for the subordinate bus.
>> Earlier I had thought that this was a symptom of the double remove
>> but I have been able to reproduce it without that.
>>
>> On just a little bit deeper investigation it looks like the cases
>> are dying are all coming when the nested bridge reappears.
>>
>> Which is wrong on so many levels as I am toggle power to the outer
>> slot, so the nested bridge should not even exist at that time.  Ugh.
>> More tracing to for me on that one.
> 
> Ok. Got it.  I was processing the interrupt for a device after it had
> been hot removed but before the device state had disappeared.
> 
> pcie_isr looks like it would be even worse in that situation.  Looping forever
> if pciehp_readw(ctrl, PCIE_EXPSLTA) always succeed sand returns 0xffff.
> 
> That loop in there appears impossibly misguided.  If the pending interrupt
> values change after you have received the interrupt another instance
> of the same interrupt should be pending so the loop should be completely
> unnecessary.
> 

For level-triggered interrupt, I think it's true.

But for edge-triggered interrupt, I don't think it's true. I think
only one interrupt is generated if the first hotplug event occurs
and the second hotplug event occurs before clearing the status of
first hotplug event.

Thanks,
Kenji Kaneshige

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Eric W. Biederman]

Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:

> Eric W. Biederman wrote:
>> ebiederm@xmission.com (Eric W. Biederman) writes:
>> 
>>> And on the big gotcha's I have found one more I am tracking.
>>>
>>> I am seeing pci bridges with a NULL pointer for the subordinate bus.
>>> Earlier I had thought that this was a symptom of the double remove
>>> but I have been able to reproduce it without that.
>>>
>>> On just a little bit deeper investigation it looks like the cases
>>> are dying are all coming when the nested bridge reappears.
>>>
>>> Which is wrong on so many levels as I am toggle power to the outer
>>> slot, so the nested bridge should not even exist at that time.  Ugh.
>>> More tracing to for me on that one.
>> 
>> Ok. Got it.  I was processing the interrupt for a device after it had
>> been hot removed but before the device state had disappeared.
>> 
>> pcie_isr looks like it would be even worse in that situation.  Looping forever
>> if pciehp_readw(ctrl, PCIE_EXPSLTA) always succeed sand returns 0xffff.
>> 
>> That loop in there appears impossibly misguided.  If the pending interrupt
>> values change after you have received the interrupt another instance
>> of the same interrupt should be pending so the loop should be completely
>> unnecessary.
>> 
>
> For level-triggered interrupt, I think it's true.
>
> But for edge-triggered interrupt, I don't think it's true. I think
> only one interrupt is generated if the first hotplug event occurs
> and the second hotplug event occurs before clearing the status of
> first hotplug event.

My test case is edge-triggered MSI's.  The issue is that I get an interrupt
from the card that I am unplugging, but by the time the interrupt handler
is executed the card is physically absent, but the pci_dev structure is
still present in the kernel.

Eric

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Kenji Kaneshige]

Eric W. Biederman wrote:
> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:
> 
>> Eric W. Biederman wrote:
>>> ebiederm@xmission.com (Eric W. Biederman) writes:
>>>
>>>> And on the big gotcha's I have found one more I am tracking.
>>>>
>>>> I am seeing pci bridges with a NULL pointer for the subordinate bus.
>>>> Earlier I had thought that this was a symptom of the double remove
>>>> but I have been able to reproduce it without that.
>>>>
>>>> On just a little bit deeper investigation it looks like the cases
>>>> are dying are all coming when the nested bridge reappears.
>>>>
>>>> Which is wrong on so many levels as I am toggle power to the outer
>>>> slot, so the nested bridge should not even exist at that time.  Ugh.
>>>> More tracing to for me on that one.
>>> Ok. Got it.  I was processing the interrupt for a device after it had
>>> been hot removed but before the device state had disappeared.
>>>
>>> pcie_isr looks like it would be even worse in that situation.  Looping forever
>>> if pciehp_readw(ctrl, PCIE_EXPSLTA) always succeed sand returns 0xffff.
>>>
>>> That loop in there appears impossibly misguided.  If the pending interrupt
>>> values change after you have received the interrupt another instance
>>> of the same interrupt should be pending so the loop should be completely
>>> unnecessary.
>>>
>> For level-triggered interrupt, I think it's true.
>>
>> But for edge-triggered interrupt, I don't think it's true. I think
>> only one interrupt is generated if the first hotplug event occurs
>> and the second hotplug event occurs before clearing the status of
>> first hotplug event.
> 
> My test case is edge-triggered MSI's.  The issue is that I get an interrupt
> from the card that I am unplugging, but by the time the interrupt handler
> is executed the card is physically absent, but the pci_dev structure is
> still present in the kernel.
> 

Ok, I understood what is happening. Could you try the following patch?
It is currently in Jesse's linux-next.

http://marc.info/?l=linux-pci&m=123364118418484&w=2

BTW, I don't think surprise removal is well tested.

Thanks,
Kenji Kaneshige

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Eric W. Biederman]

Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:

> Ok, I understood what is happening. Could you try the following patch?
> It is currently in Jesse's linux-next.
>
> http://marc.info/?l=linux-pci&m=123364118418484&w=2
>
> BTW, I don't think surprise removal is well tested.

That patch should guarantee that we don't loop forever, and if we are
going to loop that looks like a reasonable way to handle it.

When I start working on what is the most maintainable way to implement
merge my hotplug driver work I will come back and test this.

At the moment it appears that it will at least suffer from detecting a
presence change event with a device showing up.  Before pci structure
for the device is removed.  I seem to recall some dead locks on the
pciehp work queue hotunplugging a hotplug driver as well.

Eric

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Jesse Barnes]

On Monday, February 23, 2009 3:08:44 am Eric W. Biederman wrote:
> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:
> > Ok, I understood what is happening. Could you try the following patch?
> > It is currently in Jesse's linux-next.
> >
> > http://marc.info/?l=linux-pci&m=123364118418484&w=2
> >
> > BTW, I don't think surprise removal is well tested.
>
> That patch should guarantee that we don't loop forever, and if we are
> going to loop that looks like a reasonable way to handle it.
>
> When I start working on what is the most maintainable way to implement
> merge my hotplug driver work I will come back and test this.
>
> At the moment it appears that it will at least suffer from detecting a
> presence change event with a device showing up.  Before pci structure
> for the device is removed.  I seem to recall some dead locks on the
> pciehp work queue hotunplugging a hotplug driver as well.

So where does that leave us with your original patch; do you think we need 
something in 2.6.29 for this issue?  Or are you planning on reworking things 
for .30?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Jesse Barnes]

On Monday, February 23, 2009 3:08:44 am Eric W. Biederman wrote:
> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:
> > Ok, I understood what is happening. Could you try the following patch?
> > It is currently in Jesse's linux-next.
> >
> > http://marc.info/?l=linux-pci&m=123364118418484&w=2
> >
> > BTW, I don't think surprise removal is well tested.
>
> That patch should guarantee that we don't loop forever, and if we are
> going to loop that looks like a reasonable way to handle it.
>
> When I start working on what is the most maintainable way to implement
> merge my hotplug driver work I will come back and test this.
>
> At the moment it appears that it will at least suffer from detecting a
> presence change event with a device showing up.  Before pci structure
> for the device is removed.  I seem to recall some dead locks on the
> pciehp work queue hotunplugging a hotplug driver as well.

Ah looks like Kenji-san wants this one pushed; so unless I hear otherwise I'll 
queue it up in my for-linus branch for Linus to pull tomorrow or so.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Kenji Kaneshige]

Eric W. Biederman wrote:
> Jesse Barnes <jbarnes@virtuousgeek.org> writes:
> 
>> Any update here, Eric?  Sounds like you're using hotplug in real environments 
>> with complex topologies (based on your earlier messages), so we're interested 
>> in what you're seeing here...
> 
> Yes.
> 
> Currently I have a test system that is a subset of what I'm worried
> about and will shortly have the real hardware, so my immediate goal is
> to get things working well enough so my internal users won't get
> blocked by bugs.  Currently I only have the pcie hotplug and pcie
> hotplug surprise case.  My basic topology is 16 hotplug slots into
> which I will be plugging in pci express switches with a couple of
> additional hotplug slots.  As for the firmware, I will have it reserving
> bus numbers and mmio space on each of the first 16 slots and the rest
> is going to be up to the linux kernel.  This is an embedded design
> so no ACPI is appears more pain than it is worth to implement.
> 

Very interesting. Can I ask you some questions?

- On hot-insertion of pci express switches with a additional hotplug
  slots, who initialize HwInit registers (for example, physical slot
  number field in the Slot Capabilities register)? OS, firmware,
  hardware or others?

- Bus numbers and MMIO space that needs to be reserved is depending
  on platform design. How do you tell kernel (or hotplug drivers) how
  many resources need to be reserved, in your current design?

> I am also looking at the case of pcie switches with two upstream
> ports, and switching which cpu they are connected to at runtime.  So
> in some cases I will have devices whose presence is detected but will
> not get link for hours or days, as opposed to the 20ms time limit in
> the pci express specification.  Call it a necessary extension.
> 
> I need to revisit the pciehp driver but my first pass through it
> looked like every corner case appeared to get something wrong. So I
> have written myself a little 430 line replaces that handles the case
> that I currently care about.  Part of what I was seeing before is that
> we don't clear pending events in the pciehp driver before we enable
> interrupts.  So if booting the system has left some pending and you
> have CONFIG_DEBUG_SHIRQ enabled you get a nice oops because p_slot has
> not been initialized and so the interrupts can't be handled.
> 

I've made a fix (c4635eb06af700820d658a163f06aff12e17cfb2) for a similar
problem several months ago. With this fix, pciehp had been changed to
initialize p_slot before installing interrupt service routine. So I still
don't understand what is happening. Could you please tell me the details
about "p_slot has not been initialized..."?

Thanks,
Kenji Kaneshige

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Eric W. Biederman]

Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:

> Eric W. Biederman wrote:
>> Jesse Barnes <jbarnes@virtuousgeek.org> writes:
>> 
>>> Any update here, Eric?  Sounds like you're using hotplug in real environments
>>> with complex topologies (based on your earlier messages), so we're interested
>>> in what you're seeing here...
>> 
>> Yes.
>> 
>> Currently I have a test system that is a subset of what I'm worried
>> about and will shortly have the real hardware, so my immediate goal is
>> to get things working well enough so my internal users won't get
>> blocked by bugs.  Currently I only have the pcie hotplug and pcie
>> hotplug surprise case.  My basic topology is 16 hotplug slots into
>> which I will be plugging in pci express switches with a couple of
>> additional hotplug slots.  As for the firmware, I will have it reserving
>> bus numbers and mmio space on each of the first 16 slots and the rest
>> is going to be up to the linux kernel.  This is an embedded design
>> so no ACPI is appears more pain than it is worth to implement.
>> 
>
> Very interesting. Can I ask you some questions?
>
> - On hot-insertion of pci express switches with a additional hotplug
>   slots, who initialize HwInit registers (for example, physical slot
>   number field in the Slot Capabilities register)? OS, firmware,
>   hardware or others?

It happens before the linux kernel gets to see it.  Call it firmware.

> - Bus numbers and MMIO space that needs to be reserved is depending
>   on platform design. How do you tell kernel (or hotplug drivers) how
>   many resources need to be reserved, in your current design?

So far it looks like I can get away without telling the kernel
anything, and just perform reservations at the layer of the
firmware on the primary board, and have the kernel see those
reservations when it boots up, and just subdivide them.

I have some thoughts on how to do things better but I'm not at a point
where it makes a difference right now.


>> I need to revisit the pciehp driver but my first pass through it
>> looked like every corner case appeared to get something wrong. So I
>> have written myself a little 430 line replaces that handles the case
>> that I currently care about.  Part of what I was seeing before is that
>> we don't clear pending events in the pciehp driver before we enable
>> interrupts.  So if booting the system has left some pending and you
>> have CONFIG_DEBUG_SHIRQ enabled you get a nice oops because p_slot has
>> not been initialized and so the interrupts can't be handled.
>> 
>
> I've made a fix (c4635eb06af700820d658a163f06aff12e17cfb2) for a similar
> problem several months ago. With this fix, pciehp had been changed to
> initialize p_slot before installing interrupt service routine. So I still
> don't understand what is happening. Could you please tell me the details
> about "p_slot has not been initialized..."?

kobject_name is not initialized, and slot_name(p_slot) calls
hoptlug_slot_name which calls pci_slot_name which kobj_name. 
It looks like this problem was introduced in commit 
e1acb24f059defdaa0264e925f19cc21b0a3e592

Eric

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Kenji Kaneshige]

Eric W. Biederman wrote:
> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:
> 
>> Eric W. Biederman wrote:
>>> Jesse Barnes <jbarnes@virtuousgeek.org> writes:
>>>
>>>> Any update here, Eric?  Sounds like you're using hotplug in real environments
>>>> with complex topologies (based on your earlier messages), so we're interested
>>>> in what you're seeing here...
>>> Yes.
>>>
>>> Currently I have a test system that is a subset of what I'm worried
>>> about and will shortly have the real hardware, so my immediate goal is
>>> to get things working well enough so my internal users won't get
>>> blocked by bugs.  Currently I only have the pcie hotplug and pcie
>>> hotplug surprise case.  My basic topology is 16 hotplug slots into
>>> which I will be plugging in pci express switches with a couple of
>>> additional hotplug slots.  As for the firmware, I will have it reserving
>>> bus numbers and mmio space on each of the first 16 slots and the rest
>>> is going to be up to the linux kernel.  This is an embedded design
>>> so no ACPI is appears more pain than it is worth to implement.
>>>
>> Very interesting. Can I ask you some questions?
>>
>> - On hot-insertion of pci express switches with a additional hotplug
>>   slots, who initialize HwInit registers (for example, physical slot
>>   number field in the Slot Capabilities register)? OS, firmware,
>>   hardware or others?
> 
> It happens before the linux kernel gets to see it.  Call it firmware.
> 
>> - Bus numbers and MMIO space that needs to be reserved is depending
>>   on platform design. How do you tell kernel (or hotplug drivers) how
>>   many resources need to be reserved, in your current design?
> 
> So far it looks like I can get away without telling the kernel
> anything, and just perform reservations at the layer of the
> firmware on the primary board, and have the kernel see those
> reservations when it boots up, and just subdivide them.
> 
> I have some thoughts on how to do things better but I'm not at a point
> where it makes a difference right now.
> 

In the current pciehp implementation, minimum resources enough to
enable devices under the bridge are assigned when P2P bridge is
hot-added. My concern is that enough resources are NOT assigned to
the bridge if an additional slot is empty. As a result, hot-add
adapter card on the additional slot won't work because of resource
shortage.

>>> I need to revisit the pciehp driver but my first pass through it
>>> looked like every corner case appeared to get something wrong. So I
>>> have written myself a little 430 line replaces that handles the case
>>> that I currently care about.  Part of what I was seeing before is that
>>> we don't clear pending events in the pciehp driver before we enable
>>> interrupts.  So if booting the system has left some pending and you
>>> have CONFIG_DEBUG_SHIRQ enabled you get a nice oops because p_slot has
>>> not been initialized and so the interrupts can't be handled.
>>>
>> I've made a fix (c4635eb06af700820d658a163f06aff12e17cfb2) for a similar
>> problem several months ago. With this fix, pciehp had been changed to
>> initialize p_slot before installing interrupt service routine. So I still
>> don't understand what is happening. Could you please tell me the details
>> about "p_slot has not been initialized..."?
> 
> kobject_name is not initialized, and slot_name(p_slot) calls
> hoptlug_slot_name which calls pci_slot_name which kobj_name. 
> It looks like this problem was introduced in commit 
> e1acb24f059defdaa0264e925f19cc21b0a3e592

Thank your for the information. I understood what is happening.
This needs to be fixed. But, as I mentioned before, I think
software notification mechanism should be initialized before
sysfs entries are created. I'll consider alternative fix.

Thanks,
Kenji Kaneshige

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Eric W. Biederman]

Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:

> In the current pciehp implementation, minimum resources enough to
> enable devices under the bridge are assigned when P2P bridge is
> hot-added. My concern is that enough resources are NOT assigned to
> the bridge if an additional slot is empty. As a result, hot-add
> adapter card on the additional slot won't work because of resource
> shortage.

It is a good concern.  Right now I know I won't need a bus number
but you are quite right the mmio and iospace may be a problem.

My preliminary test case doesn't cover that so I will look and
confirm it is a problem I need to address.

>> kobject_name is not initialized, and slot_name(p_slot) calls
>> hoptlug_slot_name which calls pci_slot_name which kobj_name. 
>> It looks like this problem was introduced in commit 
>> e1acb24f059defdaa0264e925f19cc21b0a3e592
>
> Thank your for the information. I understood what is happening.
> This needs to be fixed. But, as I mentioned before, I think
> software notification mechanism should be initialized before
> sysfs entries are created. I'll consider alternative fix.

Reasonable.  I haven't had the need nor gotten brave enough to
support those sysfs entries in my minimal driver.

Eric

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Kenji Kaneshige]

Eric W. Biederman wrote:
> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:
> 
>> In the current pciehp implementation, minimum resources enough to
>> enable devices under the bridge are assigned when P2P bridge is
>> hot-added. My concern is that enough resources are NOT assigned to
>> the bridge if an additional slot is empty. As a result, hot-add
>> adapter card on the additional slot won't work because of resource
>> shortage.
> 
> It is a good concern.  Right now I know I won't need a bus number
> but you are quite right the mmio and iospace may be a problem.
> 
> My preliminary test case doesn't cover that so I will look and
> confirm it is a problem I need to address.
> 
>>> kobject_name is not initialized, and slot_name(p_slot) calls
>>> hoptlug_slot_name which calls pci_slot_name which kobj_name. 
>>> It looks like this problem was introduced in commit 
>>> e1acb24f059defdaa0264e925f19cc21b0a3e592
>> Thank your for the information. I understood what is happening.
>> This needs to be fixed. But, as I mentioned before, I think
>> software notification mechanism should be initialized before
>> sysfs entries are created. I'll consider alternative fix.
> 
> Reasonable.  I haven't had the need nor gotten brave enough to
> support those sysfs entries in my minimal driver.
> 

Eric-san, Jesse-san,

I tried to make alternative fix, but now I have a conclusion that
Eric-san's patch is the best solution. The reasons are

- Eric-san's approach prevents the regression such as commit
  e1acb24f059defdaa0264e925f19cc21b0a3e592.

- My concern was that command completion might not be handled if
  we enable software notification mechanism after creating sysfs
  entries. But it is not true because pciehp uses polling approach
  if command is issued before enabling software notification (I
  made it, but I forgot it...). Just in case, I confirmed that
  hotplug operations (power on/off, attention indication on/off
  and so on) works even if I make pciehp_init_notification() NOP.

Jesse-san, could you consider applying Eric-san's patch?

Here are my

Reviewed-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Tested-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

Note: I tested Eric-san's patch on Jesse-san's for-linus tree.

Thanks,
Kenji Kaneshige

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:
>
>> In the current pciehp implementation, minimum resources enough to
>> enable devices under the bridge are assigned when P2P bridge is
>> hot-added. My concern is that enough resources are NOT assigned to
>> the bridge if an additional slot is empty. As a result, hot-add
>> adapter card on the additional slot won't work because of resource
>> shortage.
>
> It is a good concern.  Right now I know I won't need a bus number
> but you are quite right the mmio and iospace may be a problem.
>
> My preliminary test case doesn't cover that so I will look and
> confirm it is a problem I need to address.

Tested.  I do have a problem with not assigning resources to new
bridges.

My plan to handle this currently is to do what the pci code does for
cardbus without resources.  To give them a fixed sized resource
assignment without looking downstream from the bridge.  The code
looks simple, and we have existing practice so it should not be too
bad to deal with.

Eric

Re: [PATCH] pciehp: Handle interrupts that happen during initialization.

[Jesse Barnes]

On Wednesday, January 28, 2009 7:31:18 pm Eric W. Biederman wrote:
> Move the enabling of interrupts after all of the data structures
> are setup so that we can safely run the interrupt handler as
> soon as it is registered.
>
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>

Applied to my for-linus branch, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center