About commit "io: change inX() to have their own IO barrier overrides"

About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

Hi Sinan,

About the commit in the $subject 87fe2d543f81, would there be any 
specific reason why the logic pio versions of these functions did not 
get the same treatment or should not? I'm talking about lib/logic_pio.c 
here - commit 031e3601869c ("lib: Add generic PIO mapping method") 
introduced this.

In fact, logic pio will override these for arm64 with the vanilla 
defconfig these days.

It seems that your change was made just after that logic pio stuff went 
into the kernel.

Thanks,
John

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Sinan Kaya]

Hi John,

On 2/28/2020 4:52 AM, John Garry wrote:
> About the commit in the $subject 87fe2d543f81, would there be any
> specific reason why the logic pio versions of these functions did not
> get the same treatment or should not? I'm talking about lib/logic_pio.c
> here - commit 031e3601869c ("lib: Add generic PIO mapping method")
> introduced this.
> 
> In fact, logic pio will override these for arm64 with the vanilla
> defconfig these days.

We only looked at inX()/inY() and readX()/writeX() API because the
semantics of these API are defined in the kernel documentation.
We looked at how to generalize this so that there is a uniform
behavior across different architectures.

Is logic PIO subject to ordering issues?
How is the behavior on different architectures?

As long as the expectations are set, I see no reason why it shouldn't
but, I'll let Arnd comment on it too.

Sinan

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

Hi Sinan,

Thanks for getting back to me.

> On 2/28/2020 4:52 AM, John Garry wrote:
>> About the commit in the $subject 87fe2d543f81, would there be any
>> specific reason why the logic pio versions of these functions did not
>> get the same treatment 

In fact, your changes and the logic PIO changes went in at the same time.

or should not? I'm talking about lib/logic_pio.c
>> here - commit 031e3601869c ("lib: Add generic PIO mapping method")
>> introduced this.
>>
>> In fact, logic pio will override these for arm64 with the vanilla
>> defconfig these days.
> 
> We only looked at inX()/inY() and readX()/writeX() API because the
> semantics of these API are defined in the kernel documentation.

Could we consider adding __io_pbr() et al to the kernel Documentation? I 
couldn't find them and I had to rely on checking 64e2c67738 ("io: define 
several IO & PIO barrier types for the asm-generic version") commit 
message to find the definition.

> We looked at how to generalize this so that there is a uniform
> behavior across different architectures.
> 
> Is logic PIO subject to ordering issues?

Well the point is that we're still concerned here with using 
readX/writeX for MMIO-based IO port accesses, see *** from logic_pio.c:

#define BUILD_LOGIC_IO(bw, type)					
type logic_in##bw(unsigned long addr)					
{									
	type ret = (type)~0;						
	if (addr < MMIO_UPPER_LIMIT) {					
		ret = read##bw(PCI_IOBASE + addr); ***	
	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {
		struct logic_pio_hwaddr *entry = find_io_range(addr);	
									
		if (entry)						
			ret = entry->ops->in(entry->hostdata,		
					addr, sizeof(type));		
		else							
			WARN_ON_ONCE(1);				
	}								
	return ret;							
}		

 > How is the behavior on different architectures?

So today only ARM64 uses it for this relevant code, above. But maybe 
others in future will want to use it - any arch without native IO port 
access is a candidate.

> 
> As long as the expectations are set, I see no reason why it shouldn't
> but, I'll let Arnd comment on it too.

ok, so it looks reasonable consider replicating your change for ***, above.

Thanks,
John

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Sinan Kaya]

On 3/2/2020 7:35 AM, John Garry wrote:
> Hi Sinan,
> 
> Thanks for getting back to me.
> 
>> On 2/28/2020 4:52 AM, John Garry wrote:
>>> About the commit in the $subject 87fe2d543f81, would there be any
>>> specific reason why the logic pio versions of these functions did not
>>> get the same treatment 
> 
> In fact, your changes and the logic PIO changes went in at the same time.
> 
> or should not? I'm talking about lib/logic_pio.c

I think your change missed "cross-architecture" category.

> 
> #define BUILD_LOGIC_IO(bw, type)                   
> type logic_in##bw(unsigned long addr)                   
> {                                   
>     type ret = (type)~0;                       
>     if (addr < MMIO_UPPER_LIMIT) {                   
>         ret = read##bw(PCI_IOBASE + addr); ***   
>     } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {
>         struct logic_pio_hwaddr *entry = find_io_range(addr);   
>                                    
>         if (entry)                       
>             ret = entry->ops->in(entry->hostdata,       
>                     addr, sizeof(type));       
>         else                           
>             WARN_ON_ONCE(1);               
>     }                               
>     return ret;                           
> }       
> 
>> How is the behavior on different architectures?
> 
> So today only ARM64 uses it for this relevant code, above. But maybe
> others in future will want to use it - any arch without native IO port
> access is a candidate.

I'm looking at Arnd here for help.

> 
>>
>> As long as the expectations are set, I see no reason why it shouldn't
>> but, I'll let Arnd comment on it too.
> 
> ok, so it looks reasonable consider replicating your change for ***, above.

Arnd is the maintainer here. We should consult first.
I believe there is also a linux-arch mailing list. Going there with this
question makes sense IMO.


> 
> Thanks,
> John

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

+ linux-arch

For background, see 
https://lore.kernel.org/lkml/2e80d7bc-32a0-cc40-00a9-8a383a1966c2@huawei.com/

>>
>> So today only ARM64 uses it for this relevant code, above. But maybe
>> others in future will want to use it - any arch without native IO port
>> access is a candidate.
> 
> I'm looking at Arnd here for help.
> 
>>
>>>
>>> As long as the expectations are set, I see no reason why it shouldn't
>>> but, I'll let Arnd comment on it too.
>>
>> ok, so it looks reasonable consider replicating your change for ***, above.

To be clear, I would make this change in lib/logic_pio.c since 
__io_pbr() can be overridden per-arch:

  #define BUILD_LOGIC_IO(bw, type)
  type logic_in##bw(unsigned long addr)
  {
       type ret = (type)~0;
       if (addr < MMIO_UPPER_LIMIT) {
-          ret = read##bw(PCI_IOBASE + addr);
+          __io_pbr();
+          ret = __raw_read##bw(PCI_IOBASE + addr);
+          __io_pbr();	
       } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {
           struct logic_pio_hwaddr *entry = find_io_range(addr);

...

(forgetting leX_to_cpu for the moment)

> 
> Arnd is the maintainer here. We should consult first.

ok, fine.

> I believe there is also a linux-arch mailing list. Going there with this
> question makes sense IMO.

Cheers,
John

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Arnd Bergmann]

On Tue, Mar 3, 2020 at 2:18 PM John Garry <john.garry@huawei.com> wrote:
>
> + linux-arch
>
> For background, see
> https://lore.kernel.org/lkml/2e80d7bc-32a0-cc40-00a9-8a383a1966c2@huawei.com/
>
> >>
> >> So today only ARM64 uses it for this relevant code, above. But maybe
> >> others in future will want to use it - any arch without native IO port
> >> access is a candidate.
> >
> > I'm looking at Arnd here for help.
> >
> >>
> >>>
> >>> As long as the expectations are set, I see no reason why it shouldn't
> >>> but, I'll let Arnd comment on it too.
> >>
> >> ok, so it looks reasonable consider replicating your change for ***, above.
>
> To be clear, I would make this change in lib/logic_pio.c since
> __io_pbr() can be overridden per-arch:
>
>   #define BUILD_LOGIC_IO(bw, type)
>   type logic_in##bw(unsigned long addr)
>   {
>        type ret = (type)~0;
>        if (addr < MMIO_UPPER_LIMIT) {
> -          ret = read##bw(PCI_IOBASE + addr);
> +          __io_pbr();
> +          ret = __raw_read##bw(PCI_IOBASE + addr);
> +          __io_pbr();

__io_par();

>        } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {
>            struct logic_pio_hwaddr *entry = find_io_range(addr);
>
> ...
>
> (forgetting leX_to_cpu for the moment)

Yes, I suppose this is required to get consistent behavior on arm64,
which overrides __io_par() but not __io_ar(), with the current code
the barrier after read is weaker when LOGIC_PIO is enabled than it
is otherwise.

For other architectures, I suppose we would need another indirection
level, as those can also override the default inb() itself to do something
other than readb(PCI_IOBASE + addr), and that is not handled
here either. We can do that if we need LOGIC_PIO on a second
architecture.

       Arnd

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

On 03/03/2020 16:40, Arnd Bergmann wrote:
> On Tue, Mar 3, 2020 at 2:18 PM John Garry <john.garry@huawei.com> wrote:
>>
>> + linux-arch
>>
>> For background, see
>> https://lore.kernel.org/lkml/2e80d7bc-32a0-cc40-00a9-8a383a1966c2@huawei.com/
>>
>>>>
>>>> So today only ARM64 uses it for this relevant code, above. But maybe
>>>> others in future will want to use it - any arch without native IO port
>>>> access is a candidate.
>>>
>>> I'm looking at Arnd here for help.
>>>
>>>>
>>>>>
>>>>> As long as the expectations are set, I see no reason why it shouldn't
>>>>> but, I'll let Arnd comment on it too.
>>>>
>>>> ok, so it looks reasonable consider replicating your change for ***, above.
>>
>> To be clear, I would make this change in lib/logic_pio.c since
>> __io_pbr() can be overridden per-arch:
>>
>>    #define BUILD_LOGIC_IO(bw, type)
>>    type logic_in##bw(unsigned long addr)
>>    {
>>         type ret = (type)~0;
>>         if (addr < MMIO_UPPER_LIMIT) {
>> -          ret = read##bw(PCI_IOBASE + addr);
>> +          __io_pbr();
>> +          ret = __raw_read##bw(PCI_IOBASE + addr);
>> +          __io_pbr();
> 
> __io_par();
> 
>>         } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {
>>             struct logic_pio_hwaddr *entry = find_io_range(addr);
>>
>> ...
>>
>> (forgetting leX_to_cpu for the moment)
> 
> Yes, I suppose this is required to get consistent behavior on arm64,
> which overrides __io_par() but not __io_ar(), with the current code
> the barrier after read is weaker when LOGIC_PIO is enabled than it
> is otherwise.

Ok.

Apart from that, this code is somewhat hidden. I mean, most people would 
consider generic IO port accessors come from asm-generic/io.h only, 
which is not the case here. Maybe this can be better integrated into 
asm-generic/io.h, the only hint today being the logic_pio.h include half 
way through the file.

> 
> For other architectures, I suppose we would need another indirection
> level, as those can also override the default inb() itself to do something
> other than readb(PCI_IOBASE + addr), and that is not handled
> here either. We can do that if we need LOGIC_PIO on a second
> architecture.

Jiaxun Yang did mention that MIPS may want to move away from its own IO 
space management.

Thanks,
John

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Sinan Kaya]

On 3/3/2020 11:40 AM, Arnd Bergmann wrote:
>> -          ret = read##bw(PCI_IOBASE + addr);
>> +          __io_pbr();
>> +          ret = __raw_read##bw(PCI_IOBASE + addr);
>> +          __io_pbr();
> __io_par();
> 

Why do we need to change read##bw above?

read##bw already provides strong ordering guarantees across multiple
architectures.

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Arnd Bergmann]

On Fri, Mar 6, 2020 at 4:44 AM Sinan Kaya <okaya@kernel.org> wrote:
>
> On 3/3/2020 11:40 AM, Arnd Bergmann wrote:
> >> -          ret = read##bw(PCI_IOBASE + addr);
> >> +          __io_pbr();
> >> +          ret = __raw_read##bw(PCI_IOBASE + addr);
> >> +          __io_pbr();
> > __io_par();
> >
>
> Why do we need to change read##bw above?
>
> read##bw already provides strong ordering guarantees across multiple
> architectures.

The exact semantics of inl() and readl() are slightly different, so they
have distinct sets of barriers in the asm-generic/io.h implementation.

For instance, the arm64 architectures defines in_par() as '__iormb(v)',
but defines __io_ar() as a  '__rmb()'. Similarly, riscv defines them
as "fence i,ior" and "fence i,r".

You could argue that the definitions are wrong (I have not checked the
history of the definitions), but as long as the inb() in asm-generic/io.h
uses those, the implementation in lib/logic_pio.c uses the same ones
to make the two behave the same way.

       Arnd

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

On 06/03/2020 07:54, Arnd Bergmann wrote:
> On Fri, Mar 6, 2020 at 4:44 AM Sinan Kaya <okaya@kernel.org> wrote:
>>
>> On 3/3/2020 11:40 AM, Arnd Bergmann wrote:
>>>> -          ret = read##bw(PCI_IOBASE + addr);
>>>> +          __io_pbr();
>>>> +          ret = __raw_read##bw(PCI_IOBASE + addr);
>>>> +          __io_pbr();
>>> __io_par();
>>>
>>
>> Why do we need to change read##bw above?
>>
>> read##bw already provides strong ordering guarantees across multiple
>> architectures.
> 
> The exact semantics of inl() and readl() are slightly different, so they
> have distinct sets of barriers in the asm-generic/io.h implementation.
> 
> For instance, the arm64 architectures defines in_par() as '__iormb(v)',
> but defines __io_ar() as a  '__rmb()'. Similarly, riscv defines them
> as "fence i,ior" and "fence i,r".
> 
> You could argue that the definitions are wrong (I have not checked the
> history of the definitions), but as long as the inb() in asm-generic/io.h
> uses those, the implementation in lib/logic_pio.c uses the same ones
> to make the two behave the same way.
> 

So the change would look like:

-- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -229,13 +229,21 @@ unsigned long 
logic_pio_trans_cpuaddr(resource_size_t addr)
  }

  #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
+
+#define logic_in_to_cpu_b(x) (x)
+#define logic_in_to_cpu_w(x) __le16_to_cpu(x)
+#define logic_in_to_cpu_l(x) __le32_to_cpu(x)
+
  #define BUILD_LOGIC_IO(bw, type)                                      \
  type logic_in##bw(unsigned long addr)                                 \
  {                                                                     \
         type ret = (type)~0;                                           \
                                                                        \
         if (addr < MMIO_UPPER_LIMIT) {                                 \
-               ret = read##bw(PCI_IOBASE + addr);                     \
+               void __iomem *_addr = PCI_IOBASE + addr;               \
+               __io_pbr();                                            \
+               ret = logic_in_to_cpu_##bw(__raw_read##bw(_addr));     \
+               __io_par(ret);                                         \
         } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {\
                 struct logic_pio_hwaddr *entry = find_io_rang

We could prob combine the le_to_cpu and __raw_read into a single macro.

John

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Arnd Bergmann]

On Fri, Mar 6, 2020 at 11:40 AM John Garry <john.garry@huawei.com> wrote:
> On 06/03/2020 07:54, Arnd Bergmann wrote:
> > On Fri, Mar 6, 2020 at 4:44 AM Sinan Kaya <okaya@kernel.org> wrote:
> -- a/lib/logic_pio.c
> +++ b/lib/logic_pio.c
> @@ -229,13 +229,21 @@ unsigned long
> logic_pio_trans_cpuaddr(resource_size_t addr)
>   }
>
>   #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
> +
> +#define logic_in_to_cpu_b(x) (x)
> +#define logic_in_to_cpu_w(x) __le16_to_cpu(x)
> +#define logic_in_to_cpu_l(x) __le32_to_cpu(x)
> +
>   #define BUILD_LOGIC_IO(bw, type)                                      \
>   type logic_in##bw(unsigned long addr)                                 \
>   {                                                                     \
>          type ret = (type)~0;                                           \
>                                                                         \
>          if (addr < MMIO_UPPER_LIMIT) {                                 \
> -               ret = read##bw(PCI_IOBASE + addr);                     \
> +               void __iomem *_addr = PCI_IOBASE + addr;               \
> +               __io_pbr();                                            \
> +               ret = logic_in_to_cpu_##bw(__raw_read##bw(_addr));     \
> +               __io_par(ret);                                         \
>          } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {\
>                  struct logic_pio_hwaddr *entry = find_io_rang
>
> We could prob combine the le_to_cpu and __raw_read into a single macro.

What is the purpose of splitting out the byteswap rather than leaving the
open-coded rather than __le16_to_cpu(__raw_readw(PCI_IOBASE + addr))?

If this is needed to work across architectures, how about adding
an intermediate __raw_inw() etc in asm-generic/io.h like

#ifndef __raw_inw
#define __raw_inw(a) __raw_readw(PCI_IOBASE + addr));
#endif

#include <linux/logic_pio.h>

#ifndef inw
static inline u16 inw(unsigned long addr)
{
        u16 val;

        __io_pbr();
        val = __le16_to_cpu(__raw_inw(addr));
        __io_par(val);
        return val;
}
#endif

       Arnd

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

On 06/03/2020 15:16, Arnd Bergmann wrote:
> On Fri, Mar 6, 2020 at 11:40 AM John Garry <john.garry@huawei.com> wrote:
>> On 06/03/2020 07:54, Arnd Bergmann wrote:
>>> On Fri, Mar 6, 2020 at 4:44 AM Sinan Kaya <okaya@kernel.org> wrote:
>> -- a/lib/logic_pio.c
>> +++ b/lib/logic_pio.c
>> @@ -229,13 +229,21 @@ unsigned long
>> logic_pio_trans_cpuaddr(resource_size_t addr)
>>    }
>>
>>    #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
>> +
>> +#define logic_in_to_cpu_b(x) (x)
>> +#define logic_in_to_cpu_w(x) __le16_to_cpu(x)
>> +#define logic_in_to_cpu_l(x) __le32_to_cpu(x)
>> +
>>    #define BUILD_LOGIC_IO(bw, type)                                      \

Note: The "bw" argument name could be improved to "bwl", since this 
macro is used for building inl() also.

>>    type logic_in##bw(unsigned long addr)                                 \
>>    {                                                                     \
>>           type ret = (type)~0;                                           \
>>                                                                          \
>>           if (addr < MMIO_UPPER_LIMIT) {                                 \
>> -               ret = read##bw(PCI_IOBASE + addr);                     \
>> +               void __iomem *_addr = PCI_IOBASE + addr;               \
>> +               __io_pbr();                                            \
>> +               ret = logic_in_to_cpu_##bw(__raw_read##bw(_addr));     \
>> +               __io_par(ret);                                         \
>>           } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {\
>>                   struct logic_pio_hwaddr *entry = find_io_rang
>>
>> We could prob combine the le_to_cpu and __raw_read into a single macro.
> 
> What is the purpose of splitting out the byteswap rather than leaving the
> open-coded rather than __le16_to_cpu(__raw_readw(PCI_IOBASE + addr))?

I'm just copying what is in asm-generic io.h, which uses the 16b and 32b 
byteswaps in the w and l variants, respectively.

> 
> If this is needed to work across architectures, how about adding
> an intermediate __raw_inw() etc in asm-generic/io.h like
> 
> #ifndef __raw_inw
> #define __raw_inw(a) __raw_readw(PCI_IOBASE + addr));
> #endif
> 
> #include <linux/logic_pio.h>
> 
> #ifndef inw
> static inline u16 inw(unsigned long addr)
> {
>          u16 val;
> 
>          __io_pbr();
>          val = __le16_to_cpu(__raw_inw(addr));
>          __io_par(val);
>          return val;
> }
> #endif
> 

The idea is good, but it would be nice if we just somehow use a common 
asm-generic io.h definition directly in logic_pio.c, like:

asm-generic io.h:

#ifndef __raw_inw // name?
#define __raw_inw __raw_inw
static inline u16 __raw_inw(unsigned long addr)
{
	u16 val;

	__io_pbr();
	val = __le16_to_cpu(__raw_readw(addr));
	__io_par(val);
	return val;
}
#endif

#include <linux/logic_pio.h>

#ifndef inw
#define inw __raw_inw
#endif

logic_pio.c:

  #define BUILD_LOGIC_IO(bw, type)                                      \
  type logic_in##bw(unsigned long addr)                                 \
  {                                                                     \
         type ret = (type)~0;                                           \
                                                                        \
         if (addr < MMIO_UPPER_LIMIT) {                                 \
-               ret = read##bw(PCI_IOBASE + addr);                     \
+               ret = __raw_in##bw(PCI_IOBASE + addr);                 \
         } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {\
                 struct logic_pio_hwaddr *entry = find_io_rang

Thanks,
John

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Arnd Bergmann]

On Fri, Mar 6, 2020 at 5:18 PM John Garry <john.garry@huawei.com> wrote:
> On 06/03/2020 15:16, Arnd Bergmann wrote:
> > On Fri, Mar 6, 2020 at 11:40 AM John Garry <john.garry@huawei.com> wrote:
> >> On 06/03/2020 07:54, Arnd Bergmann wrote:
> >>> On Fri, Mar 6, 2020 at 4:44 AM Sinan Kaya <okaya@kernel.org> wrote:
> >> -- a/lib/logic_pio.c
> >> +++ b/lib/logic_pio.c
> >> @@ -229,13 +229,21 @@ unsigned long
> >> logic_pio_trans_cpuaddr(resource_size_t addr)
> >>    }
> >>
> >>    #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
> >> +
> >> +#define logic_in_to_cpu_b(x) (x)
> >> +#define logic_in_to_cpu_w(x) __le16_to_cpu(x)
> >> +#define logic_in_to_cpu_l(x) __le32_to_cpu(x)
> >> +
> >>    #define BUILD_LOGIC_IO(bw, type)                                      \
>
> Note: The "bw" argument name could be improved to "bwl", since this
> macro is used for building inl() also.
>
> >>    type logic_in##bw(unsigned long addr)                                 \
> >>    {                                                                     \
> >>           type ret = (type)~0;                                           \
> >>                                                                          \
> >>           if (addr < MMIO_UPPER_LIMIT) {                                 \
> >> -               ret = read##bw(PCI_IOBASE + addr);                     \
> >> +               void __iomem *_addr = PCI_IOBASE + addr;               \
> >> +               __io_pbr();                                            \
> >> +               ret = logic_in_to_cpu_##bw(__raw_read##bw(_addr));     \
> >> +               __io_par(ret);                                         \
> >>           } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {\
> >>                   struct logic_pio_hwaddr *entry = find_io_rang
> >>
> >> We could prob combine the le_to_cpu and __raw_read into a single macro.
> >
> > What is the purpose of splitting out the byteswap rather than leaving the
> > open-coded rather than __le16_to_cpu(__raw_readw(PCI_IOBASE + addr))?
>
> I'm just copying what is in asm-generic io.h, which uses the 16b and 32b
> byteswaps in the w and l variants, respectively.

Sure, but I don't think that needs another macro.

>
> The idea is good, but it would be nice if we just somehow use a common
> asm-generic io.h definition directly in logic_pio.c, like:
>
> asm-generic io.h:
>
> #ifndef __raw_inw // name?
> #define __raw_inw __raw_inw
> static inline u16 __raw_inw(unsigned long addr)
> {
>         u16 val;
>
>         __io_pbr();
>         val = __le16_to_cpu(__raw_readw(addr));
>         __io_par(val);
>         return val;
> }
> #endif
>
> #include <linux/logic_pio.h>
>
> #ifndef inw
> #define inw __raw_inw
> #endif

Yes, makes sense. Maybe __arch_inw() then? Not great either, but I think
that's better than __raw_inw() because __raw_* would sound like it
mirrors __raw_readl() that lacks the barriers and byteswaps.

      Arnd

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

On 06/03/2020 16:29, Arnd Bergmann wrote:
>> The idea is good, but it would be nice if we just somehow use a common
>> asm-generic io.h definition directly in logic_pio.c, like:
>>
>> asm-generic io.h:
>>
>> #ifndef __raw_inw // name?
>> #define __raw_inw __raw_inw
>> static inline u16 __raw_inw(unsigned long addr)
>> {
>>          u16 val;
>>
>>          __io_pbr();
>>          val = __le16_to_cpu(__raw_readw(addr));
>>          __io_par(val);
>>          return val;
>> }
>> #endif
>>
>> #include <linux/logic_pio.h>
>>
>> #ifndef inw
>> #define inw __raw_inw
>> #endif
> Yes, makes sense. Maybe __arch_inw() then? Not great either, but I think
> that's better than __raw_inw() because __raw_* would sound like it
> mirrors __raw_readl() that lacks the barriers and byteswaps.

Right, I had the same concern. And maybe the "arch" prefix is 
misleading. Just __inw could be ok, and hopefully not conflict with the 
arch/arm/mach-* definitions.

Thanks,
John

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

On 06/03/2020 16:43, John Garry wrote:
> On 06/03/2020 16:29, Arnd Bergmann wrote:
>>> The idea is good, but it would be nice if we just somehow use a common
>>> asm-generic io.h definition directly in logic_pio.c, like:
>>>
>>> asm-generic io.h:
>>>
>>> #ifndef __raw_inw // name?
>>> #define __raw_inw __raw_inw
>>> static inline u16 __raw_inw(unsigned long addr)
>>> {
>>>          u16 val;
>>>
>>>          __io_pbr();
>>>          val = __le16_to_cpu(__raw_readw(addr));
>>>          __io_par(val);
>>>          return val;
>>> }
>>> #endif
>>>
>>> #include <linux/logic_pio.h>
>>>
>>> #ifndef inw
>>> #define inw __raw_inw
>>> #endif
>> Yes, makes sense. Maybe __arch_inw() then? Not great either, but I think
>> that's better than __raw_inw() because __raw_* would sound like it
>> mirrors __raw_readl() that lacks the barriers and byteswaps.
> 
> Right, I had the same concern. And maybe the "arch" prefix is 
> misleading. Just __inw could be ok, and hopefully not conflict with the 
> arch/arm/mach-* definitions.
> 

I think that it hasn't been mentioned already, but it looks like the 
outX methods also need the same treatment, from a7851aa54c.

thanks,
John

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Sinan Kaya]

On 3/6/2020 2:54 AM, Arnd Bergmann wrote:
> The exact semantics of inl() and readl() are slightly different, so they
> have distinct sets of barriers in the asm-generic/io.h implementation.
> 
> For instance, the arm64 architectures defines in_par() as '__iormb(v)',
> but defines __io_ar() as a  '__rmb()'. Similarly, riscv defines them
> as "fence i,ior" and "fence i,r".

makes sense