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

[Hitoshi Mitake]

On Sun, May 17, 2009 at 17:06, Jeff Garzik <jeff@xxxxxxxxxx> wrote:
> Hitoshi Mitake wrote:
>>
>> On Fri, 15 May 2009 19:44:03 -0400
>> Jeff Garzik <jeff@xxxxxxxxxx> wrote:
>>
>>> Hitoshi Mitake wrote:
>>>>
>>>> On Wed, 13 May 2009 20:49:26 -0400
>>>> Jeff Garzik <jeff@xxxxxxxxxx> 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@xxxxxxxxx>
>>>>
>>>> ---
>>>>  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@xxxxxxxxx>
>>
>> ---
>>  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
>
>
>

Please discard this mail, this is only Ccing to my another email
address of university.
Because IMAP of Gmail is too slow to read LKML...
So I'll switch my email address from h.mitake@xxxxxxxxx to
mitake@xxxxxxxxxxxxxxxxxxxxx,
and reply from new one.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/