Re: [PATCH 6/16][BUG] ACPI pci_slot: Fix slot removal path (Not for mainline!)

[Alex Chiang <achiang@hp.com>]

* Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
> Current ACPI pci slot driver scans ACPI namespace and slot list on
> pci_bus structure to find the pci_slot pointer. It is definitely
> redundant. In addition, it scans slot list on pci_bus without holding
> pci_bus_sem. This patch fixes those problems.

Tested and merged, thanks.

/ac

> 
> Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> 
> ---
>  drivers/acpi/pci_slot.c |  104 +++++++++++++++++++++++++-----------------------
>  1 file changed, 55 insertions(+), 49 deletions(-)
> 
> Index: linux-2.6.25-rc6/drivers/acpi/pci_slot.c
> ===================================================================
> --- linux-2.6.25-rc6.orig/drivers/acpi/pci_slot.c
> +++ linux-2.6.25-rc6/drivers/acpi/pci_slot.c
> @@ -55,9 +55,17 @@ ACPI_MODULE_NAME("pci_slot");
>  				MY_NAME , ## arg);		\
>  	} while (0)
>  
> +struct acpi_pci_slot {
> +	acpi_handle root_handle;	/* handle of the root bridge */
> +	struct pci_slot *pci_slot;	/* corresponding pci_slot */
> +	struct list_head list;		/* node in the list of slots */
> +};
> +
>  static int acpi_pci_slot_add(acpi_handle handle);
>  static void acpi_pci_slot_remove(acpi_handle handle);
>  
> +static LIST_HEAD(slot_list);
> +static DEFINE_MUTEX(slot_list_lock);
>  static struct acpi_pci_driver acpi_pci_slot_driver = {
>  	.add = acpi_pci_slot_add,
>  	.remove = acpi_pci_slot_remove,
> @@ -105,34 +113,11 @@ out:
>  	return retval;
>  }
>  
> -/*
> - * unregister_slot
> - *
> - * Called once for each SxFy object in the namespace. Each call to
> - * pci_destroy_slot decrements the refcount on the pci_slot, and
> - * eventually calls kobject_unregister at the appropriate time.
> - */
> -static acpi_status
> -unregister_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
> -{
> -	int device;
> -	unsigned long sun;
> -	struct pci_slot *slot, *tmp;
> -	struct pci_bus *pci_bus = context;
> -
> -	if (check_slot(handle, &device, &sun))
> -		return AE_OK;
> -
> -	/* FIXME - Need pci_bus_sem to be held */
> -	list_for_each_entry_safe(slot, tmp, &pci_bus->slots, list) {
> -		if (slot->number == device) {
> -			pci_destroy_slot(slot);
> -			break;
> -		}
> -	}
> -
> -	return AE_OK;
> -}
> +struct callback_args {
> +	acpi_walk_callback	user_function;	/* only for walk_p2p_bridge */
> +	struct pci_bus		*pci_bus;
> +	acpi_handle		root_handle;
> +};
>  
>  /*
>   * register_slot
> @@ -150,17 +135,33 @@ register_slot(acpi_handle handle, u32 lv
>  	int device;
>  	unsigned long sun;
>  	char name[KOBJ_NAME_LEN];
> -
> +	struct acpi_pci_slot *slot;
>  	struct pci_slot *pci_slot;
> -	struct pci_bus *pci_bus = context;
> +	struct callback_args *parent_context = context;
> +	struct pci_bus *pci_bus = parent_context->pci_bus;
>  
>  	if (check_slot(handle, &device, &sun))
>  		return AE_OK;
>  
> +	slot = kmalloc(sizeof(*slot), GFP_KERNEL);
> +	if (!slot) {
> +		err("%s: cannot allocate memory\n", __FUNCTION__);
> +		return AE_OK;
> +	}
> +
>  	snprintf(name, sizeof(name), "%u", (u32)sun);
>  	pci_slot = pci_create_slot(pci_bus, device, name);
> -	if (IS_ERR(pci_slot))
> +	if (IS_ERR(pci_slot)) {
>  		err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
> +		kfree(slot);
> +	}
> +
> +	slot->root_handle = parent_context->root_handle;
> +	slot->pci_slot = pci_slot;
> +	INIT_LIST_HEAD(&slot->list);
> +	mutex_lock(&slot_list_lock);
> +	list_add(&slot->list, &slot_list);
> +	mutex_unlock(&slot_list_lock);
>  
>  	dbg("pci_slot: %#lx, pci_bus: %x, device: %d, name: %s\n",
>  		(uint64_t)pci_slot, pci_bus->number, device, name);
> @@ -168,11 +169,6 @@ register_slot(acpi_handle handle, u32 lv
>  	return AE_OK;
>  }
>  
> -struct p2p_bridge_context {
> -	acpi_walk_callback	user_function;
> -	struct pci_bus		*pci_bus;
> -};
> -
>  /*
>   * walk_p2p_bridge - discover and walk p2p bridges
>   * @handle: points to an acpi_pci_root
> @@ -192,8 +188,8 @@ walk_p2p_bridge(acpi_handle handle, u32 
>  
>  	struct pci_dev *dev;
>  	struct pci_bus *pci_bus;
> -	struct p2p_bridge_context child_context;
> -	struct p2p_bridge_context *parent_context = context;
> +	struct callback_args child_context;
> +	struct callback_args *parent_context = context;
>  
>  	pci_bus = parent_context->pci_bus;
>  	user_function = parent_context->user_function;
> @@ -213,14 +209,16 @@ walk_p2p_bridge(acpi_handle handle, u32 
>  	if (!dev || !dev->subordinate)
>  		goto out;
>  
> +	child_context.pci_bus = dev->subordinate;
> +	child_context.user_function = user_function;
> +	child_context.root_handle = parent_context->root_handle;
> +
>  	dbg("p2p bridge walk, pci_bus = %x\n", dev->subordinate->number);
>  	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> -				     user_function, dev->subordinate, NULL);
> +				     user_function, &child_context, NULL);
>  	if (ACPI_FAILURE(status))
>  		goto out;
>  
> -	child_context.pci_bus = dev->subordinate;
> -	child_context.user_function = user_function;
>  	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>  				     walk_p2p_bridge, &child_context, NULL);
>  out:
> @@ -246,7 +244,7 @@ walk_root_bridge(acpi_handle handle, acp
>  	acpi_status status;
>  	acpi_handle dummy_handle;
>  	struct pci_bus *pci_bus;
> -	struct p2p_bridge_context context;
> +	struct callback_args context;
>  
>  	/* If the bridge doesn't have _STA, we assume it is always there */
>  	status = acpi_get_handle(handle, "_STA", &dummy_handle);
> @@ -271,14 +269,16 @@ walk_root_bridge(acpi_handle handle, acp
>  	if (!pci_bus)
>  		return 0;
>  
> +	context.pci_bus = pci_bus;
> +	context.user_function = user_function;
> +	context.root_handle = handle;
> +
>  	dbg("root bridge walk, pci_bus = %x\n", pci_bus->number);
>  	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
> -				     user_function, pci_bus, NULL);
> +				     user_function, &context, NULL);
>  	if (ACPI_FAILURE(status))
>  		return status;
>  
> -	context.pci_bus = pci_bus;
> -	context.user_function = user_function;
>  	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
>  				     walk_p2p_bridge, &context, NULL);
>  	if (ACPI_FAILURE(status))
> @@ -310,11 +310,17 @@ acpi_pci_slot_add(acpi_handle handle)
>  static void
>  acpi_pci_slot_remove(acpi_handle handle)
>  {
> -	acpi_status status;
> +	struct acpi_pci_slot *slot, *tmp;
>  
> -	status = walk_root_bridge(handle, unregister_slot);
> -	if (ACPI_FAILURE(status))
> -		err("%s: unregister_slot failure - %d\n", __func__, status);
> +	mutex_lock(&slot_list_lock);
> +	list_for_each_entry_safe(slot, tmp, &slot_list, list) {
> +		if (slot->root_handle == handle) {
> +			list_del(&slot->list);
> +			pci_destroy_slot(slot->pci_slot);
> +			kfree(slot);
> +		}
> +	}
> +	mutex_unlock(&slot_list_lock);
>  }
>  
>  #ifdef CONFIG_DMI
> 
>