linux-kernel - Re: [PATCH v6] devres: combine function devm

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:   Mon, 12 Feb 2018 12:21:17 +0100
From:   Christophe LEROY <christophe.leroy@....fr>
To:     Yisheng Xie <xieyisheng1@...wei.com>, gregkh@...uxfoundation.org
Cc:     thomas.lendacky@....com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] devres: combine function devm_ioremap*

Hi Yishen,

I don't have any comment.

Reviewed-by: <christophe.leroy@....fr>

Christophe

Le 12/02/2018 à 12:07, Yisheng Xie a écrit :
> Hi Greg, Christophe,
> 
> Any comment about this version?  And sorry to disturb  :)
> 
> Thanks
> Yisheng
> 
> On 2018/1/29 19:48, Yisheng Xie wrote:
>> When I tried to use devm_ioremap function and review related
>> code, I found devm_ioremap_* almost have the similar realize
>> with each other, which can be combined.
>>
>> In the former version, I have tried to kill ioremap_cache to
>> reduce the size of devres, which can not work for ioremap is
>> not the same as ioremap_nocache in some ARCHs likes ia64.
>> Therefore, as the suggestion of Christophe, I introduce a help
>> function __devm_ioremap, let devm_ioremap* inline and call
>> __devm_ioremap with different devm_ioremap_type.
>>
>> After apply the patch, the size of devres.o can be reduce from
>> 8216 Bytes to 8052 Bytes in my compile environment.
>>
>> Suggested-by: Greg KH <gregkh@...uxfoundation.org>
>> Suggested-by: Christophe LEROY <christophe.leroy@....fr>
>> Signed-off-by: Yisheng Xie <xieyisheng1@...wei.com>
>> ---
>> v2:
>>   - use MARCO for ioremap
>> v3:
>>   - kill dev_ioremap_nocache
>> v4:
>>   - combine function devm_ioremap*       - per Christophe
>> v5:
>>   - fix code style.                      - per Christophe
>> v6:
>>   - just put the cleanup in the devres.c - per Greg
>>
>>   lib/devres.c | 78 +++++++++++++++++++++++++++++-------------------------------
>>   1 file changed, 38 insertions(+), 40 deletions(-)
>>
>> diff --git a/lib/devres.c b/lib/devres.c
>> index 5f2aedd..5bec112 100644
>> --- a/lib/devres.c
>> +++ b/lib/devres.c
>> @@ -5,6 +5,12 @@
>>   #include <linux/gfp.h>
>>   #include <linux/export.h>
>>   
>> +enum devm_ioremap_type {
>> +	DEVM_IOREMAP = 0,
>> +	DEVM_IOREMAP_NC,
>> +	DEVM_IOREMAP_WC,
>> +};
>> +
>>   void devm_ioremap_release(struct device *dev, void *res)
>>   {
>>   	iounmap(*(void __iomem **)res);
>> @@ -15,24 +21,28 @@ static int devm_ioremap_match(struct device *dev, void *res, void *match_data)
>>   	return *(void **)res == match_data;
>>   }
>>   
>> -/**
>> - * devm_ioremap - Managed ioremap()
>> - * @dev: Generic device to remap IO address for
>> - * @offset: Resource address to map
>> - * @size: Size of map
>> - *
>> - * Managed ioremap().  Map is automatically unmapped on driver detach.
>> - */
>> -void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>> -			   resource_size_t size)
>> +static void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
>> +				    resource_size_t size,
>> +				    enum devm_ioremap_type type)
>>   {
>> -	void __iomem **ptr, *addr;
>> +	void __iomem **ptr, *addr = NULL;
>>   
>>   	ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
>>   	if (!ptr)
>>   		return NULL;
>>   
>> -	addr = ioremap(offset, size);
>> +	switch (type) {
>> +	case DEVM_IOREMAP:
>> +		addr = ioremap(offset, size);
>> +		break;
>> +	case DEVM_IOREMAP_NC:
>> +		addr = ioremap_nocache(offset, size);
>> +		break;
>> +	case DEVM_IOREMAP_WC:
>> +		addr = ioremap_wc(offset, size);
>> +		break;
>> +	}
>> +
>>   	if (addr) {
>>   		*ptr = addr;
>>   		devres_add(dev, ptr);
>> @@ -41,6 +51,20 @@ void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>>   
>>   	return addr;
>>   }
>> +
>> +/**
>> + * devm_ioremap - Managed ioremap()
>> + * @dev: Generic device to remap IO address for
>> + * @offset: Resource address to map
>> + * @size: Size of map
>> + *
>> + * Managed ioremap().  Map is automatically unmapped on driver detach.
>> + */
>> +void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>> +			   resource_size_t size)
>> +{
>> +	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP);
>> +}
>>   EXPORT_SYMBOL(devm_ioremap);
>>   
>>   /**
>> @@ -55,20 +79,7 @@ void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>>   void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
>>   				   resource_size_t size)
>>   {
>> -	void __iomem **ptr, *addr;
>> -
>> -	ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
>> -	if (!ptr)
>> -		return NULL;
>> -
>> -	addr = ioremap_nocache(offset, size);
>> -	if (addr) {
>> -		*ptr = addr;
>> -		devres_add(dev, ptr);
>> -	} else
>> -		devres_free(ptr);
>> -
>> -	return addr;
>> +	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_NC);
>>   }
>>   EXPORT_SYMBOL(devm_ioremap_nocache);
>>   
>> @@ -83,20 +94,7 @@ void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
>>   void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
>>   			      resource_size_t size)
>>   {
>> -	void __iomem **ptr, *addr;
>> -
>> -	ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
>> -	if (!ptr)
>> -		return NULL;
>> -
>> -	addr = ioremap_wc(offset, size);
>> -	if (addr) {
>> -		*ptr = addr;
>> -		devres_add(dev, ptr);
>> -	} else
>> -		devres_free(ptr);
>> -
>> -	return addr;
>> +	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_WC);
>>   }
>>   EXPORT_SYMBOL(devm_ioremap_wc);
>>   
>>