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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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