[PATCH v5] devres: combine function devm_ioremap*

[PATCH v5] devres: combine function devm_ioremap*

[Yisheng Xie]

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 7352Bytes in my compile environment.

Suggested-by: Christophe LEROY <christophe.leroy@c-s.fr>
Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
---
v2:
 - use MARCO for ioremap
v3:
 - kill dev_ioremap_nocache
v4:
 - combine function devm_ioremap*
v5:
 - fix code style.

 include/linux/io.h | 61 +++++++++++++++++++++++++++++++++++----
 lib/devres.c       | 84 ++++++++++--------------------------------------------
 2 files changed, 70 insertions(+), 75 deletions(-)

diff --git a/include/linux/io.h b/include/linux/io.h
index 32e30e8..4d0a640 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -73,12 +73,61 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr)
 
 #define IOMEM_ERR_PTR(err) (__force void __iomem *)ERR_PTR(err)
 
-void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
-			   resource_size_t size);
-void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
-				   resource_size_t size);
-void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
-				   resource_size_t size);
+enum devm_ioremap_type {
+	DEVM_IOREMAP = 0,
+	DEVM_IOREMAP_NC,
+	DEVM_IOREMAP_WC,
+};
+
+void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
+			     resource_size_t size, enum devm_ioremap_type type);
+
+/**
+ * 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.
+ */
+static inline void __iomem *devm_ioremap(struct device *dev,
+					 resource_size_t offset,
+					 resource_size_t size)
+{
+	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP);
+}
+
+/**
+ * devm_ioremap_nocache - Managed ioremap_nocache()
+ * @dev: Generic device to remap IO address for
+ * @offset: Resource address to map
+ * @size: Size of map
+ *
+ * Managed ioremap_nocache().  Map is automatically unmapped on driver
+ * detach.
+ */
+static inline void __iomem *devm_ioremap_nocache(struct device *dev,
+						 resource_size_t offset,
+						 resource_size_t size)
+{
+	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_NC);
+}
+
+/**
+ * devm_ioremap_wc - Managed ioremap_wc()
+ * @dev: Generic device to remap IO address for
+ * @offset: Resource address to map
+ * @size: Size of map
+ *
+ * Managed ioremap_wc().  Map is automatically unmapped on driver detach.
+ */
+static inline void __iomem *devm_ioremap_wc(struct device *dev,
+					    resource_size_t offset,
+					    resource_size_t size)
+{
+	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_WC);
+}
+
 void devm_iounmap(struct device *dev, void __iomem *addr);
 int check_signature(const volatile void __iomem *io_addr,
 			const unsigned char *signature, int length);
diff --git a/lib/devres.c b/lib/devres.c
index 5f2aedd..88a4520 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -15,81 +15,27 @@ 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)
+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);
-	if (addr) {
-		*ptr = addr;
-		devres_add(dev, ptr);
-	} else
-		devres_free(ptr);
-
-	return addr;
-}
-EXPORT_SYMBOL(devm_ioremap);
-
-/**
- * devm_ioremap_nocache - Managed ioremap_nocache()
- * @dev: Generic device to remap IO address for
- * @offset: Resource address to map
- * @size: Size of map
- *
- * Managed ioremap_nocache().  Map is automatically unmapped on driver
- * detach.
- */
-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;
-}
-EXPORT_SYMBOL(devm_ioremap_nocache);
-
-/**
- * devm_ioremap_wc - Managed ioremap_wc()
- * @dev: Generic device to remap IO address for
- * @offset: Resource address to map
- * @size: Size of map
- *
- * Managed ioremap_wc().  Map is automatically unmapped on driver detach.
- */
-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;
+	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;
+	}
 
-	addr = ioremap_wc(offset, size);
 	if (addr) {
 		*ptr = addr;
 		devres_add(dev, ptr);
@@ -98,7 +44,7 @@ void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
 
 	return addr;
 }
-EXPORT_SYMBOL(devm_ioremap_wc);
+EXPORT_SYMBOL(__devm_ioremap);
 
 /**
  * devm_iounmap - Managed iounmap()
-- 
1.8.3.1

Re: [PATCH v5] devres: combine function devm_ioremap*

[Christophe LEROY]

Le 16/01/2018 à 13:03, Yisheng Xie a écrit :
> 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 7352Bytes in my compile environment.
> 
> Suggested-by: Christophe LEROY <christophe.leroy@c-s.fr>
> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>

Review-by: Christophe LEROY <christophe.leroy@c-s.fr>

> ---
> v2:
>   - use MARCO for ioremap
> v3:
>   - kill dev_ioremap_nocache
> v4:
>   - combine function devm_ioremap*
> v5:
>   - fix code style.
> 
>   include/linux/io.h | 61 +++++++++++++++++++++++++++++++++++----
>   lib/devres.c       | 84 ++++++++++--------------------------------------------
>   2 files changed, 70 insertions(+), 75 deletions(-)
> 
> diff --git a/include/linux/io.h b/include/linux/io.h
> index 32e30e8..4d0a640 100644
> --- a/include/linux/io.h
> +++ b/include/linux/io.h
> @@ -73,12 +73,61 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr)
>   
>   #define IOMEM_ERR_PTR(err) (__force void __iomem *)ERR_PTR(err)
>   
> -void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> -			   resource_size_t size);
> -void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
> -				   resource_size_t size);
> -void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
> -				   resource_size_t size);
> +enum devm_ioremap_type {
> +	DEVM_IOREMAP = 0,
> +	DEVM_IOREMAP_NC,
> +	DEVM_IOREMAP_WC,
> +};
> +
> +void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
> +			     resource_size_t size, enum devm_ioremap_type type);
> +
> +/**
> + * 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.
> + */
> +static inline void __iomem *devm_ioremap(struct device *dev,
> +					 resource_size_t offset,
> +					 resource_size_t size)
> +{
> +	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP);
> +}
> +
> +/**
> + * devm_ioremap_nocache - Managed ioremap_nocache()
> + * @dev: Generic device to remap IO address for
> + * @offset: Resource address to map
> + * @size: Size of map
> + *
> + * Managed ioremap_nocache().  Map is automatically unmapped on driver
> + * detach.
> + */
> +static inline void __iomem *devm_ioremap_nocache(struct device *dev,
> +						 resource_size_t offset,
> +						 resource_size_t size)
> +{
> +	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_NC);
> +}
> +
> +/**
> + * devm_ioremap_wc - Managed ioremap_wc()
> + * @dev: Generic device to remap IO address for
> + * @offset: Resource address to map
> + * @size: Size of map
> + *
> + * Managed ioremap_wc().  Map is automatically unmapped on driver detach.
> + */
> +static inline void __iomem *devm_ioremap_wc(struct device *dev,
> +					    resource_size_t offset,
> +					    resource_size_t size)
> +{
> +	return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_WC);
> +}
> +
>   void devm_iounmap(struct device *dev, void __iomem *addr);
>   int check_signature(const volatile void __iomem *io_addr,
>   			const unsigned char *signature, int length);
> diff --git a/lib/devres.c b/lib/devres.c
> index 5f2aedd..88a4520 100644
> --- a/lib/devres.c
> +++ b/lib/devres.c
> @@ -15,81 +15,27 @@ 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)
> +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);
> -	if (addr) {
> -		*ptr = addr;
> -		devres_add(dev, ptr);
> -	} else
> -		devres_free(ptr);
> -
> -	return addr;
> -}
> -EXPORT_SYMBOL(devm_ioremap);
> -
> -/**
> - * devm_ioremap_nocache - Managed ioremap_nocache()
> - * @dev: Generic device to remap IO address for
> - * @offset: Resource address to map
> - * @size: Size of map
> - *
> - * Managed ioremap_nocache().  Map is automatically unmapped on driver
> - * detach.
> - */
> -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;
> -}
> -EXPORT_SYMBOL(devm_ioremap_nocache);
> -
> -/**
> - * devm_ioremap_wc - Managed ioremap_wc()
> - * @dev: Generic device to remap IO address for
> - * @offset: Resource address to map
> - * @size: Size of map
> - *
> - * Managed ioremap_wc().  Map is automatically unmapped on driver detach.
> - */
> -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;
> +	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;
> +	}
>   
> -	addr = ioremap_wc(offset, size);
>   	if (addr) {
>   		*ptr = addr;
>   		devres_add(dev, ptr);
> @@ -98,7 +44,7 @@ void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
>   
>   	return addr;
>   }
> -EXPORT_SYMBOL(devm_ioremap_wc);
> +EXPORT_SYMBOL(__devm_ioremap);
>   
>   /**
>    * devm_iounmap - Managed iounmap()
> 

Re: [PATCH v5] devres: combine function devm_ioremap*

[Greg KH]

On Tue, Jan 16, 2018 at 08:03:41PM +0800, 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 7352Bytes in my compile environment.
> 
> Suggested-by: Christophe LEROY <christophe.leroy@c-s.fr>
> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
> ---
> v2:
>  - use MARCO for ioremap
> v3:
>  - kill dev_ioremap_nocache
> v4:
>  - combine function devm_ioremap*
> v5:
>  - fix code style.
> 
>  include/linux/io.h | 61 +++++++++++++++++++++++++++++++++++----
>  lib/devres.c       | 84 ++++++++++--------------------------------------------
>  2 files changed, 70 insertions(+), 75 deletions(-)
> 
> diff --git a/include/linux/io.h b/include/linux/io.h
> index 32e30e8..4d0a640 100644
> --- a/include/linux/io.h
> +++ b/include/linux/io.h
> @@ -73,12 +73,61 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr)
>  
>  #define IOMEM_ERR_PTR(err) (__force void __iomem *)ERR_PTR(err)
>  
> -void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> -			   resource_size_t size);
> -void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
> -				   resource_size_t size);
> -void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
> -				   resource_size_t size);
> +enum devm_ioremap_type {
> +	DEVM_IOREMAP = 0,
> +	DEVM_IOREMAP_NC,
> +	DEVM_IOREMAP_WC,
> +};

Why do these types need to be in a public .h file?

Why not just keep the .h file as-is and then just put the cleanup in the
.c file like you did?

thanks,

greg k-h

Re: [PATCH v5] devres: combine function devm_ioremap*

[Yisheng Xie]

On 2018/1/23 16:42, Greg KH wrote:
> On Tue, Jan 16, 2018 at 08:03:41PM +0800, 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 7352Bytes in my compile environment.
>>
>> Suggested-by: Christophe LEROY <christophe.leroy@c-s.fr>
>> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
>> ---
>> v2:
>>  - use MARCO for ioremap
>> v3:
>>  - kill dev_ioremap_nocache
>> v4:
>>  - combine function devm_ioremap*
>> v5:
>>  - fix code style.
>>
>>  include/linux/io.h | 61 +++++++++++++++++++++++++++++++++++----
>>  lib/devres.c       | 84 ++++++++++--------------------------------------------
>>  2 files changed, 70 insertions(+), 75 deletions(-)
>>
>> diff --git a/include/linux/io.h b/include/linux/io.h
>> index 32e30e8..4d0a640 100644
>> --- a/include/linux/io.h
>> +++ b/include/linux/io.h
>> @@ -73,12 +73,61 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr)
>>  
>>  #define IOMEM_ERR_PTR(err) (__force void __iomem *)ERR_PTR(err)
>>  
>> -void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>> -			   resource_size_t size);
>> -void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
>> -				   resource_size_t size);
>> -void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
>> -				   resource_size_t size);
>> +enum devm_ioremap_type {
>> +	DEVM_IOREMAP = 0,
>> +	DEVM_IOREMAP_NC,
>> +	DEVM_IOREMAP_WC,
>> +};
> 
> Why do these types need to be in a public .h file?
> 
> Why not just keep the .h file as-is and then just put the cleanup in the
> .c file like you did?
> 
Right. I was just trying to inline these functions. Anyway, I
will follow your suggestion. Sorry for sending so many versions.

Thanks
Yisheng

> thanks,
> 
> greg k-h
> 
> .
> 

Re: [PATCH v5] devres: combine function devm_ioremap*

[Greg KH]

On Tue, Jan 23, 2018 at 06:50:49PM +0800, Yisheng Xie wrote:
> 
> 
> On 2018/1/23 16:42, Greg KH wrote:
> > On Tue, Jan 16, 2018 at 08:03:41PM +0800, 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 7352Bytes in my compile environment.
> >>
> >> Suggested-by: Christophe LEROY <christophe.leroy@c-s.fr>
> >> Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
> >> ---
> >> v2:
> >>  - use MARCO for ioremap
> >> v3:
> >>  - kill dev_ioremap_nocache
> >> v4:
> >>  - combine function devm_ioremap*
> >> v5:
> >>  - fix code style.
> >>
> >>  include/linux/io.h | 61 +++++++++++++++++++++++++++++++++++----
> >>  lib/devres.c       | 84 ++++++++++--------------------------------------------
> >>  2 files changed, 70 insertions(+), 75 deletions(-)
> >>
> >> diff --git a/include/linux/io.h b/include/linux/io.h
> >> index 32e30e8..4d0a640 100644
> >> --- a/include/linux/io.h
> >> +++ b/include/linux/io.h
> >> @@ -73,12 +73,61 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr)
> >>  
> >>  #define IOMEM_ERR_PTR(err) (__force void __iomem *)ERR_PTR(err)
> >>  
> >> -void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
> >> -			   resource_size_t size);
> >> -void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
> >> -				   resource_size_t size);
> >> -void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,
> >> -				   resource_size_t size);
> >> +enum devm_ioremap_type {
> >> +	DEVM_IOREMAP = 0,
> >> +	DEVM_IOREMAP_NC,
> >> +	DEVM_IOREMAP_WC,
> >> +};
> > 
> > Why do these types need to be in a public .h file?
> > 
> > Why not just keep the .h file as-is and then just put the cleanup in the
> > .c file like you did?
> > 
> Right. I was just trying to inline these functions.

Doesn't really matter much, so no real need to.

> Anyway, I will follow your suggestion. Sorry for sending so many
> versions.

Not a problem at all, keep up the great work :)

greg k-h