Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Martin Schwidefsky]

On Mon, 5 Oct 2015 19:09:14 +0200
"Luis R. Rodriguez"  wrote:

> On Sun, Oct 04, 2015 at 09:02:04PM +0200, Arnd Bergmann wrote:
> > On Saturday 03 October 2015 01:53:46 Luis R. Rodriguez wrote:
> > > > 
> > > > Hmm, my gut feeling tells me that your approach won't solve the problem
> > > > in general. s390 PCI is just weird in many ways and it will occasionally
> > > > suffer from problems like this (as do other aspects of the s390 
> > > > architecture
> > > > that are unlike the rest of the world).
> > > > 
> > > > Maybe Martin and Heiko can comment on this, they may have a preference
> > > > from the s390 point of view.
> > > 
> > > Hrm, so S390 is quirky is really odd ways that no other architecture is or
> > > is at least for now not expected to be ?
> > 
> > Absolutely correct. It is the only architecture I'm aware of that tries to
> > support PCI that does not use pointer dereferences for MMIO.
> 
> So its not worth it to have a formal semantic via Kconfig for this and are
> happy with the status quo of having to find out through a bot compile test
> any changes in this domain fails?

For my part I still do not see the value in ARCH_PCI_NON_DISJUNCTIVE.
The existing GENERIC_PCI_IOMAP already allows an architecture to select
the generic implementation (or not to select it). To add another Kconfig
symbol and make GENERIC_PCI_IOMAP depend on it doesn't help.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Martin Schwidefsky]

On Mon, 5 Oct 2015 19:09:14 +0200
"Luis R. Rodriguez"  wrote:

> On Sun, Oct 04, 2015 at 09:02:04PM +0200, Arnd Bergmann wrote:
> > On Saturday 03 October 2015 01:53:46 Luis R. Rodriguez wrote:
> > > > 
> > > > Hmm, my gut feeling tells me that your approach won't solve the problem
> > > > in general. s390 PCI is just weird in many ways and it will occasionally
> > > > suffer from problems like this (as do other aspects of the s390 
> > > > architecture
> > > > that are unlike the rest of the world).
> > > > 
> > > > Maybe Martin and Heiko can comment on this, they may have a preference
> > > > from the s390 point of view.
> > > 
> > > Hrm, so S390 is quirky is really odd ways that no other architecture is or
> > > is at least for now not expected to be ?
> > 
> > Absolutely correct. It is the only architecture I'm aware of that tries to
> > support PCI that does not use pointer dereferences for MMIO.
> 
> So its not worth it to have a formal semantic via Kconfig for this and are
> happy with the status quo of having to find out through a bot compile test
> any changes in this domain fails?

For my part I still do not see the value in ARCH_PCI_NON_DISJUNCTIVE.
The existing GENERIC_PCI_IOMAP already allows an architecture to select
the generic implementation (or not to select it). To add another Kconfig
symbol and make GENERIC_PCI_IOMAP depend on it doesn't help.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Sun, Oct 04, 2015 at 09:02:04PM +0200, Arnd Bergmann wrote:
> On Saturday 03 October 2015 01:53:46 Luis R. Rodriguez wrote:
> > > 
> > > Hmm, my gut feeling tells me that your approach won't solve the problem
> > > in general. s390 PCI is just weird in many ways and it will occasionally
> > > suffer from problems like this (as do other aspects of the s390 
> > > architecture
> > > that are unlike the rest of the world).
> > > 
> > > Maybe Martin and Heiko can comment on this, they may have a preference
> > > from the s390 point of view.
> > 
> > Hrm, so S390 is quirky is really odd ways that no other architecture is or
> > is at least for now not expected to be ?
> 
> Absolutely correct. It is the only architecture I'm aware of that tries to
> support PCI that does not use pointer dereferences for MMIO.

So its not worth it to have a formal semantic via Kconfig for this and are
happy with the status quo of having to find out through a bot compile test
any changes in this domain fails?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Sat, Oct 03, 2015 at 05:14:20PM -0700, Randy Dunlap wrote:
> On 10/02/15 16:17, Luis R. Rodriguez wrote:
> > On Fri, Aug 28, 2015 at 07:13:08PM -0700, Randy Dunlap wrote:
> >> On 08/28/15 17:17, Luis R. Rodriguez wrote:
> >>>
> >>>  arch/s390/Kconfig |  8 +
> >>>  arch/s390/include/asm/io.h| 11 ---
> >>>  arch/s390/include/asm/pci.h   |  2 --
> >>>  arch/s390/include/asm/pci_iomap.h | 33 +
> >>>  arch/s390/pci/pci.c   |  2 ++
> >>>  include/asm-generic/io.h  | 12 
> >>>  include/asm-generic/iomap.h   | 10 ---
> >>>  include/asm-generic/pci_iomap.h   | 62 
> >>> +++
> >>>  lib/Kconfig   |  1 +
> >>>  lib/pci_iomap.c   |  5 
> >>>  10 files changed, 105 insertions(+), 41 deletions(-)
> >>>  create mode 100644 arch/s390/include/asm/pci_iomap.h
> >>>
> >>> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> >>> index 1d57000b1b24..1217b7db4265 100644
> >>> --- a/arch/s390/Kconfig
> >>> +++ b/arch/s390/Kconfig
> >>> @@ -614,6 +614,14 @@ endif# PCI
> >>>  config PCI_DOMAINS
> >>>   def_bool PCI
> >>>  
> >>> +config ARCH_PCI_NON_DISJUNCTIVE
> >>> + def_bool PCI
> >>> + help
> >>> +   On the S390 architecture PCI BAR spaces are not disjunctive, as such
> >>
> >>are not disjoint?  may be overlapping?
> >>
> >>> +   the PCI bar is required on a series of otherwise asm generic PCI
> >>> +   routines, as such S390 requires itw own implemention for these
> >>
> >>  its own implementation
> > 
> > Thanks, I've re-written this as:
> > 
> > mcgrof@ergon ~/linux-next (git::(no branch, rebasing 20150805-pend-all))$ 
> > git diff
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > index f4725d1af438..8ba5826ed13b 100644
> > --- a/arch/s390/Kconfig
> > +++ b/arch/s390/Kconfig
> > @@ -618,10 +618,10 @@ config PCI_DOMAINS
> >  config ARCH_PCI_NON_DISJUNCTIVE
> > def_bool PCI
> > help
> > - On the S390 architecture PCI BAR spaces are not disjunctive, as 
> > such
> > - the PCI bar is required on a series of otherwise asm generic PCI
> > - routines, as such S390 requires itw own implemention for these
> > - routines.
> > + On the S390 architecture PCI BAR spaces may overlap with each 
> > other,
> 
> That "other," should not end with a comma.  Either use a semi-colon or use a
> period and begin the next sentence with Because.

Amended, thanks.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Sun, Oct 04, 2015 at 09:02:04PM +0200, Arnd Bergmann wrote:
> On Saturday 03 October 2015 01:53:46 Luis R. Rodriguez wrote:
> > > 
> > > Hmm, my gut feeling tells me that your approach won't solve the problem
> > > in general. s390 PCI is just weird in many ways and it will occasionally
> > > suffer from problems like this (as do other aspects of the s390 
> > > architecture
> > > that are unlike the rest of the world).
> > > 
> > > Maybe Martin and Heiko can comment on this, they may have a preference
> > > from the s390 point of view.
> > 
> > Hrm, so S390 is quirky is really odd ways that no other architecture is or
> > is at least for now not expected to be ?
> 
> Absolutely correct. It is the only architecture I'm aware of that tries to
> support PCI that does not use pointer dereferences for MMIO.

So its not worth it to have a formal semantic via Kconfig for this and are
happy with the status quo of having to find out through a bot compile test
any changes in this domain fails?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Sat, Oct 03, 2015 at 05:14:20PM -0700, Randy Dunlap wrote:
> On 10/02/15 16:17, Luis R. Rodriguez wrote:
> > On Fri, Aug 28, 2015 at 07:13:08PM -0700, Randy Dunlap wrote:
> >> On 08/28/15 17:17, Luis R. Rodriguez wrote:
> >>>
> >>>  arch/s390/Kconfig |  8 +
> >>>  arch/s390/include/asm/io.h| 11 ---
> >>>  arch/s390/include/asm/pci.h   |  2 --
> >>>  arch/s390/include/asm/pci_iomap.h | 33 +
> >>>  arch/s390/pci/pci.c   |  2 ++
> >>>  include/asm-generic/io.h  | 12 
> >>>  include/asm-generic/iomap.h   | 10 ---
> >>>  include/asm-generic/pci_iomap.h   | 62 
> >>> +++
> >>>  lib/Kconfig   |  1 +
> >>>  lib/pci_iomap.c   |  5 
> >>>  10 files changed, 105 insertions(+), 41 deletions(-)
> >>>  create mode 100644 arch/s390/include/asm/pci_iomap.h
> >>>
> >>> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> >>> index 1d57000b1b24..1217b7db4265 100644
> >>> --- a/arch/s390/Kconfig
> >>> +++ b/arch/s390/Kconfig
> >>> @@ -614,6 +614,14 @@ endif# PCI
> >>>  config PCI_DOMAINS
> >>>   def_bool PCI
> >>>  
> >>> +config ARCH_PCI_NON_DISJUNCTIVE
> >>> + def_bool PCI
> >>> + help
> >>> +   On the S390 architecture PCI BAR spaces are not disjunctive, as such
> >>
> >>are not disjoint?  may be overlapping?
> >>
> >>> +   the PCI bar is required on a series of otherwise asm generic PCI
> >>> +   routines, as such S390 requires itw own implemention for these
> >>
> >>  its own implementation
> > 
> > Thanks, I've re-written this as:
> > 
> > mcgrof@ergon ~/linux-next (git::(no branch, rebasing 20150805-pend-all))$ 
> > git diff
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > index f4725d1af438..8ba5826ed13b 100644
> > --- a/arch/s390/Kconfig
> > +++ b/arch/s390/Kconfig
> > @@ -618,10 +618,10 @@ config PCI_DOMAINS
> >  config ARCH_PCI_NON_DISJUNCTIVE
> > def_bool PCI
> > help
> > - On the S390 architecture PCI BAR spaces are not disjunctive, as 
> > such
> > - the PCI bar is required on a series of otherwise asm generic PCI
> > - routines, as such S390 requires itw own implemention for these
> > - routines.
> > + On the S390 architecture PCI BAR spaces may overlap with each 
> > other,
> 
> That "other," should not end with a comma.  Either use a semi-colon or use a
> period and begin the next sentence with Because.

Amended, thanks.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Arnd Bergmann]

On Saturday 03 October 2015 01:53:46 Luis R. Rodriguez wrote:
> > 
> > Hmm, my gut feeling tells me that your approach won't solve the problem
> > in general. s390 PCI is just weird in many ways and it will occasionally
> > suffer from problems like this (as do other aspects of the s390 architecture
> > that are unlike the rest of the world).
> > 
> > Maybe Martin and Heiko can comment on this, they may have a preference
> > from the s390 point of view.
> 
> Hrm, so S390 is quirky is really odd ways that no other architecture is or
> is at least for now not expected to be ?

Absolutely correct. It is the only architecture I'm aware of that tries to
support PCI that does not use pointer dereferences for MMIO.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Arnd Bergmann]

On Saturday 03 October 2015 01:53:46 Luis R. Rodriguez wrote:
> > 
> > Hmm, my gut feeling tells me that your approach won't solve the problem
> > in general. s390 PCI is just weird in many ways and it will occasionally
> > suffer from problems like this (as do other aspects of the s390 architecture
> > that are unlike the rest of the world).
> > 
> > Maybe Martin and Heiko can comment on this, they may have a preference
> > from the s390 point of view.
> 
> Hrm, so S390 is quirky is really odd ways that no other architecture is or
> is at least for now not expected to be ?

Absolutely correct. It is the only architecture I'm aware of that tries to
support PCI that does not use pointer dereferences for MMIO.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Randy Dunlap]

On 10/02/15 16:17, Luis R. Rodriguez wrote:
> On Fri, Aug 28, 2015 at 07:13:08PM -0700, Randy Dunlap wrote:
>> On 08/28/15 17:17, Luis R. Rodriguez wrote:
>>>
>>>  arch/s390/Kconfig |  8 +
>>>  arch/s390/include/asm/io.h| 11 ---
>>>  arch/s390/include/asm/pci.h   |  2 --
>>>  arch/s390/include/asm/pci_iomap.h | 33 +
>>>  arch/s390/pci/pci.c   |  2 ++
>>>  include/asm-generic/io.h  | 12 
>>>  include/asm-generic/iomap.h   | 10 ---
>>>  include/asm-generic/pci_iomap.h   | 62 
>>> +++
>>>  lib/Kconfig   |  1 +
>>>  lib/pci_iomap.c   |  5 
>>>  10 files changed, 105 insertions(+), 41 deletions(-)
>>>  create mode 100644 arch/s390/include/asm/pci_iomap.h
>>>
>>> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
>>> index 1d57000b1b24..1217b7db4265 100644
>>> --- a/arch/s390/Kconfig
>>> +++ b/arch/s390/Kconfig
>>> @@ -614,6 +614,14 @@ endif  # PCI
>>>  config PCI_DOMAINS
>>> def_bool PCI
>>>  
>>> +config ARCH_PCI_NON_DISJUNCTIVE
>>> +   def_bool PCI
>>> +   help
>>> + On the S390 architecture PCI BAR spaces are not disjunctive, as such
>>
>>  are not disjoint?  may be overlapping?
>>
>>> + the PCI bar is required on a series of otherwise asm generic PCI
>>> + routines, as such S390 requires itw own implemention for these
>>
>>its own implementation
> 
> Thanks, I've re-written this as:
> 
> mcgrof@ergon ~/linux-next (git::(no branch, rebasing 20150805-pend-all))$ git 
> diff
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index f4725d1af438..8ba5826ed13b 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -618,10 +618,10 @@ config PCI_DOMAINS
>  config ARCH_PCI_NON_DISJUNCTIVE
> def_bool PCI
> help
> - On the S390 architecture PCI BAR spaces are not disjunctive, as such
> - the PCI bar is required on a series of otherwise asm generic PCI
> - routines, as such S390 requires itw own implemention for these
> - routines.
> + On the S390 architecture PCI BAR spaces may overlap with each other,

That "other," should not end with a comma.  Either use a semi-colon or use a
period and begin the next sentence with Because.

> + because of this the PCI bar is required on a series of otherwise asm
> + generic PCI routines and this in turn requires S390 to provide its
> + own implementation for these routines.
>  
>  config HAS_IOMEM
> def_bool PCI
>>


-- 
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Randy Dunlap]

On 10/02/15 16:17, Luis R. Rodriguez wrote:
> On Fri, Aug 28, 2015 at 07:13:08PM -0700, Randy Dunlap wrote:
>> On 08/28/15 17:17, Luis R. Rodriguez wrote:
>>>
>>>  arch/s390/Kconfig |  8 +
>>>  arch/s390/include/asm/io.h| 11 ---
>>>  arch/s390/include/asm/pci.h   |  2 --
>>>  arch/s390/include/asm/pci_iomap.h | 33 +
>>>  arch/s390/pci/pci.c   |  2 ++
>>>  include/asm-generic/io.h  | 12 
>>>  include/asm-generic/iomap.h   | 10 ---
>>>  include/asm-generic/pci_iomap.h   | 62 
>>> +++
>>>  lib/Kconfig   |  1 +
>>>  lib/pci_iomap.c   |  5 
>>>  10 files changed, 105 insertions(+), 41 deletions(-)
>>>  create mode 100644 arch/s390/include/asm/pci_iomap.h
>>>
>>> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
>>> index 1d57000b1b24..1217b7db4265 100644
>>> --- a/arch/s390/Kconfig
>>> +++ b/arch/s390/Kconfig
>>> @@ -614,6 +614,14 @@ endif  # PCI
>>>  config PCI_DOMAINS
>>> def_bool PCI
>>>  
>>> +config ARCH_PCI_NON_DISJUNCTIVE
>>> +   def_bool PCI
>>> +   help
>>> + On the S390 architecture PCI BAR spaces are not disjunctive, as such
>>
>>  are not disjoint?  may be overlapping?
>>
>>> + the PCI bar is required on a series of otherwise asm generic PCI
>>> + routines, as such S390 requires itw own implemention for these
>>
>>its own implementation
> 
> Thanks, I've re-written this as:
> 
> mcgrof@ergon ~/linux-next (git::(no branch, rebasing 20150805-pend-all))$ git 
> diff
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index f4725d1af438..8ba5826ed13b 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -618,10 +618,10 @@ config PCI_DOMAINS
>  config ARCH_PCI_NON_DISJUNCTIVE
> def_bool PCI
> help
> - On the S390 architecture PCI BAR spaces are not disjunctive, as such
> - the PCI bar is required on a series of otherwise asm generic PCI
> - routines, as such S390 requires itw own implemention for these
> - routines.
> + On the S390 architecture PCI BAR spaces may overlap with each other,

That "other," should not end with a comma.  Either use a semi-colon or use a
period and begin the next sentence with Because.

> + because of this the PCI bar is required on a series of otherwise asm
> + generic PCI routines and this in turn requires S390 to provide its
> + own implementation for these routines.
>  
>  config HAS_IOMEM
> def_bool PCI
>>


-- 
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Fri, Sep 11, 2015 at 10:14:09AM +0200, Martin Schwidefsky wrote:
> On Tue, 08 Sep 2015 15:42:40 +0200
> Arnd Bergmann  wrote:
> 
> > On Thursday 03 September 2015 03:44:15 Luis R. Rodriguez wrote:
> > > On Sun, Aug 30, 2015 at 09:30:26PM +0200, Arnd Bergmann wrote:
> > > > On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> > > > > While at it, as with the ioremap*() variants, since we have no clear
> > > > > semantics yet well defined provide a solution for them that returns
> > > > > NULL. This allows architectures to move forward by defining 
> > > > > pci_ioremap*()
> > > > > variants without requiring immediate changes to all architectures. 
> > > > > Each
> > > > > architecture then can implement their own solution as needed and
> > > > > when they get to it.
> > > > 
> > > > Which architectures are you thinking about here?
> > > 
> > > Really only S390 would benefit from this now.
> > 
> > Ok
> > 
> > > > > Build tested with allyesconfig on:
> > > > > 
> > > > > * S390
> > > > > * x86_64
> > > > > 
> > > > > Signed-off-by: Luis R. Rodriguez 
> > > > 
> > > > It's not really clear to me what the purpose of the patch is, is this 
> > > > meant as a cleanup, or are you trying to avoid some real-life bugs
> > > > you ran into?
> > > 
> > > Upon adding a new helper into CONFIG_PCI_IOMAP it was only through
> > > 0-day build testing that I found that I needed to add something for S390.
> > > This means we fix S390 reactively. With the asm-generic stuff in place
> > > to return NULL we don't need to do anything but a respective return
> > > NULL static inline, the moment that is done the author would know some
> > > architectures may not get support for the functionality they are adding.
> > > Without this we only find out reactively.
> > 
> > Hmm, my gut feeling tells me that your approach won't solve the problem
> > in general. s390 PCI is just weird in many ways and it will occasionally
> > suffer from problems like this (as do other aspects of the s390 architecture
> > that are unlike the rest of the world).
> > 
> > Maybe Martin and Heiko can comment on this, they may have a preference
> > from the s390 point of view.
> 
> I do not see how the additional Kconfig ARCH_PCI_NON_DISJUNCTIVE and the
> #ifdef indirections help with anything. An extension to lib/pci_iomap.c
> now requires an extra inline function in include/asm-generic/pci_iomap.h
> which I am sure will be added blindly without any consideration what
> s390 needs.

The purpose here was to enable evolution of this code *without* having to
require a solution in place for S390, instead on S390 such things would just
not fail to compile and when and if folks needed it they'd write it.

> Actually the patch makes it worse as the new inline will cover things up.
> Instead of a zero day compile error we will be left with a silently broken
> extension.
> 
> I prefer a compile error as it points out that there is a problem.

Of course you would though, the patch's intention is to enable folks to
have to consider a solution for S390, it would let *you* the maintainers
of S390 eventually get to it without requiring a solution in place to be
defiend for S390 always in this area due to the overlapping PCI bar
situation on S390. This is how we devised support requirements for
ioremap_*() variants, for instance, it means architecture and/or
drivers that need these variants can evolve within Linux without having
to wait to iron things out for other architectures. Is that a design
mistake ? I figured we'd want to take advantage of similar practice here,
but if S390 is soo quircky that we'd end up with too many of these
I can understand this is undesirable... but that reason is different
than *wanting* a compile error to prompt a solution before code gets
merged upstream or with the hope that a bot compile test should figure
these issues out somehow.

Now, even if these quirky design considerations are spread all over S390
it would seem to me *good* to have them well documented, it does not seem
that's the case today, so using something like this should be considered
for the gains of having these properly identified.

You guys can decide. Was just trying to be proactive here about all this.
I really don't think waiting for a compile bot to tell you there is an
issue scale wells, nor do I think its the best of designs possible.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Tue, Sep 08, 2015 at 03:42:40PM +0200, Arnd Bergmann wrote:
> On Thursday 03 September 2015 03:44:15 Luis R. Rodriguez wrote:
> > On Sun, Aug 30, 2015 at 09:30:26PM +0200, Arnd Bergmann wrote:
> > > On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> > > > While at it, as with the ioremap*() variants, since we have no clear
> > > > semantics yet well defined provide a solution for them that returns
> > > > NULL. This allows architectures to move forward by defining 
> > > > pci_ioremap*()
> > > > variants without requiring immediate changes to all architectures. Each
> > > > architecture then can implement their own solution as needed and
> > > > when they get to it.
> > > 
> > > Which architectures are you thinking about here?
> > 
> > Really only S390 would benefit from this now.
> 
> Ok
> 
> > > > Build tested with allyesconfig on:
> > > > 
> > > > * S390
> > > > * x86_64
> > > > 
> > > > Signed-off-by: Luis R. Rodriguez 
> > > 
> > > It's not really clear to me what the purpose of the patch is, is this 
> > > meant as a cleanup, or are you trying to avoid some real-life bugs
> > > you ran into?
> > 
> > Upon adding a new helper into CONFIG_PCI_IOMAP it was only through
> > 0-day build testing that I found that I needed to add something for S390.
> > This means we fix S390 reactively. With the asm-generic stuff in place
> > to return NULL we don't need to do anything but a respective return
> > NULL static inline, the moment that is done the author would know some
> > architectures may not get support for the functionality they are adding.
> > Without this we only find out reactively.
> 
> Hmm, my gut feeling tells me that your approach won't solve the problem
> in general. s390 PCI is just weird in many ways and it will occasionally
> suffer from problems like this (as do other aspects of the s390 architecture
> that are unlike the rest of the world).
> 
> Maybe Martin and Heiko can comment on this, they may have a preference
> from the s390 point of view.

Hrm, so S390 is quirky is really odd ways that no other architecture is or
is at least for now not expected to be ?

> > > The version from lib/iomap.c seems correct for uses of 
> > > CONFIG_GENERIC_IOMAP,
> > > but most architectures can do better without that option.
> > 
> > By do better do you mean a more optimized solution ?
> 
> Yes: most architectures access the PCI I/O space through memory mapped I/O,
> so we can return a regular __iomem pointer from ioport_map, rather than
> a garbled pointer that the x86 version has to use.
> 
> This means we also get to define iowrite32() to be identical to writel()
> and can save the conditional for each caller.
>
> The lib/iomap.c version is really only needed for architectures that use
> pointer access for PCI memory space, and special instructions for PCI
> I/O space, like x86. s390 has special instructions for both, some
> architectures do not have any I/O port access at all, and most of them
> treat memory and I/O space the same way.

I see, thanks.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Fri, Aug 28, 2015 at 07:13:08PM -0700, Randy Dunlap wrote:
> On 08/28/15 17:17, Luis R. Rodriguez wrote:
> > 
> >  arch/s390/Kconfig |  8 +
> >  arch/s390/include/asm/io.h| 11 ---
> >  arch/s390/include/asm/pci.h   |  2 --
> >  arch/s390/include/asm/pci_iomap.h | 33 +
> >  arch/s390/pci/pci.c   |  2 ++
> >  include/asm-generic/io.h  | 12 
> >  include/asm-generic/iomap.h   | 10 ---
> >  include/asm-generic/pci_iomap.h   | 62 
> > +++
> >  lib/Kconfig   |  1 +
> >  lib/pci_iomap.c   |  5 
> >  10 files changed, 105 insertions(+), 41 deletions(-)
> >  create mode 100644 arch/s390/include/asm/pci_iomap.h
> > 
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > index 1d57000b1b24..1217b7db4265 100644
> > --- a/arch/s390/Kconfig
> > +++ b/arch/s390/Kconfig
> > @@ -614,6 +614,14 @@ endif  # PCI
> >  config PCI_DOMAINS
> > def_bool PCI
> >  
> > +config ARCH_PCI_NON_DISJUNCTIVE
> > +   def_bool PCI
> > +   help
> > + On the S390 architecture PCI BAR spaces are not disjunctive, as such
> 
>   are not disjoint?  may be overlapping?
> 
> > + the PCI bar is required on a series of otherwise asm generic PCI
> > + routines, as such S390 requires itw own implemention for these
> 
> its own implementation

Thanks, I've re-written this as:

mcgrof@ergon ~/linux-next (git::(no branch, rebasing 20150805-pend-all))$ git 
diff
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index f4725d1af438..8ba5826ed13b 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -618,10 +618,10 @@ config PCI_DOMAINS
 config ARCH_PCI_NON_DISJUNCTIVE
def_bool PCI
help
- On the S390 architecture PCI BAR spaces are not disjunctive, as such
- the PCI bar is required on a series of otherwise asm generic PCI
- routines, as such S390 requires itw own implemention for these
- routines.
+ On the S390 architecture PCI BAR spaces may overlap with each other,
+ because of this the PCI bar is required on a series of otherwise asm
+ generic PCI routines and this in turn requires S390 to provide its
+ own implementation for these routines.
 
 config HAS_IOMEM
def_bool PCI
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Fri, Aug 28, 2015 at 07:13:08PM -0700, Randy Dunlap wrote:
> On 08/28/15 17:17, Luis R. Rodriguez wrote:
> > 
> >  arch/s390/Kconfig |  8 +
> >  arch/s390/include/asm/io.h| 11 ---
> >  arch/s390/include/asm/pci.h   |  2 --
> >  arch/s390/include/asm/pci_iomap.h | 33 +
> >  arch/s390/pci/pci.c   |  2 ++
> >  include/asm-generic/io.h  | 12 
> >  include/asm-generic/iomap.h   | 10 ---
> >  include/asm-generic/pci_iomap.h   | 62 
> > +++
> >  lib/Kconfig   |  1 +
> >  lib/pci_iomap.c   |  5 
> >  10 files changed, 105 insertions(+), 41 deletions(-)
> >  create mode 100644 arch/s390/include/asm/pci_iomap.h
> > 
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > index 1d57000b1b24..1217b7db4265 100644
> > --- a/arch/s390/Kconfig
> > +++ b/arch/s390/Kconfig
> > @@ -614,6 +614,14 @@ endif  # PCI
> >  config PCI_DOMAINS
> > def_bool PCI
> >  
> > +config ARCH_PCI_NON_DISJUNCTIVE
> > +   def_bool PCI
> > +   help
> > + On the S390 architecture PCI BAR spaces are not disjunctive, as such
> 
>   are not disjoint?  may be overlapping?
> 
> > + the PCI bar is required on a series of otherwise asm generic PCI
> > + routines, as such S390 requires itw own implemention for these
> 
> its own implementation

Thanks, I've re-written this as:

mcgrof@ergon ~/linux-next (git::(no branch, rebasing 20150805-pend-all))$ git 
diff
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index f4725d1af438..8ba5826ed13b 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -618,10 +618,10 @@ config PCI_DOMAINS
 config ARCH_PCI_NON_DISJUNCTIVE
def_bool PCI
help
- On the S390 architecture PCI BAR spaces are not disjunctive, as such
- the PCI bar is required on a series of otherwise asm generic PCI
- routines, as such S390 requires itw own implemention for these
- routines.
+ On the S390 architecture PCI BAR spaces may overlap with each other,
+ because of this the PCI bar is required on a series of otherwise asm
+ generic PCI routines and this in turn requires S390 to provide its
+ own implementation for these routines.
 
 config HAS_IOMEM
def_bool PCI
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Tue, Sep 08, 2015 at 03:42:40PM +0200, Arnd Bergmann wrote:
> On Thursday 03 September 2015 03:44:15 Luis R. Rodriguez wrote:
> > On Sun, Aug 30, 2015 at 09:30:26PM +0200, Arnd Bergmann wrote:
> > > On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> > > > While at it, as with the ioremap*() variants, since we have no clear
> > > > semantics yet well defined provide a solution for them that returns
> > > > NULL. This allows architectures to move forward by defining 
> > > > pci_ioremap*()
> > > > variants without requiring immediate changes to all architectures. Each
> > > > architecture then can implement their own solution as needed and
> > > > when they get to it.
> > > 
> > > Which architectures are you thinking about here?
> > 
> > Really only S390 would benefit from this now.
> 
> Ok
> 
> > > > Build tested with allyesconfig on:
> > > > 
> > > > * S390
> > > > * x86_64
> > > > 
> > > > Signed-off-by: Luis R. Rodriguez 
> > > 
> > > It's not really clear to me what the purpose of the patch is, is this 
> > > meant as a cleanup, or are you trying to avoid some real-life bugs
> > > you ran into?
> > 
> > Upon adding a new helper into CONFIG_PCI_IOMAP it was only through
> > 0-day build testing that I found that I needed to add something for S390.
> > This means we fix S390 reactively. With the asm-generic stuff in place
> > to return NULL we don't need to do anything but a respective return
> > NULL static inline, the moment that is done the author would know some
> > architectures may not get support for the functionality they are adding.
> > Without this we only find out reactively.
> 
> Hmm, my gut feeling tells me that your approach won't solve the problem
> in general. s390 PCI is just weird in many ways and it will occasionally
> suffer from problems like this (as do other aspects of the s390 architecture
> that are unlike the rest of the world).
> 
> Maybe Martin and Heiko can comment on this, they may have a preference
> from the s390 point of view.

Hrm, so S390 is quirky is really odd ways that no other architecture is or
is at least for now not expected to be ?

> > > The version from lib/iomap.c seems correct for uses of 
> > > CONFIG_GENERIC_IOMAP,
> > > but most architectures can do better without that option.
> > 
> > By do better do you mean a more optimized solution ?
> 
> Yes: most architectures access the PCI I/O space through memory mapped I/O,
> so we can return a regular __iomem pointer from ioport_map, rather than
> a garbled pointer that the x86 version has to use.
> 
> This means we also get to define iowrite32() to be identical to writel()
> and can save the conditional for each caller.
>
> The lib/iomap.c version is really only needed for architectures that use
> pointer access for PCI memory space, and special instructions for PCI
> I/O space, like x86. s390 has special instructions for both, some
> architectures do not have any I/O port access at all, and most of them
> treat memory and I/O space the same way.

I see, thanks.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Fri, Sep 11, 2015 at 10:14:09AM +0200, Martin Schwidefsky wrote:
> On Tue, 08 Sep 2015 15:42:40 +0200
> Arnd Bergmann  wrote:
> 
> > On Thursday 03 September 2015 03:44:15 Luis R. Rodriguez wrote:
> > > On Sun, Aug 30, 2015 at 09:30:26PM +0200, Arnd Bergmann wrote:
> > > > On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> > > > > While at it, as with the ioremap*() variants, since we have no clear
> > > > > semantics yet well defined provide a solution for them that returns
> > > > > NULL. This allows architectures to move forward by defining 
> > > > > pci_ioremap*()
> > > > > variants without requiring immediate changes to all architectures. 
> > > > > Each
> > > > > architecture then can implement their own solution as needed and
> > > > > when they get to it.
> > > > 
> > > > Which architectures are you thinking about here?
> > > 
> > > Really only S390 would benefit from this now.
> > 
> > Ok
> > 
> > > > > Build tested with allyesconfig on:
> > > > > 
> > > > > * S390
> > > > > * x86_64
> > > > > 
> > > > > Signed-off-by: Luis R. Rodriguez 
> > > > 
> > > > It's not really clear to me what the purpose of the patch is, is this 
> > > > meant as a cleanup, or are you trying to avoid some real-life bugs
> > > > you ran into?
> > > 
> > > Upon adding a new helper into CONFIG_PCI_IOMAP it was only through
> > > 0-day build testing that I found that I needed to add something for S390.
> > > This means we fix S390 reactively. With the asm-generic stuff in place
> > > to return NULL we don't need to do anything but a respective return
> > > NULL static inline, the moment that is done the author would know some
> > > architectures may not get support for the functionality they are adding.
> > > Without this we only find out reactively.
> > 
> > Hmm, my gut feeling tells me that your approach won't solve the problem
> > in general. s390 PCI is just weird in many ways and it will occasionally
> > suffer from problems like this (as do other aspects of the s390 architecture
> > that are unlike the rest of the world).
> > 
> > Maybe Martin and Heiko can comment on this, they may have a preference
> > from the s390 point of view.
> 
> I do not see how the additional Kconfig ARCH_PCI_NON_DISJUNCTIVE and the
> #ifdef indirections help with anything. An extension to lib/pci_iomap.c
> now requires an extra inline function in include/asm-generic/pci_iomap.h
> which I am sure will be added blindly without any consideration what
> s390 needs.

The purpose here was to enable evolution of this code *without* having to
require a solution in place for S390, instead on S390 such things would just
not fail to compile and when and if folks needed it they'd write it.

> Actually the patch makes it worse as the new inline will cover things up.
> Instead of a zero day compile error we will be left with a silently broken
> extension.
> 
> I prefer a compile error as it points out that there is a problem.

Of course you would though, the patch's intention is to enable folks to
have to consider a solution for S390, it would let *you* the maintainers
of S390 eventually get to it without requiring a solution in place to be
defiend for S390 always in this area due to the overlapping PCI bar
situation on S390. This is how we devised support requirements for
ioremap_*() variants, for instance, it means architecture and/or
drivers that need these variants can evolve within Linux without having
to wait to iron things out for other architectures. Is that a design
mistake ? I figured we'd want to take advantage of similar practice here,
but if S390 is soo quircky that we'd end up with too many of these
I can understand this is undesirable... but that reason is different
than *wanting* a compile error to prompt a solution before code gets
merged upstream or with the hope that a bot compile test should figure
these issues out somehow.

Now, even if these quirky design considerations are spread all over S390
it would seem to me *good* to have them well documented, it does not seem
that's the case today, so using something like this should be considered
for the gains of having these properly identified.

You guys can decide. Was just trying to be proactive here about all this.
I really don't think waiting for a compile bot to tell you there is an
issue scale wells, nor do I think its the best of designs possible.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Martin Schwidefsky]

On Tue, 08 Sep 2015 15:42:40 +0200
Arnd Bergmann  wrote:

> On Thursday 03 September 2015 03:44:15 Luis R. Rodriguez wrote:
> > On Sun, Aug 30, 2015 at 09:30:26PM +0200, Arnd Bergmann wrote:
> > > On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> > > > While at it, as with the ioremap*() variants, since we have no clear
> > > > semantics yet well defined provide a solution for them that returns
> > > > NULL. This allows architectures to move forward by defining 
> > > > pci_ioremap*()
> > > > variants without requiring immediate changes to all architectures. Each
> > > > architecture then can implement their own solution as needed and
> > > > when they get to it.
> > > 
> > > Which architectures are you thinking about here?
> > 
> > Really only S390 would benefit from this now.
> 
> Ok
> 
> > > > Build tested with allyesconfig on:
> > > > 
> > > > * S390
> > > > * x86_64
> > > > 
> > > > Signed-off-by: Luis R. Rodriguez 
> > > 
> > > It's not really clear to me what the purpose of the patch is, is this 
> > > meant as a cleanup, or are you trying to avoid some real-life bugs
> > > you ran into?
> > 
> > Upon adding a new helper into CONFIG_PCI_IOMAP it was only through
> > 0-day build testing that I found that I needed to add something for S390.
> > This means we fix S390 reactively. With the asm-generic stuff in place
> > to return NULL we don't need to do anything but a respective return
> > NULL static inline, the moment that is done the author would know some
> > architectures may not get support for the functionality they are adding.
> > Without this we only find out reactively.
> 
> Hmm, my gut feeling tells me that your approach won't solve the problem
> in general. s390 PCI is just weird in many ways and it will occasionally
> suffer from problems like this (as do other aspects of the s390 architecture
> that are unlike the rest of the world).
> 
> Maybe Martin and Heiko can comment on this, they may have a preference
> from the s390 point of view.

I do not see how the additional Kconfig ARCH_PCI_NON_DISJUNCTIVE and the
#ifdef indirections help with anything. An extension to lib/pci_iomap.c
now requires an extra inline function in include/asm-generic/pci_iomap.h
which I am sure will be added blindly without any consideration what
s390 needs.

Actually the patch makes it worse as the new inline will cover things up.
Instead of a zero day compile error we will be left with a silently broken
extension.

I prefer a compile error as it points out that there is a problem.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Martin Schwidefsky]

On Tue, 08 Sep 2015 15:42:40 +0200
Arnd Bergmann  wrote:

> On Thursday 03 September 2015 03:44:15 Luis R. Rodriguez wrote:
> > On Sun, Aug 30, 2015 at 09:30:26PM +0200, Arnd Bergmann wrote:
> > > On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> > > > While at it, as with the ioremap*() variants, since we have no clear
> > > > semantics yet well defined provide a solution for them that returns
> > > > NULL. This allows architectures to move forward by defining 
> > > > pci_ioremap*()
> > > > variants without requiring immediate changes to all architectures. Each
> > > > architecture then can implement their own solution as needed and
> > > > when they get to it.
> > > 
> > > Which architectures are you thinking about here?
> > 
> > Really only S390 would benefit from this now.
> 
> Ok
> 
> > > > Build tested with allyesconfig on:
> > > > 
> > > > * S390
> > > > * x86_64
> > > > 
> > > > Signed-off-by: Luis R. Rodriguez 
> > > 
> > > It's not really clear to me what the purpose of the patch is, is this 
> > > meant as a cleanup, or are you trying to avoid some real-life bugs
> > > you ran into?
> > 
> > Upon adding a new helper into CONFIG_PCI_IOMAP it was only through
> > 0-day build testing that I found that I needed to add something for S390.
> > This means we fix S390 reactively. With the asm-generic stuff in place
> > to return NULL we don't need to do anything but a respective return
> > NULL static inline, the moment that is done the author would know some
> > architectures may not get support for the functionality they are adding.
> > Without this we only find out reactively.
> 
> Hmm, my gut feeling tells me that your approach won't solve the problem
> in general. s390 PCI is just weird in many ways and it will occasionally
> suffer from problems like this (as do other aspects of the s390 architecture
> that are unlike the rest of the world).
> 
> Maybe Martin and Heiko can comment on this, they may have a preference
> from the s390 point of view.

I do not see how the additional Kconfig ARCH_PCI_NON_DISJUNCTIVE and the
#ifdef indirections help with anything. An extension to lib/pci_iomap.c
now requires an extra inline function in include/asm-generic/pci_iomap.h
which I am sure will be added blindly without any consideration what
s390 needs.

Actually the patch makes it worse as the new inline will cover things up.
Instead of a zero day compile error we will be left with a silently broken
extension.

I prefer a compile error as it points out that there is a problem.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Arnd Bergmann]

On Thursday 03 September 2015 03:44:15 Luis R. Rodriguez wrote:
> On Sun, Aug 30, 2015 at 09:30:26PM +0200, Arnd Bergmann wrote:
> > On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> > > While at it, as with the ioremap*() variants, since we have no clear
> > > semantics yet well defined provide a solution for them that returns
> > > NULL. This allows architectures to move forward by defining pci_ioremap*()
> > > variants without requiring immediate changes to all architectures. Each
> > > architecture then can implement their own solution as needed and
> > > when they get to it.
> > 
> > Which architectures are you thinking about here?
> 
> Really only S390 would benefit from this now.

Ok

> > > Build tested with allyesconfig on:
> > > 
> > > * S390
> > > * x86_64
> > > 
> > > Signed-off-by: Luis R. Rodriguez 
> > 
> > It's not really clear to me what the purpose of the patch is, is this 
> > meant as a cleanup, or are you trying to avoid some real-life bugs
> > you ran into?
> 
> Upon adding a new helper into CONFIG_PCI_IOMAP it was only through
> 0-day build testing that I found that I needed to add something for S390.
> This means we fix S390 reactively. With the asm-generic stuff in place
> to return NULL we don't need to do anything but a respective return
> NULL static inline, the moment that is done the author would know some
> architectures may not get support for the functionality they are adding.
> Without this we only find out reactively.

Hmm, my gut feeling tells me that your approach won't solve the problem
in general. s390 PCI is just weird in many ways and it will occasionally
suffer from problems like this (as do other aspects of the s390 architecture
that are unlike the rest of the world).

Maybe Martin and Heiko can comment on this, they may have a preference
from the s390 point of view.

> > The version from lib/iomap.c seems correct for uses of CONFIG_GENERIC_IOMAP,
> > but most architectures can do better without that option.
> 
> By do better do you mean a more optimized solution ?

Yes: most architectures access the PCI I/O space through memory mapped I/O,
so we can return a regular __iomem pointer from ioport_map, rather than
a garbled pointer that the x86 version has to use.

This means we also get to define iowrite32() to be identical to writel()
and can save the conditional for each caller.

The lib/iomap.c version is really only needed for architectures that use
pointer access for PCI memory space, and special instructions for PCI
I/O space, like x86. s390 has special instructions for both, some
architectures do not have any I/O port access at all, and most of them
treat memory and I/O space the same way.

Note that there are still two possible implementations for ioport_map
on those:

a) just return a pointer from ioport_map() that points to the existing
   mapping, define ioport_unmap() as an empty function, and have
   pci_iounmap() check the pointer.

b) create a new ioremap() mapping in ioport_map(), and define both
   ioport_unmap() and pci_iounmap as iounmap.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Wed, Sep 2, 2015 at 6:47 PM, Luis R. Rodriguez
 wrote:
> On Wed, Sep 2, 2015 at 6:44 PM, Luis R. Rodriguez  wrote:
>>> I don't think we really need to spell it out here. s390 PCI is different
>>> from everybody else's in a lot of ways, so a simple 'depends on PCI &&
>>> !S390' for CONFIG_PCI_IOMAP seems simpler and more intuitive.
>>
>> Sure that would work as well.
>
> But on second thought it still would leave us reactive to S390, this
> solution would get folks extending this asm-generic stuff to also have
> something in place for S390, even if its just returning NULL.

Arnd, thoughts?

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Wed, Sep 2, 2015 at 6:47 PM, Luis R. Rodriguez
 wrote:
> On Wed, Sep 2, 2015 at 6:44 PM, Luis R. Rodriguez  wrote:
>>> I don't think we really need to spell it out here. s390 PCI is different
>>> from everybody else's in a lot of ways, so a simple 'depends on PCI &&
>>> !S390' for CONFIG_PCI_IOMAP seems simpler and more intuitive.
>>
>> Sure that would work as well.
>
> But on second thought it still would leave us reactive to S390, this
> solution would get folks extending this asm-generic stuff to also have
> something in place for S390, even if its just returning NULL.

Arnd, thoughts?

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Arnd Bergmann]

On Thursday 03 September 2015 03:44:15 Luis R. Rodriguez wrote:
> On Sun, Aug 30, 2015 at 09:30:26PM +0200, Arnd Bergmann wrote:
> > On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> > > While at it, as with the ioremap*() variants, since we have no clear
> > > semantics yet well defined provide a solution for them that returns
> > > NULL. This allows architectures to move forward by defining pci_ioremap*()
> > > variants without requiring immediate changes to all architectures. Each
> > > architecture then can implement their own solution as needed and
> > > when they get to it.
> > 
> > Which architectures are you thinking about here?
> 
> Really only S390 would benefit from this now.

Ok

> > > Build tested with allyesconfig on:
> > > 
> > > * S390
> > > * x86_64
> > > 
> > > Signed-off-by: Luis R. Rodriguez 
> > 
> > It's not really clear to me what the purpose of the patch is, is this 
> > meant as a cleanup, or are you trying to avoid some real-life bugs
> > you ran into?
> 
> Upon adding a new helper into CONFIG_PCI_IOMAP it was only through
> 0-day build testing that I found that I needed to add something for S390.
> This means we fix S390 reactively. With the asm-generic stuff in place
> to return NULL we don't need to do anything but a respective return
> NULL static inline, the moment that is done the author would know some
> architectures may not get support for the functionality they are adding.
> Without this we only find out reactively.

Hmm, my gut feeling tells me that your approach won't solve the problem
in general. s390 PCI is just weird in many ways and it will occasionally
suffer from problems like this (as do other aspects of the s390 architecture
that are unlike the rest of the world).

Maybe Martin and Heiko can comment on this, they may have a preference
from the s390 point of view.

> > The version from lib/iomap.c seems correct for uses of CONFIG_GENERIC_IOMAP,
> > but most architectures can do better without that option.
> 
> By do better do you mean a more optimized solution ?

Yes: most architectures access the PCI I/O space through memory mapped I/O,
so we can return a regular __iomem pointer from ioport_map, rather than
a garbled pointer that the x86 version has to use.

This means we also get to define iowrite32() to be identical to writel()
and can save the conditional for each caller.

The lib/iomap.c version is really only needed for architectures that use
pointer access for PCI memory space, and special instructions for PCI
I/O space, like x86. s390 has special instructions for both, some
architectures do not have any I/O port access at all, and most of them
treat memory and I/O space the same way.

Note that there are still two possible implementations for ioport_map
on those:

a) just return a pointer from ioport_map() that points to the existing
   mapping, define ioport_unmap() as an empty function, and have
   pci_iounmap() check the pointer.

b) create a new ioremap() mapping in ioport_map(), and define both
   ioport_unmap() and pci_iounmap as iounmap.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Wed, Sep 2, 2015 at 6:44 PM, Luis R. Rodriguez  wrote:
>> I don't think we really need to spell it out here. s390 PCI is different
>> from everybody else's in a lot of ways, so a simple 'depends on PCI &&
>> !S390' for CONFIG_PCI_IOMAP seems simpler and more intuitive.
>
> Sure that would work as well.

But on second thought it still would leave us reactive to S390, this
solution would get folks extending this asm-generic stuff to also have
something in place for S390, even if its just returning NULL.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Sat, Aug 29, 2015 at 08:25:05AM -0700, Christoph Hellwig wrote:
> On Fri, Aug 28, 2015 at 05:17:27PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" 
> > 
> > The S390 architecture requires a custom pci_iomap() implementation
> > as the asm-generic implementation assumes there are disjunctions
> > between PCI BARs, and on S390 PCI BAR are not disjunctive, S390 requires
> > the bar parameter in order to find the corresponding device and create
> > the mapping cookie.
> > 
> > This clash with the asm-generic pci_iomap() implementation is implicit,
> > there are no current semantics to make this incompatability explicit.
> > Make the S390 PCI BAR non-disjunction incompatibility explicit, and
> > also pave the way for alternative incompatibilities to be defined.
> > 
> > While at it, as with the ioremap*() variants, since we have no clear
> > semantics yet well defined provide a solution for them that returns
> > NULL. This allows architectures to move forward by defining pci_ioremap*()
> > variants without requiring immediate changes to all architectures. Each
> > architecture then can implement their own solution as needed and
> > when they get to it.
> 
> Now that you have the config symbol available why not move the S390
> implementation to generic code based on that can kill of
> asm/pci_iomap.h?  Seems like we're really not dealing with something
> inherent to the architecture, but two possible implementations based
> on architecture constraints.

That would be the other approach to take too, but perhaps that can wait
until yet another architecture with similar requirement pops up.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Sun, Aug 30, 2015 at 09:30:26PM +0200, Arnd Bergmann wrote:
> On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" 
> > 
> > The S390 architecture requires a custom pci_iomap() implementation
> > as the asm-generic implementation assumes there are disjunctions
> > between PCI BARs, and on S390 PCI BAR are not disjunctive, S390 requires
> > the bar parameter in order to find the corresponding device and create
> > the mapping cookie.
> > 
> > This clash with the asm-generic pci_iomap() implementation is implicit,
> > there are no current semantics to make this incompatability explicit.
> > Make the S390 PCI BAR non-disjunction incompatibility explicit, and
> > also pave the way for alternative incompatibilities to be defined.
> 
> I don't think we really need to spell it out here. s390 PCI is different
> from everybody else's in a lot of ways, so a simple 'depends on PCI &&
> !S390' for CONFIG_PCI_IOMAP seems simpler and more intuitive.

Sure that would work as well.

> > While at it, as with the ioremap*() variants, since we have no clear
> > semantics yet well defined provide a solution for them that returns
> > NULL. This allows architectures to move forward by defining pci_ioremap*()
> > variants without requiring immediate changes to all architectures. Each
> > architecture then can implement their own solution as needed and
> > when they get to it.
> 
> Which architectures are you thinking about here?

Really only S390 would benefit from this now.

> > Build tested with allyesconfig on:
> > 
> > * S390
> > * x86_64
> > 
> > Signed-off-by: Luis R. Rodriguez 
> 
> It's not really clear to me what the purpose of the patch is, is this 
> meant as a cleanup, or are you trying to avoid some real-life bugs
> you ran into?

Upon adding a new helper into CONFIG_PCI_IOMAP it was only through
0-day build testing that I found that I needed to add something for S390.
This means we fix S390 reactively. With the asm-generic stuff in place
to return NULL we don't need to do anything but a respective return
NULL static inline, the moment that is done the author would know some
architectures may not get support for the functionality they are adding.
Without this we only find out reactively.

> > This came up as an idea after noting that S390 has no way to be
> > explicit about its requirements, this also means we don't have a
> > quick easy way to ensure that other architectures might have a
> > conflict as well. This provides an easy way to do that by having
> > the architectures define their incompatibilities and allowing those
> > then to negate GENERIC_PCI_IOMAP on lib/Kconfig.
> 
> PCI_IOMAP seems much simpler than ioport_map here, as a lot of
> architectures can have overlapping port numbers across PCI host
> bridges.

Sure, agreed.

> > I think a next cleanup could be the move of the pci_iounmap() out to
> > pci_iomap.h to avoid having each arch having to declare it. That's
> > a separate atomic change though so should go in separately.
> 
> pci_iounmap() is rather tricky, and I think some architectures currently
> get it wrong 

Whoops.

> and will actually unmap parts of the I/O port ranges from
> the PCI host controller mapping. It might be rare enough that it never
> caused problems (that got reported), but it might be nice to actually
> provide an implementation that has a chance of working.

Sure.

> The version from lib/iomap.c seems correct for uses of CONFIG_GENERIC_IOMAP,
> but most architectures can do better without that option.

By do better do you mean a more optimized solution ?

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Wed, Sep 2, 2015 at 6:44 PM, Luis R. Rodriguez  wrote:
>> I don't think we really need to spell it out here. s390 PCI is different
>> from everybody else's in a lot of ways, so a simple 'depends on PCI &&
>> !S390' for CONFIG_PCI_IOMAP seems simpler and more intuitive.
>
> Sure that would work as well.

But on second thought it still would leave us reactive to S390, this
solution would get folks extending this asm-generic stuff to also have
something in place for S390, even if its just returning NULL.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Sun, Aug 30, 2015 at 09:30:26PM +0200, Arnd Bergmann wrote:
> On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" 
> > 
> > The S390 architecture requires a custom pci_iomap() implementation
> > as the asm-generic implementation assumes there are disjunctions
> > between PCI BARs, and on S390 PCI BAR are not disjunctive, S390 requires
> > the bar parameter in order to find the corresponding device and create
> > the mapping cookie.
> > 
> > This clash with the asm-generic pci_iomap() implementation is implicit,
> > there are no current semantics to make this incompatability explicit.
> > Make the S390 PCI BAR non-disjunction incompatibility explicit, and
> > also pave the way for alternative incompatibilities to be defined.
> 
> I don't think we really need to spell it out here. s390 PCI is different
> from everybody else's in a lot of ways, so a simple 'depends on PCI &&
> !S390' for CONFIG_PCI_IOMAP seems simpler and more intuitive.

Sure that would work as well.

> > While at it, as with the ioremap*() variants, since we have no clear
> > semantics yet well defined provide a solution for them that returns
> > NULL. This allows architectures to move forward by defining pci_ioremap*()
> > variants without requiring immediate changes to all architectures. Each
> > architecture then can implement their own solution as needed and
> > when they get to it.
> 
> Which architectures are you thinking about here?

Really only S390 would benefit from this now.

> > Build tested with allyesconfig on:
> > 
> > * S390
> > * x86_64
> > 
> > Signed-off-by: Luis R. Rodriguez 
> 
> It's not really clear to me what the purpose of the patch is, is this 
> meant as a cleanup, or are you trying to avoid some real-life bugs
> you ran into?

Upon adding a new helper into CONFIG_PCI_IOMAP it was only through
0-day build testing that I found that I needed to add something for S390.
This means we fix S390 reactively. With the asm-generic stuff in place
to return NULL we don't need to do anything but a respective return
NULL static inline, the moment that is done the author would know some
architectures may not get support for the functionality they are adding.
Without this we only find out reactively.

> > This came up as an idea after noting that S390 has no way to be
> > explicit about its requirements, this also means we don't have a
> > quick easy way to ensure that other architectures might have a
> > conflict as well. This provides an easy way to do that by having
> > the architectures define their incompatibilities and allowing those
> > then to negate GENERIC_PCI_IOMAP on lib/Kconfig.
> 
> PCI_IOMAP seems much simpler than ioport_map here, as a lot of
> architectures can have overlapping port numbers across PCI host
> bridges.

Sure, agreed.

> > I think a next cleanup could be the move of the pci_iounmap() out to
> > pci_iomap.h to avoid having each arch having to declare it. That's
> > a separate atomic change though so should go in separately.
> 
> pci_iounmap() is rather tricky, and I think some architectures currently
> get it wrong 

Whoops.

> and will actually unmap parts of the I/O port ranges from
> the PCI host controller mapping. It might be rare enough that it never
> caused problems (that got reported), but it might be nice to actually
> provide an implementation that has a chance of working.

Sure.

> The version from lib/iomap.c seems correct for uses of CONFIG_GENERIC_IOMAP,
> but most architectures can do better without that option.

By do better do you mean a more optimized solution ?

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

On Sat, Aug 29, 2015 at 08:25:05AM -0700, Christoph Hellwig wrote:
> On Fri, Aug 28, 2015 at 05:17:27PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" 
> > 
> > The S390 architecture requires a custom pci_iomap() implementation
> > as the asm-generic implementation assumes there are disjunctions
> > between PCI BARs, and on S390 PCI BAR are not disjunctive, S390 requires
> > the bar parameter in order to find the corresponding device and create
> > the mapping cookie.
> > 
> > This clash with the asm-generic pci_iomap() implementation is implicit,
> > there are no current semantics to make this incompatability explicit.
> > Make the S390 PCI BAR non-disjunction incompatibility explicit, and
> > also pave the way for alternative incompatibilities to be defined.
> > 
> > While at it, as with the ioremap*() variants, since we have no clear
> > semantics yet well defined provide a solution for them that returns
> > NULL. This allows architectures to move forward by defining pci_ioremap*()
> > variants without requiring immediate changes to all architectures. Each
> > architecture then can implement their own solution as needed and
> > when they get to it.
> 
> Now that you have the config symbol available why not move the S390
> implementation to generic code based on that can kill of
> asm/pci_iomap.h?  Seems like we're really not dealing with something
> inherent to the architecture, but two possible implementations based
> on architecture constraints.

That would be the other approach to take too, but perhaps that can wait
until yet another architecture with similar requirement pops up.

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Arnd Bergmann]

On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" 
> 
> The S390 architecture requires a custom pci_iomap() implementation
> as the asm-generic implementation assumes there are disjunctions
> between PCI BARs, and on S390 PCI BAR are not disjunctive, S390 requires
> the bar parameter in order to find the corresponding device and create
> the mapping cookie.
> 
> This clash with the asm-generic pci_iomap() implementation is implicit,
> there are no current semantics to make this incompatability explicit.
> Make the S390 PCI BAR non-disjunction incompatibility explicit, and
> also pave the way for alternative incompatibilities to be defined.

I don't think we really need to spell it out here. s390 PCI is different
from everybody else's in a lot of ways, so a simple 'depends on PCI &&
!S390' for CONFIG_PCI_IOMAP seems simpler and more intuitive.

> While at it, as with the ioremap*() variants, since we have no clear
> semantics yet well defined provide a solution for them that returns
> NULL. This allows architectures to move forward by defining pci_ioremap*()
> variants without requiring immediate changes to all architectures. Each
> architecture then can implement their own solution as needed and
> when they get to it.

Which architectures are you thinking about here?

> Build tested with allyesconfig on:
> 
> * S390
> * x86_64
> 
> Signed-off-by: Luis R. Rodriguez 

It's not really clear to me what the purpose of the patch is, is this 
meant as a cleanup, or are you trying to avoid some real-life bugs
you ran into?

> This came up as an idea after noting that S390 has no way to be
> explicit about its requirements, this also means we don't have a
> quick easy way to ensure that other architectures might have a
> conflict as well. This provides an easy way to do that by having
> the architectures define their incompatibilities and allowing those
> then to negate GENERIC_PCI_IOMAP on lib/Kconfig.

PCI_IOMAP seems much simpler than ioport_map here, as a lot of
architectures can have overlapping port numbers across PCI host
bridges.
 
> I think a next cleanup could be the move of the pci_iounmap() out to
> pci_iomap.h to avoid having each arch having to declare it. That's
> a separate atomic change though so should go in separately.

pci_iounmap() is rather tricky, and I think some architectures currently
get it wrong and will actually unmap parts of the I/O port ranges from
the PCI host controller mapping. It might be rare enough that it never
caused problems (that got reported), but it might be nice to actually
provide an implementation that has a chance of working.

The version from lib/iomap.c seems correct for uses of CONFIG_GENERIC_IOMAP,
but most architectures can do better without that option.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Arnd Bergmann]

On Friday 28 August 2015 17:17:27 Luis R. Rodriguez wrote:
 From: Luis R. Rodriguez mcg...@suse.com
 
 The S390 architecture requires a custom pci_iomap() implementation
 as the asm-generic implementation assumes there are disjunctions
 between PCI BARs, and on S390 PCI BAR are not disjunctive, S390 requires
 the bar parameter in order to find the corresponding device and create
 the mapping cookie.
 
 This clash with the asm-generic pci_iomap() implementation is implicit,
 there are no current semantics to make this incompatability explicit.
 Make the S390 PCI BAR non-disjunction incompatibility explicit, and
 also pave the way for alternative incompatibilities to be defined.

I don't think we really need to spell it out here. s390 PCI is different
from everybody else's in a lot of ways, so a simple 'depends on PCI 
!S390' for CONFIG_PCI_IOMAP seems simpler and more intuitive.

 While at it, as with the ioremap*() variants, since we have no clear
 semantics yet well defined provide a solution for them that returns
 NULL. This allows architectures to move forward by defining pci_ioremap*()
 variants without requiring immediate changes to all architectures. Each
 architecture then can implement their own solution as needed and
 when they get to it.

Which architectures are you thinking about here?

 Build tested with allyesconfig on:
 
 * S390
 * x86_64
 
 Signed-off-by: Luis R. Rodriguez mcg...@suse.com

It's not really clear to me what the purpose of the patch is, is this 
meant as a cleanup, or are you trying to avoid some real-life bugs
you ran into?

 This came up as an idea after noting that S390 has no way to be
 explicit about its requirements, this also means we don't have a
 quick easy way to ensure that other architectures might have a
 conflict as well. This provides an easy way to do that by having
 the architectures define their incompatibilities and allowing those
 then to negate GENERIC_PCI_IOMAP on lib/Kconfig.

PCI_IOMAP seems much simpler than ioport_map here, as a lot of
architectures can have overlapping port numbers across PCI host
bridges.
 
 I think a next cleanup could be the move of the pci_iounmap() out to
 pci_iomap.h to avoid having each arch having to declare it. That's
 a separate atomic change though so should go in separately.

pci_iounmap() is rather tricky, and I think some architectures currently
get it wrong and will actually unmap parts of the I/O port ranges from
the PCI host controller mapping. It might be rare enough that it never
caused problems (that got reported), but it might be nice to actually
provide an implementation that has a chance of working.

The version from lib/iomap.c seems correct for uses of CONFIG_GENERIC_IOMAP,
but most architectures can do better without that option.

Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Christoph Hellwig]

On Fri, Aug 28, 2015 at 05:17:27PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" 
> 
> The S390 architecture requires a custom pci_iomap() implementation
> as the asm-generic implementation assumes there are disjunctions
> between PCI BARs, and on S390 PCI BAR are not disjunctive, S390 requires
> the bar parameter in order to find the corresponding device and create
> the mapping cookie.
> 
> This clash with the asm-generic pci_iomap() implementation is implicit,
> there are no current semantics to make this incompatability explicit.
> Make the S390 PCI BAR non-disjunction incompatibility explicit, and
> also pave the way for alternative incompatibilities to be defined.
> 
> While at it, as with the ioremap*() variants, since we have no clear
> semantics yet well defined provide a solution for them that returns
> NULL. This allows architectures to move forward by defining pci_ioremap*()
> variants without requiring immediate changes to all architectures. Each
> architecture then can implement their own solution as needed and
> when they get to it.

Now that you have the config symbol available why not move the S390
implementation to generic code based on that can kill of
asm/pci_iomap.h?  Seems like we're really not dealing with something
inherent to the architecture, but two possible implementations based
on architecture constraints.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Christoph Hellwig]

On Fri, Aug 28, 2015 at 05:17:27PM -0700, Luis R. Rodriguez wrote:
 From: Luis R. Rodriguez mcg...@suse.com
 
 The S390 architecture requires a custom pci_iomap() implementation
 as the asm-generic implementation assumes there are disjunctions
 between PCI BARs, and on S390 PCI BAR are not disjunctive, S390 requires
 the bar parameter in order to find the corresponding device and create
 the mapping cookie.
 
 This clash with the asm-generic pci_iomap() implementation is implicit,
 there are no current semantics to make this incompatability explicit.
 Make the S390 PCI BAR non-disjunction incompatibility explicit, and
 also pave the way for alternative incompatibilities to be defined.
 
 While at it, as with the ioremap*() variants, since we have no clear
 semantics yet well defined provide a solution for them that returns
 NULL. This allows architectures to move forward by defining pci_ioremap*()
 variants without requiring immediate changes to all architectures. Each
 architecture then can implement their own solution as needed and
 when they get to it.

Now that you have the config symbol available why not move the S390
implementation to generic code based on that can kill of
asm/pci_iomap.h?  Seems like we're really not dealing with something
inherent to the architecture, but two possible implementations based
on architecture constraints.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Randy Dunlap]

On 08/28/15 17:17, Luis R. Rodriguez wrote:
> 
>  arch/s390/Kconfig |  8 +
>  arch/s390/include/asm/io.h| 11 ---
>  arch/s390/include/asm/pci.h   |  2 --
>  arch/s390/include/asm/pci_iomap.h | 33 +
>  arch/s390/pci/pci.c   |  2 ++
>  include/asm-generic/io.h  | 12 
>  include/asm-generic/iomap.h   | 10 ---
>  include/asm-generic/pci_iomap.h   | 62 
> +++
>  lib/Kconfig   |  1 +
>  lib/pci_iomap.c   |  5 
>  10 files changed, 105 insertions(+), 41 deletions(-)
>  create mode 100644 arch/s390/include/asm/pci_iomap.h
> 
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 1d57000b1b24..1217b7db4265 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -614,6 +614,14 @@ endif# PCI
>  config PCI_DOMAINS
>   def_bool PCI
>  
> +config ARCH_PCI_NON_DISJUNCTIVE
> + def_bool PCI
> + help
> +   On the S390 architecture PCI BAR spaces are not disjunctive, as such

are not disjoint?  may be overlapping?

> +   the PCI bar is required on a series of otherwise asm generic PCI
> +   routines, as such S390 requires itw own implemention for these

  its own implementation

> +   routines.
> +
>  config HAS_IOMEM
>   def_bool PCI
>  


-- 
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" 

The S390 architecture requires a custom pci_iomap() implementation
as the asm-generic implementation assumes there are disjunctions
between PCI BARs, and on S390 PCI BAR are not disjunctive, S390 requires
the bar parameter in order to find the corresponding device and create
the mapping cookie.

This clash with the asm-generic pci_iomap() implementation is implicit,
there are no current semantics to make this incompatability explicit.
Make the S390 PCI BAR non-disjunction incompatibility explicit, and
also pave the way for alternative incompatibilities to be defined.

While at it, as with the ioremap*() variants, since we have no clear
semantics yet well defined provide a solution for them that returns
NULL. This allows architectures to move forward by defining pci_ioremap*()
variants without requiring immediate changes to all architectures. Each
architecture then can implement their own solution as needed and
when they get to it.

Build tested with allyesconfig on:

* S390
* x86_64

Signed-off-by: Luis R. Rodriguez 
---

This came up as an idea after noting that S390 has no way to be
explicit about its requirements, this also means we don't have a
quick easy way to ensure that other architectures might have a
conflict as well. This provides an easy way to do that by having
the architectures define their incompatibilities and allowing those
then to negate GENERIC_PCI_IOMAP on lib/Kconfig.

I think a next cleanup could be the move of the pci_iounmap() out to
pci_iomap.h to avoid having each arch having to declare it. That's
a separate atomic change though so should go in separately.

I am not sure if this is the best name for this incompatibility, please
let me know.

 arch/s390/Kconfig |  8 +
 arch/s390/include/asm/io.h| 11 ---
 arch/s390/include/asm/pci.h   |  2 --
 arch/s390/include/asm/pci_iomap.h | 33 +
 arch/s390/pci/pci.c   |  2 ++
 include/asm-generic/io.h  | 12 
 include/asm-generic/iomap.h   | 10 ---
 include/asm-generic/pci_iomap.h   | 62 +++
 lib/Kconfig   |  1 +
 lib/pci_iomap.c   |  5 
 10 files changed, 105 insertions(+), 41 deletions(-)
 create mode 100644 arch/s390/include/asm/pci_iomap.h

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 1d57000b1b24..1217b7db4265 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -614,6 +614,14 @@ endif  # PCI
 config PCI_DOMAINS
def_bool PCI
 
+config ARCH_PCI_NON_DISJUNCTIVE
+   def_bool PCI
+   help
+ On the S390 architecture PCI BAR spaces are not disjunctive, as such
+ the PCI bar is required on a series of otherwise asm generic PCI
+ routines, as such S390 requires itw own implemention for these
+ routines.
+
 config HAS_IOMEM
def_bool PCI
 
diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
index 437e9af96688..27443c778367 100644
--- a/arch/s390/include/asm/io.h
+++ b/arch/s390/include/asm/io.h
@@ -49,17 +49,6 @@ static inline void ioport_unmap(void __iomem *p)
 {
 }
 
-/*
- * s390 needs a private implementation of pci_iomap since ioremap with its
- * offset parameter isn't sufficient. That's because BAR spaces are not
- * disjunctive on s390 so we need the bar parameter of pci_iomap to find
- * the corresponding device and create the mapping cookie.
- */
-#define pci_iomap pci_iomap
-#define pci_iounmap pci_iounmap
-#define pci_iomap_wc pci_iomap
-#define pci_iomap_wc_range pci_iomap_range
-
 #define memcpy_fromio(dst, src, count) zpci_memcpy_fromio(dst, src, count)
 #define memcpy_toio(dst, src, count)   zpci_memcpy_toio(dst, src, count)
 #define memset_io(dst, val, count) zpci_memset_io(dst, val, count)
diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 34d960353a08..51a0a426a6e3 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -18,8 +18,6 @@
 
 #define pcibios_assign_all_busses()(0)
 
-void __iomem *pci_iomap(struct pci_dev *, int, unsigned long);
-void pci_iounmap(struct pci_dev *, void __iomem *);
 int pci_domain_nr(struct pci_bus *);
 int pci_proc_domain(struct pci_bus *);
 
diff --git a/arch/s390/include/asm/pci_iomap.h 
b/arch/s390/include/asm/pci_iomap.h
new file mode 100644
index ..b897ff4cac59
--- /dev/null
+++ b/arch/s390/include/asm/pci_iomap.h
@@ -0,0 +1,33 @@
+#ifndef _S390_ASM_PCI_IOMAP_H
+#define _S390_ASM_PCI_IOMAP_H
+/*
+ *  S390 version
+ */
+#include 
+#include 
+#include 
+
+#ifdef CONFIG_PCI
+
+struct pci_dev;
+
+/*
+ * s390 needs a private implementation of pci_iomap since ioremap with its
+ * offset parameter isn't sufficient. That's because BAR spaces are not
+ * disjunctive on s390 so we need the bar parameter of pci_iomap to find
+ * the corresponding device and create the mapping cookie.
+ */
+
+/* XXX: in the future we'll remove these */

Re: [RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Randy Dunlap]

On 08/28/15 17:17, Luis R. Rodriguez wrote:
 
  arch/s390/Kconfig |  8 +
  arch/s390/include/asm/io.h| 11 ---
  arch/s390/include/asm/pci.h   |  2 --
  arch/s390/include/asm/pci_iomap.h | 33 +
  arch/s390/pci/pci.c   |  2 ++
  include/asm-generic/io.h  | 12 
  include/asm-generic/iomap.h   | 10 ---
  include/asm-generic/pci_iomap.h   | 62 
 +++
  lib/Kconfig   |  1 +
  lib/pci_iomap.c   |  5 
  10 files changed, 105 insertions(+), 41 deletions(-)
  create mode 100644 arch/s390/include/asm/pci_iomap.h
 
 diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
 index 1d57000b1b24..1217b7db4265 100644
 --- a/arch/s390/Kconfig
 +++ b/arch/s390/Kconfig
 @@ -614,6 +614,14 @@ endif# PCI
  config PCI_DOMAINS
   def_bool PCI
  
 +config ARCH_PCI_NON_DISJUNCTIVE
 + def_bool PCI
 + help
 +   On the S390 architecture PCI BAR spaces are not disjunctive, as such

are not disjoint?  may be overlapping?

 +   the PCI bar is required on a series of otherwise asm generic PCI
 +   routines, as such S390 requires itw own implemention for these

  its own implementation

 +   routines.
 +
  config HAS_IOMEM
   def_bool PCI
  


-- 
~Randy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC] asm-generic/pci_iomap.h: make custom PCI BAR requirements explicit

[Luis R. Rodriguez]

From: Luis R. Rodriguez mcg...@suse.com

The S390 architecture requires a custom pci_iomap() implementation
as the asm-generic implementation assumes there are disjunctions
between PCI BARs, and on S390 PCI BAR are not disjunctive, S390 requires
the bar parameter in order to find the corresponding device and create
the mapping cookie.

This clash with the asm-generic pci_iomap() implementation is implicit,
there are no current semantics to make this incompatability explicit.
Make the S390 PCI BAR non-disjunction incompatibility explicit, and
also pave the way for alternative incompatibilities to be defined.

While at it, as with the ioremap*() variants, since we have no clear
semantics yet well defined provide a solution for them that returns
NULL. This allows architectures to move forward by defining pci_ioremap*()
variants without requiring immediate changes to all architectures. Each
architecture then can implement their own solution as needed and
when they get to it.

Build tested with allyesconfig on:

* S390
* x86_64

Signed-off-by: Luis R. Rodriguez mcg...@suse.com
---

This came up as an idea after noting that S390 has no way to be
explicit about its requirements, this also means we don't have a
quick easy way to ensure that other architectures might have a
conflict as well. This provides an easy way to do that by having
the architectures define their incompatibilities and allowing those
then to negate GENERIC_PCI_IOMAP on lib/Kconfig.

I think a next cleanup could be the move of the pci_iounmap() out to
pci_iomap.h to avoid having each arch having to declare it. That's
a separate atomic change though so should go in separately.

I am not sure if this is the best name for this incompatibility, please
let me know.

 arch/s390/Kconfig |  8 +
 arch/s390/include/asm/io.h| 11 ---
 arch/s390/include/asm/pci.h   |  2 --
 arch/s390/include/asm/pci_iomap.h | 33 +
 arch/s390/pci/pci.c   |  2 ++
 include/asm-generic/io.h  | 12 
 include/asm-generic/iomap.h   | 10 ---
 include/asm-generic/pci_iomap.h   | 62 +++
 lib/Kconfig   |  1 +
 lib/pci_iomap.c   |  5 
 10 files changed, 105 insertions(+), 41 deletions(-)
 create mode 100644 arch/s390/include/asm/pci_iomap.h

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 1d57000b1b24..1217b7db4265 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -614,6 +614,14 @@ endif  # PCI
 config PCI_DOMAINS
def_bool PCI
 
+config ARCH_PCI_NON_DISJUNCTIVE
+   def_bool PCI
+   help
+ On the S390 architecture PCI BAR spaces are not disjunctive, as such
+ the PCI bar is required on a series of otherwise asm generic PCI
+ routines, as such S390 requires itw own implemention for these
+ routines.
+
 config HAS_IOMEM
def_bool PCI
 
diff --git a/arch/s390/include/asm/io.h b/arch/s390/include/asm/io.h
index 437e9af96688..27443c778367 100644
--- a/arch/s390/include/asm/io.h
+++ b/arch/s390/include/asm/io.h
@@ -49,17 +49,6 @@ static inline void ioport_unmap(void __iomem *p)
 {
 }
 
-/*
- * s390 needs a private implementation of pci_iomap since ioremap with its
- * offset parameter isn't sufficient. That's because BAR spaces are not
- * disjunctive on s390 so we need the bar parameter of pci_iomap to find
- * the corresponding device and create the mapping cookie.
- */
-#define pci_iomap pci_iomap
-#define pci_iounmap pci_iounmap
-#define pci_iomap_wc pci_iomap
-#define pci_iomap_wc_range pci_iomap_range
-
 #define memcpy_fromio(dst, src, count) zpci_memcpy_fromio(dst, src, count)
 #define memcpy_toio(dst, src, count)   zpci_memcpy_toio(dst, src, count)
 #define memset_io(dst, val, count) zpci_memset_io(dst, val, count)
diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 34d960353a08..51a0a426a6e3 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -18,8 +18,6 @@
 
 #define pcibios_assign_all_busses()(0)
 
-void __iomem *pci_iomap(struct pci_dev *, int, unsigned long);
-void pci_iounmap(struct pci_dev *, void __iomem *);
 int pci_domain_nr(struct pci_bus *);
 int pci_proc_domain(struct pci_bus *);
 
diff --git a/arch/s390/include/asm/pci_iomap.h 
b/arch/s390/include/asm/pci_iomap.h
new file mode 100644
index ..b897ff4cac59
--- /dev/null
+++ b/arch/s390/include/asm/pci_iomap.h
@@ -0,0 +1,33 @@
+#ifndef _S390_ASM_PCI_IOMAP_H
+#define _S390_ASM_PCI_IOMAP_H
+/*
+ *  S390 version
+ */
+#include linux/kernel.h
+#include asm/page.h
+#include asm/pci_io.h
+
+#ifdef CONFIG_PCI
+
+struct pci_dev;
+
+/*
+ * s390 needs a private implementation of pci_iomap since ioremap with its
+ * offset parameter isn't sufficient. That's because BAR spaces are not
+ * disjunctive on s390 so we need the bar parameter of pci_iomap to find
+ * the corresponding device and create the mapping