[PATCH] driver: have dev_platform_ioremap_resource return error pointer

[PATCH] driver: have dev_platform_ioremap_resource return error pointer

[Ahmad Fatoum]

From: Ahmad Fatoum <a.fatoum@barebox.org>

devm_request_mem_region doesn't return NULL on error on Linux and thus
it should neither in barebox. Fix this.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/base/driver.c  | 2 ++
 include/linux/device.h | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index edba037fa2dd..4e361a96c9ee 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -587,6 +587,8 @@ void __iomem *dev_request_mem_region(struct device *dev, int num)
 	struct resource *res;
 
 	res = dev_request_mem_resource(dev, num);
+	if (!IS_ERR(res) && WARN_ON(IS_ERR_VALUE(res->start)))
+		return IOMEM_ERR_PTR(res->start);
 	if (IS_ERR(res))
 		return ERR_CAST(res);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index 66294910abb3..661d8b24730e 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -39,7 +39,7 @@ static inline void __iomem *dev_platform_ioremap_resource(struct device *dev,
 	 * so we don't need to do anything besides requesting the regions
 	 * and can leave the memory attributes unchanged.
 	 */
-	return dev_request_mem_region_err_null(dev, resource);
+	return dev_request_mem_region(dev, resource);
 }
 
 static inline void __iomem *devm_ioremap(struct device *dev,
-- 
2.39.5

Re: [PATCH] driver: have dev_platform_ioremap_resource return error pointer

[Sascha Hauer]

On Mon, Apr 14, 2025 at 08:23:03AM +0200, Ahmad Fatoum wrote:
> From: Ahmad Fatoum <a.fatoum@barebox.org>
> 
> devm_request_mem_region doesn't return NULL on error on Linux and thus
> it should neither in barebox. Fix this.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/base/driver.c  | 2 ++
>  include/linux/device.h | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index edba037fa2dd..4e361a96c9ee 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -587,6 +587,8 @@ void __iomem *dev_request_mem_region(struct device *dev, int num)
>  	struct resource *res;
>  
>  	res = dev_request_mem_resource(dev, num);
> +	if (!IS_ERR(res) && WARN_ON(IS_ERR_VALUE(res->start)))
> +		return IOMEM_ERR_PTR(res->start);

Which code can set res->start to an error value?

Sascha

>  	if (IS_ERR(res))
>  		return ERR_CAST(res);
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 66294910abb3..661d8b24730e 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -39,7 +39,7 @@ static inline void __iomem *dev_platform_ioremap_resource(struct device *dev,
>  	 * so we don't need to do anything besides requesting the regions
>  	 * and can leave the memory attributes unchanged.
>  	 */
> -	return dev_request_mem_region_err_null(dev, resource);
> +	return dev_request_mem_region(dev, resource);
>  }
>  
>  static inline void __iomem *devm_ioremap(struct device *dev,
> -- 
> 2.39.5
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] driver: have dev_platform_ioremap_resource return error pointer

[Ahmad Fatoum]

Hello Sascha,

On 4/14/25 11:07, Sascha Hauer wrote:
> On Mon, Apr 14, 2025 at 08:23:03AM +0200, Ahmad Fatoum wrote:
>> From: Ahmad Fatoum <a.fatoum@barebox.org>
>>
>> devm_request_mem_region doesn't return NULL on error on Linux and thus
>> it should neither in barebox. Fix this.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  drivers/base/driver.c  | 2 ++
>>  include/linux/device.h | 2 +-
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
>> index edba037fa2dd..4e361a96c9ee 100644
>> --- a/drivers/base/driver.c
>> +++ b/drivers/base/driver.c
>> @@ -587,6 +587,8 @@ void __iomem *dev_request_mem_region(struct device *dev, int num)
>>  	struct resource *res;
>>  
>>  	res = dev_request_mem_resource(dev, num);
>> +	if (!IS_ERR(res) && WARN_ON(IS_ERR_VALUE(res->start)))
>> +		return IOMEM_ERR_PTR(res->start);
> 
> Which code can set res->start to an error value?

It can end up being an error value, because there is actual a MMIO
region at a high address. In that case, I'd rather we return an
error, so dev_request_mem_resource would need to be used instead.

Thanks,
Ahmad

> 
> Sascha
> 
>>  	if (IS_ERR(res))
>>  		return ERR_CAST(res);
>>  
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 66294910abb3..661d8b24730e 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -39,7 +39,7 @@ static inline void __iomem *dev_platform_ioremap_resource(struct device *dev,
>>  	 * so we don't need to do anything besides requesting the regions
>>  	 * and can leave the memory attributes unchanged.
>>  	 */
>> -	return dev_request_mem_region_err_null(dev, resource);
>> +	return dev_request_mem_region(dev, resource);
>>  }
>>  
>>  static inline void __iomem *devm_ioremap(struct device *dev,
>> -- 
>> 2.39.5
>>
>>
>>
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |

Re: [PATCH] driver: have dev_platform_ioremap_resource return error pointer

[Sascha Hauer]

On Mon, Apr 14, 2025 at 12:10:54PM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 4/14/25 11:07, Sascha Hauer wrote:
> > On Mon, Apr 14, 2025 at 08:23:03AM +0200, Ahmad Fatoum wrote:
> >> From: Ahmad Fatoum <a.fatoum@barebox.org>
> >>
> >> devm_request_mem_region doesn't return NULL on error on Linux and thus
> >> it should neither in barebox. Fix this.
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >>  drivers/base/driver.c  | 2 ++
> >>  include/linux/device.h | 2 +-
> >>  2 files changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> >> index edba037fa2dd..4e361a96c9ee 100644
> >> --- a/drivers/base/driver.c
> >> +++ b/drivers/base/driver.c
> >> @@ -587,6 +587,8 @@ void __iomem *dev_request_mem_region(struct device *dev, int num)
> >>  	struct resource *res;
> >>  
> >>  	res = dev_request_mem_resource(dev, num);
> >> +	if (!IS_ERR(res) && WARN_ON(IS_ERR_VALUE(res->start)))
> >> +		return IOMEM_ERR_PTR(res->start);
> > 
> > Which code can set res->start to an error value?
> 
> It can end up being an error value, because there is actual a MMIO
> region at a high address. In that case, I'd rather we return an
> error, so dev_request_mem_resource would need to be used instead.

Ok, so you don't expect code to intentionally set the resource start to
an error value, but instead the resource start actually is in a range
that is interpreted as an error value (i.e. >= 0x100000000 - MAX_ERRNO
aka the last page in the 32bit address space).

Can we add a comment here to clarify this?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |