linux-kernel - Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap

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]

Message-ID: <53C7905D.9090003@gmail.com>
Date:	Thu, 17 Jul 2014 16:59:09 +0800
From:	Chen Gang <gang.chen.5i5j@...il.com>
To:	Thierry Reding <thierry.reding@...il.com>
CC:	Guenter Roeck <linux@...ck-us.net>,
	Richard Weinberger <richard@....at>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	dmitry.torokhov@...il.com, linux-iio@...r.kernel.org,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>, teg@...m.no,
	Lennox Wu <lennox.wu@...il.com>, Marek Vasut <marex@...x.de>,
	Liqin Chen <liqin.linux@...il.com>, msalter@...hat.com,
	linux-pwm@...r.kernel.org, devel@...verdev.osuosl.org,
	linux-watchdog@...r.kernel.org, linux-input@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	knaack.h@....de, Martin Schwidefsky <schwidefsky@...ibm.com>,
	Mischa.Jonker@...opsys.com, jic23@...nel.org, arnd@...db.de,
	Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for
 'devm_ioremap_resource'



On 07/17/2014 04:37 PM, Thierry Reding wrote:
> On Thu, Jul 17, 2014 at 09:27:58AM +0800, Chen Gang wrote:
> [...]
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index c2421e0..a7500c3 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -630,7 +630,16 @@ extern unsigned long devm_get_free_pages(struct device *dev,
>>  					 gfp_t gfp_mask, unsigned int order);
>>  extern void devm_free_pages(struct device *dev, unsigned long addr);
>>  
>> +#ifdef CONFIG_HAS_IOMEM
>>  void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
>> +#elif defined(CONFIG_COMPILE_TEST)
>> +static inline void __iomem *devm_ioremap_resource(struct device *dev,
>> +						struct resource *res)
>> +{
>> +	pr_warn("no hardware io memory, only for COMPILE_TEST\n");
> 
> Maybe: "Hardware doesn't support memory-mapped I/O"? I'm not sure if
> it's useful to include the reference to COMPILE_TEST, especially since
> the #elif will be dropped in favour of a simple #else.
> 

OK, thanks. I will use the comments which you provide.

And also use #else instead of #elif, use 'dev_warn' instead of 'pr_warn'
which Guenter suggests.

>> +	return (__force void __iomem *)ERR_PTR(-ENXIO);
> 
> There's apparently an IOMEM_ERR_PTR() for this nowadays...
> 

IOMEM_ERR_PTR() is defined within "lib/devres.c", not in "./include".
But may we move it from "lib/devres.c" to "./include/linux/err.h"?

For me, I am not quite sure, it may need additional discussion, but at
least, that will be another patch.

>> +}
>> +#endif /* CONFIG_HAS_IOMEM || CONFIG_COMPILE_TEST */
>>  
>>  /* allows to add/remove a custom action to devres stack */
>>  int devm_add_action(struct device *dev, void (*action)(void *), void *data);
>> diff --git a/include/linux/io.h b/include/linux/io.h
>> index b76e6e5..59128aa 100644
>> --- a/include/linux/io.h
>> +++ b/include/linux/io.h
>> @@ -58,14 +58,42 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr)
>>  }
>>  #endif
>>  
>> +#ifdef CONFIG_HAS_IOMEM
>> +
>>  void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
>>  			    unsigned long size);
>>  void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
>>  				    unsigned long size);
>>  void devm_iounmap(struct device *dev, void __iomem *addr);
>> +void devm_ioremap_release(struct device *dev, void *res);
>> +
>> +#elif defined(CONFIG_COMPILE_TEST)
>> +
>> +static inline void __iomem *devm_ioremap(struct device *dev,
>> +				resource_size_t offset, unsigned long size)
> 
> For consistency with other functions above, please keep arguments on
> subsequent lines aligned with the first argument on the first line, like
> so:
> 
> static inline void __iomem *devm_ioremap(struct device *dev,
> 					 resource_size_t offset,
> 					 unsigned long size)
> 

That sounds fine to me, thanks.

>> +{
>> +	pr_warn("no hardware io memory, only for COMPILE_TEST\n");
>> +	return NULL;
>> +}
>> +static inline void __iomem *devm_ioremap_nocache(struct device *dev,
>> +				resource_size_t offset, unsigned long size)
>> +{
>> +	pr_warn("no hardware io memory, only for COMPILE_TEST\n");
>> +	return NULL;
>> +}
> 
> Perhaps this should call devm_ioremap() so we don't have to repeat the
> same error message? Or maybe make it:
> 
> 	#define devm_ioremap_nocache devm_ioremap
> 

That sounds fine to me, thanks.

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed
--
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/