linux-kernel - Re: [RFC Patch 17/19] resources: Move struct resource_list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 21 Jan 2015 14:37:05 +0800
From:	Jiang Liu <jiang.liu@...ux.intel.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
CC:	Thomas Gleixner <tglx@...utronix.de>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Yinghai Lu <yinghai@...nel.org>,
	Borislav Petkov <bp@...en8.de>, Len Brown <lenb@...nel.org>,
	Dan Williams <dan.j.williams@...el.com>,
	Vinod Koul <vinod.koul@...el.com>,
	Vivek Goyal <vgoyal@...hat.com>,
	Thierry Reding <treding@...dia.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mike Travis <travis@....com>, Tony Luck <tony.luck@...el.com>,
	x86@...nel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-acpi@...r.kernel.org,
	dmaengine@...r.kernel.org
Subject: Re: [RFC Patch 17/19] resources: Move struct resource_list_entry
 from ACPI into resource core

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/