Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'

[Guenter Roeck <linux@roeck-us.net>]

On 07/16/2014 06:27 PM, Chen Gang wrote:
>
>
> On 07/15/2014 10:38 PM, Chen Gang wrote:
>> On 07/15/2014 09:11 AM, Chen Gang wrote:
>>>
>>>
>>> On 07/15/2014 08:53 AM, Guenter Roeck wrote:
>>>> On 07/14/2014 05:34 PM, Chen Gang wrote:
>>>>> On 07/14/2014 05:22 PM, Chen Gang wrote:
>>>>>>
>>>>>> 在 2014年7月14日，下午4:57，Richard Weinberger <richard@nod.at> 写道：
>>>>>>
>>>>>>> Am 14.07.2014 10:48, schrieb Lars-Peter Clausen:
>>>>>>>> On 07/14/2014 10:31 AM, Richard Weinberger wrote:
>>>>>>>>> Am 13.07.2014 22:17, schrieb Greg Kroah-Hartman:
>>>>>>>>>> On Sun, Jul 13, 2014 at 09:33:38PM +0200, Richard Weinberger wrote:
>>>>>>>>>>> Maybe we could add COMPILE_TEST to the version string too?
>>>>>>>>>>> Just to detect such kernels fast in user bug reports...
>>>>>>>>>>
>>>>>>>>>> What kind of bug report are you going to get?
>>>>>>>>>
>>>>>>>>> User manages to enable CONFIG_FOO by selecting COMPILE_TEST and
>>>>>>>>> complains that it does not work. :)
>>>>>>>>
>>>>>>>> These drivers are typically drivers for some SoC peripheral and the
>>>>>>>> device will simply physically not exist on a platform that does not
>>>>>>>> provide HAS_IOMEM. This is not really any
>>>>>>>> different from making the driver selectable via COMPILE_TEST for
>>>>>>>> any other platform. To hit the issue you'd have to instantiate a
>>>>>>>> device driver instance for a device that
>>>>>>>> physically does not exist. This will always result in a failure.
>>>>>>>
>>>>>>> Okay, you have convinced me. :)
>>>>>>>
>>>>>
>>>>> After search the history patches, I found one related patch which made
>>>>> by myself (when I am in Asianux):
>>>>>
>>>>>     "https://lkml.org/lkml/2013/7/1/641"
>>>>>
>>>>> For me, it is a long discussion, and forced many members have to join
>>>>> in. Please help check again.
>>>>>
>>>>
>>>> One thing you could try would be to return NULL (or where appropriate
>>>> an error) in the #else case of CONFIG_HAS_IOMEM and CONFIG_HAS_IOPORT,
>>>> ie dont take COMPILE_TEST into account at all. Obviously that means
>>>> you won't be able to dump a warning message in the COMPILE_TEST
>>>> case, but at least the code would compile. The rejection of above patch
>>>> would make a good case for this approach.
>>>>
>
> For me, only let 'devm_io*map*' support COMPILE_TEST is OK, that can fix
> all related issues:
>
>
> [PATCH] lib: devres: Add dumy functions to support COMPILE_TEST when no IOMEM
>
> For some architectures which no IOMEM, 'devres' will be skipped. But
> many drivers may still want COMPILE_TEST, so let 'devres' support it.
>
> The related error (with allmodconfig under score):
>
>      MODPOST 1365 modules
>    ERROR: "devm_ioremap_resource" [drivers/watchdog/tegra_wdt.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/watchdog/of_xilinx_wdt.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/staging/iio/adc/mxs-lradc.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/pwm/pwm-clps711x.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/input/serio/olpc_apsp.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/input/serio/arc_ps2.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/rtc/rtc-xgene.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/rtc/rtc-stk17ta8.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/rtc/rtc-ds1742.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/rtc/rtc-ds1553.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/rtc/rtc-ds1511.ko] undefined!
>    ERROR: "devm_ioremap_resource" [drivers/rtc/rtc-ds1286.ko] undefined!
>    ERROR: "devm_ioremap" [drivers/rtc/rtc-rp5c01.ko] undefined!
>    ERROR: "devm_ioremap" [drivers/rtc/rtc-msm6242.ko] undefined!
>    ERROR: "devm_ioremap" [drivers/rtc/rtc-m48t59.ko] undefined!
>    ERROR: "devm_ioremap" [drivers/rtc/rtc-m48t35.ko] undefined!
>    ERROR: "devm_ioremap" [drivers/rtc/rtc-bq4802.ko] undefined!
>
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>   include/linux/device.h |  9 +++++++++
>   include/linux/io.h     | 30 +++++++++++++++++++++++++++++-
>   2 files changed, 38 insertions(+), 1 deletion(-)
>
> 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)

I would make it #else

> +static inline void __iomem *devm_ioremap_resource(struct device *dev,
> +						struct resource *res)
> +{
> +	pr_warn("no hardware io memory, only for COMPILE_TEST\n");

dev_warn

> +	return (__force void __iomem *)ERR_PTR(-ENXIO);
> +}
> +#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)
> +

Same as above - I would suggest to use #else.

> +static inline void __iomem *devm_ioremap(struct device *dev,
> +				resource_size_t offset, unsigned long size)
> +{
> +	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");

dev_warn

Guenter

> +	return NULL;
> +}
> +
> +static inline void devm_iounmap(struct device *dev, void __iomem *addr)
> +{
> +}
> +
> +static inline void devm_ioremap_release(struct device *dev, void *res)
> +{
> +}
> +
> +#endif /* CONFIG_HAS_IOMEM || CONFIG_COMPILE_TEST */
> +
>   int check_signature(const volatile void __iomem *io_addr,
>   			const unsigned char *signature, int length);
> -void devm_ioremap_release(struct device *dev, void *res);
>
>   /*
>    * Some systems do not have legacy ISA devices.
>