[Pcihpd-discuss] Re: [PATCH] rpaphp.patch -- multi-function devices not handled correctly

[Linda Xie]

Hi Greg,

Thank you very much for your comments.  The attached patch contains
the following fixes. (See my comments below).

John,  your rpadlpar patch has been merged.


Linda


Greg KH wrote:

>On Mon, Jun 07, 2004 at 05:39:03PM -0500, Linda Xie wrote:
>
>
>>+#ifdef DEBUG
>>+static void print_slot_pci_funcs(struct slot *slot)
>>+{
>>+	struct list_head *l;
>>+
>>+	if (slot->dev_type == PCI_DEV) {
>>+		printk("pci_funcs of slot[%s]\n", slot->name);
>>+		if (list_empty(&slot->dev.pci_funcs))
>>+			printk("	pci_funcs is EMPTY\n");
>>+
>>+		list_for_each (l, &slot->dev.pci_funcs) {
>>+			struct rpaphp_pci_func *func =
>>+				list_entry(l, struct rpaphp_pci_func, sibling);
>>+			printk("	FOUND dev=%s\n", pci_name(func->pci_dev));
>>+		}
>>+	}
>>+}
>>+#endif
>>
>>
>
>Please provide an empty version of this function if DEBUG is not
>defined.  That way you get rid of the #ifdef in the places where you
>call this function.
>
Fixed.

>
>
>>+static ssize_t removable_read_file (struct hotplug_slot *php_slot, char *buf)
>>+{
>>+        u8 value;
>>+        int retval = -ENOENT;
>>+	struct slot *slot = (struct slot *)php_slot->private;
>>+
>>+	if (!slot)
>>+		return retval;
>>+
>>+        value = slot->removable;
>>+        retval = sprintf (buf, "%d\n", value);
>>+        return retval;
>>+}
>>
>>
>
>Hm, you used spaces here instead of tabs :(
>
Fixed.

>
>
>>+static struct hotplug_slot_attribute hotplug_slot_attr_removable = {
>>+	.attr = {.name ="phy_removable", .mode = S_IFREG | S_IRUGO},
>>+	.show = removable_read_file,
>>+};
>>
>>
>
>Please use the proper macros for this.  You didn't set the owner field
>due to this :(
>
>
I checked the owner field of the file, it did get set. I found that
pci_hotplug_core.c uses the same macors for "adapter" (read only
attribute). Am I missing anything?

>
>
>
>>+
>>+static void rpaphp_sysfs_add_attr_removable (struct hotplug_slot *slot)
>>+{
>>+	sysfs_create_file(&slot->kobj, &hotplug_slot_attr_removable.attr);
>>+}
>>+
>>+void rpaphp_sysfs_remove_attr_removable (struct hotplug_slot *slot)
>>+{
>>+	sysfs_remove_file(&slot->kobj, &hotplug_slot_attr_removable.attr);
>>+}
>>
>>
>
>Why is the add files function static, yet the remove one global?
>That's implying that your create is done at a different "level" than the
>remove, which it looks like it is.  Should this be also fixed up?
>
Fixed.

>
>
>>+static int is_registered(struct slot *slot)
>>+{
>>+	struct list_head        *tmp, *n;
>>+	struct slot             *tmp_slot;
>>+
>>+	list_for_each_safe(tmp, n, &rpaphp_slot_head) {
>>+		tmp_slot = list_entry(tmp, struct slot, rpaphp_slot_list);
>>+		if (!strcmp(tmp_slot->name, slot->name))
>>+			return 1;
>>+	}
>>+	return 0;
>>+}
>>
>>
>
>list_for_each_safe() is not needed here.  How about a simple
>list_for_each_entry() instead?
>
Fixed.

>Also, the pci hotplug core will catch this error, right?  Why duplicate
>the logic again?
>
I don't think that the pci hotplug core handles  this error very nicely
-- crahsing the system, if pci_hp_register(slot) is called with the same
slot->name that has been registered before.

>
>
>
>
>>+	/* 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);
>>+	}
>>
>>
>
>return is not a function :)
>
changed to  "return 1" .

>
>
>>+static char *get_my_drc_name(struct device_node *dn, u32 my_drc_index)
>>+{
>>+        char *name, *ptr = NULL;
>>+        int *drc_names, *drc_indexes, i;
>>+	struct device_node *parent = dn->parent;
>>+
>>+	if (!parent)
>>+		return ptr;
>>+
>>+        drc_names = (int *) get_property(parent, "ibm,drc-names", NULL);
>>+        drc_indexes = (int *) get_property(parent, "ibm,drc-indexes", NULL);
>>+        if (!drc_names || !drc_indexes)
>>+		return ptr;
>>+
>>+	name = (char *) &drc_names[1];
>>+	for (i = 0; i < drc_indexes[0]; i++, name += (strlen(name) + 1)) {
>>+		if (drc_indexes[i + 1] == my_drc_index) {
>>+                	ptr = (char *) name;
>>+			break;
>>+		}
>>+	}
>>+
>>+        return ptr;
>>+}
>>
>>
>>
>
>Again with the spaces...
>
Fixed.

>thanks,
>
>greg k-h
>
>
>
>
>

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: rpaphp_updated.patch
Url: http://ozlabs.org/pipermail/linuxppc64-dev/attachments/20040609/f5a63997/attachment.txt