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

[Linda Xie]

Hi John,

Thank you for your comments. They are very good suggestions.

Linda

John Rose wrote:

>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".
>
>
 I will pick "phys_removable" for a new attribute 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.
>
>
Actually, get_sensor_state always returns "present" for "embedded" slot.

>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.
>
Will be changed to "POWER_ON".

>
>
>
>>+
>>+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.
>
is_dr_dn() doesn't check for drc-indexes, only my-drc-index.

>
>
>
>>-	int err = -EINVAL;
>>+	int err = -EINVAL;;
>>
>>
>
>Is this a typo?
>
It's 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.
>
Will be changed.

>
>
>
>>-
>>+	/* 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/