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

Kenji Kaneshige
2009-01-29T07:34:31Z

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);