Re: [PATCH 3/6] EDAC mc numbers refactor 2-of-2

[Andrew Morton <akpm@osdl.org>]

Doug Thompson <norsk5@yahoo.com> wrote:
>
> From: Doug Thompson <norsk5@xmission.com>
> 
> This is part 2 of a 2-part patch set.
> 
> 1 Reimplement add_mc_to_global_list() with semantics that allow the caller to
>   determine the ID number for a mem_ctl_info structure.  Then modify
>   edac_mc_add_mc() so that the caller specifies the ID number for the new
>   mem_ctl_info structure.  Platform-specific code should be able to assign the
>   ID numbers in a platform-specific manner.  For instance, on Opteron it makes
>   sense to have the ID of the mem_ctl_info structure match the ID of the node
>   that the memory controller belongs to.
> 
> 2 Modify callers of edac_mc_add_mc() so they use the new semantics.
> 
> ...
>  
> +/* Return 0 on success, 1 on failure.
> + * Before calling this function, caller must
> + * assign a unique value to mci->mc_idx.
> + */
> +static int add_mc_to_global_list (struct mem_ctl_info *mci)
> +{
> +	struct list_head *item, *insert_before;
> +	struct mem_ctl_info *p;
> +
> +	insert_before = &mc_devices;
> +
> +	if (unlikely((p = find_mci_by_dev(mci->dev)) != NULL))
> +		goto fail0;
> +
> +	list_for_each(item, &mc_devices) {
> +		p = list_entry(item, struct mem_ctl_info, link);
> +
> +		if (p->mc_idx >= mci->mc_idx) {
> +			if (unlikely(p->mc_idx == mci->mc_idx))
> +				goto fail1;
> +
> +			insert_before = item;
> +			break;
> +		}
> +	}
> +
> +	list_add_tail_rcu(&mci->link, insert_before);
> +	return 0;
> +
> +fail0:
> +	edac_printk(KERN_WARNING, EDAC_MC,
> +		    "%s (%s) %s %s already assigned %d\n", p->dev->bus_id,
> +		    dev_name(p->dev), p->mod_name, p->ctl_name, p->mc_idx);
> +	return 1;
> +
> +fail1:
> +	edac_printk(KERN_WARNING, EDAC_MC,
> +		    "bug in low-level driver: attempt to assign\n"
> +		    "    duplicate mc_idx %d in %s()\n", p->mc_idx, __func__);
> +	return 1;
> +}

I assume the caller must hold some lock to prevent the list from getting
trashed, but I didn't locate it in (literally) a five-second search.  I'd
suggest that a comment describing the locking requirements be added to this
function.