Re: [RFC Patch 17/19] resources: Move struct resource_list_entry from ACPI into resource core

[Jiang Liu]

On 2015/1/21 9:10, Rafael J. Wysocki wrote:
> On Thursday, January 08, 2015 10:33:04 AM Jiang Liu wrote:
<snit>
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 0bcebffc4e77..414183809383 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -1529,6 +1529,54 @@ int iomem_is_exclusive(u64 addr)
>>  	return err;
>>  }
>>  
>> +struct resource_list_entry *resource_list_alloc(struct resource *res,
>> +						size_t extra_size)
> 
> What about create_resource_list_entry()?  Less confusing surely.
Sure, I will rename it as resource_list_create_entry().

> 
>> +{
>> +	struct resource_list_entry *entry;
>> +
>> +	entry = kzalloc(sizeof(*entry) + extra_size, GFP_KERNEL);
>> +	if (entry) {
>> +		INIT_LIST_HEAD(&entry->node);
>> +		entry->res = res ? res : &entry->__res;
>> +	}
>> +
>> +	return entry;
>> +}
>> +EXPORT_SYMBOL(resource_list_alloc);
>> +
>> +void resource_list_free(struct resource_list_entry *entry)
>> +{
>> +	kfree(entry);
>> +}
>> +EXPORT_SYMBOL(resource_list_free);
> 
> Well, I'm not sure I like this.  The name suggests that it would free the
> entire list and what's wrong with using kfree() directly on "entry" anyway?
I just want to make interface symmetric. We may also support some type
of callback when freeing resources in future.

> 
>> +
>> +void resource_list_insert(struct list_head *head,
>> +			  struct resource_list_entry *entry, bool tail)
> 
> I would call this resource_list_add() if anything.
> 
> Also it may be better to have two helpers, one for "add" and one for "add_tail"
> (and perhaps define them as static inline?).
We can't use inline functions here because that needs pulling list.h
into ioport.h, then causing building issues to header inclusion order.

> 
> And why change the ordering between "head" and "entry".  That's alomost
> guaranteed to confuse people.
My fault, will change in next version.
> 
>> +{
>> +	if (tail)
>> +		list_add_tail(&entry->node, head);
>> +	else
>> +		list_add(&entry->node, head);
>> +}
>> +EXPORT_SYMBOL(resource_list_insert);
>> +
>> +void resource_list_delete(struct resource_list_entry *entry)
>> +{
>> +	list_del(&entry->node);
>> +}
>> +EXPORT_SYMBOL(resource_list_delete);
> 
> Couldn't this be a static inline)?
Inline will cause header file inclusion order issue:(

> 
> Or maybe we can combine the "list_del" with "kfree" in one function?
There are callers which need separating list_del from kfree,
so exported two interfaces here. Will add another helper interface
resource_list_destroy_entry().

Regards!
Gerry
> 
>> +
>> +void resource_list_free_list(struct list_head *head)
>> +{
>> +	struct resource_list_entry *entry, *tmp;
>> +
>> +	list_for_each_entry_safe(entry, tmp, head, node) {
>> +		list_del(&entry->node);
>> +		resource_list_free(entry);
>> +	}
>> +}
>> +EXPORT_SYMBOL(resource_list_free_list);
>> +
>>  static int __init strict_iomem(char *str)
>>  {
>>  	if (strstr(str, "relaxed"))
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/