[PATCH] rpaphp.patch -- multi-function devices not handled correctly

[John Rose]

Hi Linda-

Nice work.  I have a couple of comments.

> Along with above changes,  I added LDRSLOT(logical I/O slot) support.
> We need LDRSLOT support for DLPAR I/O.  A card in a LDRSLOT can't
> be physically removed, but can be logically removed  from one partiton and
> reassinged to another partition.
>

This handles the case of internal or "embedded" slots, right?  It might
be more clear, both in your code and comments, to use "embedded" rather
than "logical DR".

I also think it's necessary to add a new attribute "embedded" or
"phys_removable" or whatever, that tells userspace whether physical
removal is possible.  This allows the userspace tool to say "The slot
exists, but is not hotplug capable", rather than "bad slot name".

> @@ -155,7 +158,10 @@
>  	/*  have to go through this */
>  	switch (slot->dev_type) {
>  	case PCI_DEV:
> -		retval = rpaphp_get_pci_adapter_status(slot, 0, value);
> +		if (slot->type != LDRSLOT)
> +			retval = rpaphp_get_pci_adapter_status(slot, 0, value);
> +		else
> +			retval = -EINVAL; /* n/a for LDRSLOT */
>  		break;
>  	case VIO_DEV:
>  		retval = rpaphp_get_vio_adapter_status(slot, 0, value);

I don't agree with this.  The user should still be able to check the
adapter status of an embedded slot.  You could change
rpaphp_get_pci_adapter_status() to always assume PRESENT for the
get-sensor-state call. Then check the pci_funcs list just like in the
normal case.

Another thing I noticed is that the Logical DR/embedded case is ignored
for the rpaphp_get_power_level() call.  When a user cats the power file,
it should give a valid answer for an embedded slot.  Since these slots
don't have power-domain values, and you can't turn them off, this
function should always return "POWER_ON" for these cases.

> +
> +static int is_dr_dn(struct device_node *dn, int **indexes, int **names, int **types,
> +	  int **power_domains, int **my_drc_index)
> +{
> +	if (!is_hotplug_capable(dn))
> +		return (0);
> +
> +	*my_drc_index = (int *) get_property(dn, "ibm,my-drc-index", NULL);
> +	if(!*my_drc_index)
> +		return (0);
> +
> +	if (!dn->parent)
> +		return (0);
> +
> +	return get_dn_properties(dn->parent, indexes, names, types, power_domains);
>  }

This will fail for embedded slots, right?  Embedded slots won't have
drc-indexes, types, or domains.

> -	int err = -EINVAL;
> +	int err = -EINVAL;;

Is this a typo?

> +	/* remove the devices from the pci core */
> +	list_for_each (ln, &slot->dev.pci_funcs) {
> +		struct rpaphp_pci_func *func;
> +
> +		func = list_entry(ln, struct rpaphp_pci_func, sibling);
> +		if (func->pci_dev) {
> +			rpaphp_eeh_remove_bus_device(func->pci_dev);
> +			pci_remove_bus_device(func->pci_dev);
> +		}

list_for_each() isn't safe to use when removing members of the list in
question.  There are functions list_for_each_safe() and/or
list_for_each_entry_safe() that address this problem.

> -
> +	/* should not try to register the same slot twice */
> +	if (is_registered(slot)) { /* should't be here */
> +		err("register_slot: slot[%s] is already registered\n", slot->name);
> +		rpaphp_release_slot(slot->hotplug_slot);
> +		return (1);
> +	}

Good idea, this will save us alot of headaches :)


** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/