Re: [PATCH] x86: Remove readq()/writeq() on 32-bit

[Jeff Garzik <jeff@garzik.org>]

Hitoshi Mitake wrote:
> On Fri, 15 May 2009 19:44:03 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> Hitoshi Mitake wrote:
>>> On Wed, 13 May 2009 20:49:26 -0400
>>> Jeff Garzik <jeff@garzik.org> wrote:
>>>
>>>> H. Peter Anvin wrote:
>>>>> Jeff Garzik wrote:
>>>>>> Judging from this thread and past, I think people will continue to 
>>>>>> complain and get confused, even with the above.
>>>>>>
>>>>> Do you really think so?  Seems unfortunate, since an API rename would be
>>>>> way more invasive.  This is the entirety of the header patch
>>>>> (compile-tested using 32-bit allyesconfig).
>>>> The header patch does not lessen the confusion, because you cannot look 
>>>> at the code and immediately tell what is going on...
>>>>
>>>> Having a single function's behavior change based on #include selection 
>>>> is /not/ intuitive at all, particularly for driver writers.  That is 
>>>> unlike almost every other Linux API, where functions' behavior stays 
>>>> constant across platforms, regardless of magic "under the hood."
>>>>
>>>> That sort of trick is reserved for arch maintainers who know what they 
>>>> are doing :)
>>>>
>>>> 	Jeff
>>>>
>>>>
>>>>
>>> I found another way:
>>> Making architecture with atomic readq/writeq provide HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC
>>> and making architecture with non-atomic readq/writeq provide HAVE_READQ/HAVE_WRITEQ.
>>> (HAVE_READQ_ATOMIC/HAVE_WRITEQ_ATOMIC should double as HAVE_READQ/HAVE_WRITEQ.)
>>>
>>> So driver programmers who need atomic readq/writeq can judge existence of API they really need.
>>> If platform doesn't provide atomic readq/writeq, drivers need these can be disabled by Kconfig.
>>> And bugs Roland and David talking about will be banished.
>>> How about this? > Roland and David
>>> I wrote a test patch. Request for comments.
>>>
>>> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
>>>
>>> ---
>>>  arch/x86/Kconfig |   16 ++++++++++++++--
>>>  1 files changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>> index df9e885..c94fc48 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -19,8 +19,6 @@ config X86_64
>>>  config X86
>>>  	def_bool y
>>>  	select HAVE_AOUT if X86_32
>>> -	select HAVE_READQ
>>> -	select HAVE_WRITEQ
>>>  	select HAVE_UNSTABLE_SCHED_CLOCK
>>>  	select HAVE_IDE
>>>  	select HAVE_OPROFILE
>>> @@ -2022,6 +2020,20 @@ config HAVE_ATOMIC_IOMAP
>>>  	def_bool y
>>>  	depends on X86_32
>>>  
>>> +config HAVE_READQ
>>> +	def_bool y
>>> +
>>> +config HAVE_WRITEQ
>>> +	def_bool y
>>> +
>>> +config HAVE_READQ_ATOMIC
>>> +	def_bool y
>>> +	depends on X86_64
>>> +
>>> +config HAVE_WRITEQ_ATOMIC
>>> +	def_bool y
>>> +	depends on X86_64
>> If you create HAVE_{READQ,WRITEQ}_ATOMIC, then you don't really need 
>> HAVE_READQ -- the other relevant 32-bit platforms simply need a 
>> definition of readq and writeq.  Probably easy enough to have a common 
>> definition in asm-generic.
>>
> That's good idea. I didn't noticed the way to use asm-generic. Thanks.
> 
> How is this?
> 
> Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>
> 
> ---
>  arch/x86/Kconfig             |   10 ++++++++--
>  arch/x86/include/asm/io.h    |   27 ++++++---------------------
>  include/asm-generic/quadrw.h |   34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 23 deletions(-)
>  create mode 100644 include/asm-generic/quadrw.h


Seems fine to me, no objections here...



> +#ifndef CONFIG_HAVE_READQ_ATOMIC
> +static inline __u64 readq(const volatile void __iomem *addr)
> +{
> +	const volatile u32 __iomem *p = addr;
> +	u32 low, high;
> +
> +	low = readl(p);
> +	high = readl(p + 1);
> +
> +	return low + ((u64)high << 32);
> +}
> +#endif	/* CONFIG_HAVE_READQ_ATOMIC */

Personally I would do

	static inline __u64 readq(const volatile void __iomem *addr)
	{
		__u64 low, high;

		low = readl(addr);
		high = readl(addr + 4);

		return (high << 32) | low;
	}

but maybe that's just me.

It seems inconsistent that the generic readq and writeq internals, 
simple as they are, differ to such a degree in this implementation.

But overall, as mentioned above, the approach seems sound to me.

Cheers!

	Jeff