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

[Greg KH]

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.

> +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 :(

> +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 :(


> +
> +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?

> +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?

Also, the pci hotplug core will catch this error, right?  Why duplicate
the logic again?

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

> +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...

thanks,

greg k-h

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