[v2,2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()

Commit Message

Luis R. Rodriguez June 24, 2015, 5:23 p.m. UTC

From: "Luis R. Rodriguez" <[e-mail]>

On built-in kernels this warning will always splat as this is part
of the module init. Fix that by shifting the PAT requirement check
out under the code that does the "quasi-probe" for the device. This
device driver relies on an existing driver to find its own devices,
it looks for that device driver and its own found devices, then
uses driver_for_each_device() to try to see if it can probe each of
those devices as a frambuffer device with ivtvfb_init_card(). We
tuck the PAT requiremenet check then on the ivtvfb_init_card()
call making the check at least require an ivtv device present
before complaining.

Reported-by: Fengguang Wu <[e-mail]> [0-day test robot]
Signed-off-by: Luis R. Rodriguez <[e-mail]>
---
 drivers/media/pci/ivtv/ivtvfb.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Ingo Molnar June 25, 2015, 6:51 a.m. UTC | #1

* Luis R. Rodriguez <[e-mail]> wrote:

> From: "Luis R. Rodriguez" <[e-mail]>
> 
> On built-in kernels this warning will always splat as this is part
> of the module init. Fix that by shifting the PAT requirement check
> out under the code that does the "quasi-probe" for the device. This
> device driver relies on an existing driver to find its own devices,
> it looks for that device driver and its own found devices, then
> uses driver_for_each_device() to try to see if it can probe each of
> those devices as a frambuffer device with ivtvfb_init_card(). We
> tuck the PAT requiremenet check then on the ivtvfb_init_card()
> call making the check at least require an ivtv device present
> before complaining.
> 
> Reported-by: Fengguang Wu <[e-mail]> [0-day test robot]
> Signed-off-by: Luis R. Rodriguez <[e-mail]>
> ---
>  drivers/media/pci/ivtv/ivtvfb.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
> index 4cb365d..8b95eef 100644
> --- a/drivers/media/pci/ivtv/ivtvfb.c
> +++ b/drivers/media/pci/ivtv/ivtvfb.c
> @@ -38,6 +38,8 @@
>      Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/fb.h>
> @@ -1171,6 +1173,13 @@ static int ivtvfb_init_card(struct ivtv *itv)
>  {
>  	int rc;
>  
> +#ifdef CONFIG_X86_64
> +	if (pat_enabled()) {
> +		pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel parameter\n");
> +		return -ENODEV;
> +	}
> +#endif
> +
>  	if (itv->osd_info) {
>  		IVTVFB_ERR("Card %d already initialised\n", ivtvfb_card_id);
>  		return -EBUSY;

Same argument as for ipath: why not make arch_phys_wc_add() fail on PAT and return 
-1, and check it in arch_phys_wc_del()?

That way we don't do anything drastic, the remaining few drivers still keep 
working (albeit suboptimally - can be worked around with the 'nopat' boot option) 
- yet we've reduced the use of MTRRs drastically.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain June 25, 2015, 5:38 p.m. UTC | #2

On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> 
> * Luis R. Rodriguez <[e-mail]> wrote:
> 
> > From: "Luis R. Rodriguez" <[e-mail]>
> > 
> > On built-in kernels this warning will always splat as this is part
> > of the module init. Fix that by shifting the PAT requirement check
> > out under the code that does the "quasi-probe" for the device. This
> > device driver relies on an existing driver to find its own devices,
> > it looks for that device driver and its own found devices, then
> > uses driver_for_each_device() to try to see if it can probe each of
> > those devices as a frambuffer device with ivtvfb_init_card(). We
> > tuck the PAT requiremenet check then on the ivtvfb_init_card()
> > call making the check at least require an ivtv device present
> > before complaining.
> > 
> > Reported-by: Fengguang Wu <[e-mail]> [0-day test robot]
> > Signed-off-by: Luis R. Rodriguez <[e-mail]>
> > ---
> >  drivers/media/pci/ivtv/ivtvfb.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
> > index 4cb365d..8b95eef 100644
> > --- a/drivers/media/pci/ivtv/ivtvfb.c
> > +++ b/drivers/media/pci/ivtv/ivtvfb.c
> > @@ -38,6 +38,8 @@
> >      Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> >   */
> >  
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> >  #include <linux/module.h>
> >  #include <linux/kernel.h>
> >  #include <linux/fb.h>
> > @@ -1171,6 +1173,13 @@ static int ivtvfb_init_card(struct ivtv *itv)
> >  {
> >  	int rc;
> >  
> > +#ifdef CONFIG_X86_64
> > +	if (pat_enabled()) {
> > +		pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel parameter\n");
> > +		return -ENODEV;
> > +	}
> > +#endif
> > +
> >  	if (itv->osd_info) {
> >  		IVTVFB_ERR("Card %d already initialised\n", ivtvfb_card_id);
> >  		return -EBUSY;
> 
> Same argument as for ipath: why not make arch_phys_wc_add() fail on PAT and return 
> -1, and check it in arch_phys_wc_del()?

The arch_phys_wc_add() is a no-op for PAT systems but for PAT to work we need
not only need to add this in where we replace the MTRR call but we also need
to convert ioremap_nocache() calls to ioremap_wc() but only if things were
split up already.

We racked our heads [0] [1] trying to figure out how to do the split for ivtv. The
issues with ivtv were that the firmware decides where the WC area is and does
not provide APIs to expose it. Then alternatives are to for example just use WC
on the entire full range and use work arounds write(); wmb(); read(); for MMIO
registers. That idea came from the use case of the Myricom Ethernet device
driver which uses WC as a compromise to address a performance regression if
it didn't use WC on an entire range, it uses the work around for the MMIO
registers. I considered very *briefly* adding a generic API that would let
device driver use this but dropped the idea as it seems this was not a common
issue and this was rather a work around.

I should note that Benjamin recenlty noted that power pc (and he says possibly
more) writel() and co contains an implicit mb(). That addresses some of it may
maybe not all requirements.

[0] http://lkml.kernel.org/r/[e-mail]
[1] https://marc.info/?t=142894741100005&r=1&w=2

> That way we don't do anything drastic, the remaining few drivers still keep 
> working (albeit suboptimally - can be worked around with the 'nopat' boot option) 
> - yet we've reduced the use of MTRRs drastically.

It seems the 3 drivers that needed hackery are ancient, not common and likely
adding a general fix more work than the gains provided through it. We'd need
to address not only the use of the arch_phys calls but also to split their MMIO
registers / WC desire area. This later part was the harder part of all this.
Fortunately the "norm" is that modern devices have a full PCI bar designated
for each now. Furthermore in the future we should hope for buses that do the
negotiation of this for us and we can just map things out for them in the
kernel. benh seems to note ppc does some hackery for this but I wouldn't bet
on it being viable without issues on x86 just unless a thorough review / big
wagers are made.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ingo Molnar June 26, 2015, 8:45 a.m. UTC | #3

* Luis R. Rodriguez <[e-mail]> wrote:

> On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> > 
> > * Luis R. Rodriguez <[e-mail]> wrote:
> > 
> > > From: "Luis R. Rodriguez" <[e-mail]>
> > > 
> > > On built-in kernels this warning will always splat as this is part
> > > of the module init. Fix that by shifting the PAT requirement check
> > > out under the code that does the "quasi-probe" for the device. This
> > > device driver relies on an existing driver to find its own devices,
> > > it looks for that device driver and its own found devices, then
> > > uses driver_for_each_device() to try to see if it can probe each of
> > > those devices as a frambuffer device with ivtvfb_init_card(). We
> > > tuck the PAT requiremenet check then on the ivtvfb_init_card()
> > > call making the check at least require an ivtv device present
> > > before complaining.
> > > 
> > > Reported-by: Fengguang Wu <[e-mail]> [0-day test robot]
> > > Signed-off-by: Luis R. Rodriguez <[e-mail]>
> > > ---
> > >  drivers/media/pci/ivtv/ivtvfb.c | 15 +++++++++------
> > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
> > > index 4cb365d..8b95eef 100644
> > > --- a/drivers/media/pci/ivtv/ivtvfb.c
> > > +++ b/drivers/media/pci/ivtv/ivtvfb.c
> > > @@ -38,6 +38,8 @@
> > >      Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> > >   */
> > >  
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > >  #include <linux/module.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/fb.h>
> > > @@ -1171,6 +1173,13 @@ static int ivtvfb_init_card(struct ivtv *itv)
> > >  {
> > >  	int rc;
> > >  
> > > +#ifdef CONFIG_X86_64
> > > +	if (pat_enabled()) {
> > > +		pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel parameter\n");
> > > +		return -ENODEV;
> > > +	}
> > > +#endif
> > > +
> > >  	if (itv->osd_info) {
> > >  		IVTVFB_ERR("Card %d already initialised\n", ivtvfb_card_id);
> > >  		return -EBUSY;
> > 
> > Same argument as for ipath: why not make arch_phys_wc_add() fail on PAT and 
> > return -1, and check it in arch_phys_wc_del()?
> 
> The arch_phys_wc_add() is a no-op for PAT systems but for PAT to work we need 
> not only need to add this in where we replace the MTRR call but we also need to 
> convert ioremap_nocache() calls to ioremap_wc() but only if things were split up 
> already.

We don't need to do that: for such legacy drivers we can fall back to UC just 
fine, and inform the user that by booting with 'nopat' the old behavior will be 
back...

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andy Walls June 26, 2015, 12:36 p.m. UTC | #4

On Fri, 2015-06-26 at 10:45 +0200, Ingo Molnar wrote:
> * Luis R. Rodriguez <[e-mail]> wrote:
> 
> > On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> > > 
> > > * Luis R. Rodriguez <[e-mail]> wrote:
> > > 
> > > > From: "Luis R. Rodriguez" <[e-mail]>
> > > > 
> > > > On built-in kernels this warning will always splat as this is part
> > > > of the module init. Fix that by shifting the PAT requirement check
> > > > out under the code that does the "quasi-probe" for the device. This
> > > > device driver relies on an existing driver to find its own devices,
> > > > it looks for that device driver and its own found devices, then
> > > > uses driver_for_each_device() to try to see if it can probe each of
> > > > those devices as a frambuffer device with ivtvfb_init_card(). We
> > > > tuck the PAT requiremenet check then on the ivtvfb_init_card()
> > > > call making the check at least require an ivtv device present
> > > > before complaining.
> > > > 
> > > > Reported-by: Fengguang Wu <[e-mail]> [0-day test robot]
> > > > Signed-off-by: Luis R. Rodriguez <[e-mail]>
> > > > ---
> > > >  drivers/media/pci/ivtv/ivtvfb.c | 15 +++++++++------
> > > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
> > > > index 4cb365d..8b95eef 100644
> > > > --- a/drivers/media/pci/ivtv/ivtvfb.c
> > > > +++ b/drivers/media/pci/ivtv/ivtvfb.c
> > > > @@ -38,6 +38,8 @@
> > > >      Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> > > >   */
> > > >  
> > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > +
> > > >  #include <linux/module.h>
> > > >  #include <linux/kernel.h>
> > > >  #include <linux/fb.h>
> > > > @@ -1171,6 +1173,13 @@ static int ivtvfb_init_card(struct ivtv *itv)
> > > >  {
> > > >  	int rc;
> > > >  
> > > > +#ifdef CONFIG_X86_64
> > > > +	if (pat_enabled()) {
> > > > +		pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel parameter\n");
> > > > +		return -ENODEV;
> > > > +	}
> > > > +#endif
> > > > +
> > > >  	if (itv->osd_info) {
> > > >  		IVTVFB_ERR("Card %d already initialised\n", ivtvfb_card_id);
> > > >  		return -EBUSY;
> > > 
> > > Same argument as for ipath: why not make arch_phys_wc_add() fail on PAT and 
> > > return -1, and check it in arch_phys_wc_del()?
> > 
> > The arch_phys_wc_add() is a no-op for PAT systems but for PAT to work we need 
> > not only need to add this in where we replace the MTRR call but we also need to 
> > convert ioremap_nocache() calls to ioremap_wc() but only if things were split up 
> > already.
> 

Hi Ingo,

> We don't need to do that: for such legacy drivers we can fall back to UC just 
> fine, and inform the user that by booting with 'nopat' the old behavior will be 
> back...

This is really a "user experience" decision.

IMO anyone who is still using ivtvfb and an old conventional PCI PVR-350
to render, at SDTV resolution, an X Desktop display or video playback on
a television screen, isn't going to give a hoot about modern things like
PAT.  The user will simply want the framebuffer performance they are
accustomed to having with their system.  UC will probably yield
unsatisfactory performance for an ivtvfb framebuffer.

With that in mind, I would think it better to obviously and clearly
disable the ivtvfb framebuffer module with PAT enabled, so the user will
check the log and read the steps needed to obtain acceptable
performance.

Maybe that's me just wanting to head off the "poor ivtvfb performance
with latest kernel" bug reports.

Whatever the decision, my stock response to bug reports related to this
will always be "What do the logs say?".

Regards,
Andy


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ingo Molnar June 29, 2015, 6:55 a.m. UTC | #5

* Andy Walls <[e-mail]> wrote:

> On Fri, 2015-06-26 at 10:45 +0200, Ingo Molnar wrote:
> > * Luis R. Rodriguez <[e-mail]> wrote:
> > 
> > > On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> > > > 
> > > > * Luis R. Rodriguez <[e-mail]> wrote:
> > > > 
> > > > > From: "Luis R. Rodriguez" <[e-mail]>
> > > > > 
> > > > > On built-in kernels this warning will always splat as this is part
> > > > > of the module init. Fix that by shifting the PAT requirement check
> > > > > out under the code that does the "quasi-probe" for the device. This
> > > > > device driver relies on an existing driver to find its own devices,
> > > > > it looks for that device driver and its own found devices, then
> > > > > uses driver_for_each_device() to try to see if it can probe each of
> > > > > those devices as a frambuffer device with ivtvfb_init_card(). We
> > > > > tuck the PAT requiremenet check then on the ivtvfb_init_card()
> > > > > call making the check at least require an ivtv device present
> > > > > before complaining.
> > > > > 
> > > > > Reported-by: Fengguang Wu <[e-mail]> [0-day test robot]
> > > > > Signed-off-by: Luis R. Rodriguez <[e-mail]>
> > > > > ---
> > > > >  drivers/media/pci/ivtv/ivtvfb.c | 15 +++++++++------
> > > > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
> > > > > index 4cb365d..8b95eef 100644
> > > > > --- a/drivers/media/pci/ivtv/ivtvfb.c
> > > > > +++ b/drivers/media/pci/ivtv/ivtvfb.c
> > > > > @@ -38,6 +38,8 @@
> > > > >      Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> > > > >   */
> > > > >  
> > > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > > +
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/kernel.h>
> > > > >  #include <linux/fb.h>
> > > > > @@ -1171,6 +1173,13 @@ static int ivtvfb_init_card(struct ivtv *itv)
> > > > >  {
> > > > >  	int rc;
> > > > >  
> > > > > +#ifdef CONFIG_X86_64
> > > > > +	if (pat_enabled()) {
> > > > > +		pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel parameter\n");
> > > > > +		return -ENODEV;
> > > > > +	}
> > > > > +#endif
> > > > > +
> > > > >  	if (itv->osd_info) {
> > > > >  		IVTVFB_ERR("Card %d already initialised\n", ivtvfb_card_id);
> > > > >  		return -EBUSY;
> > > > 
> > > > Same argument as for ipath: why not make arch_phys_wc_add() fail on PAT and 
> > > > return -1, and check it in arch_phys_wc_del()?
> > > 
> > > The arch_phys_wc_add() is a no-op for PAT systems but for PAT to work we need 
> > > not only need to add this in where we replace the MTRR call but we also need to 
> > > convert ioremap_nocache() calls to ioremap_wc() but only if things were split up 
> > > already.
> > 
> 
> Hi Ingo,
> 
> > We don't need to do that: for such legacy drivers we can fall back to UC just 
> > fine, and inform the user that by booting with 'nopat' the old behavior will be 
> > back...
> 
> This is really a "user experience" decision.
> 
> IMO anyone who is still using ivtvfb and an old conventional PCI PVR-350 to 
> render, at SDTV resolution, an X Desktop display or video playback on a 
> television screen, isn't going to give a hoot about modern things like PAT.  The 
> user will simply want the framebuffer performance they are accustomed to having 
> with their system.  UC will probably yield unsatisfactory performance for an 
> ivtvfb framebuffer.
> 
> With that in mind, I would think it better to obviously and clearly disable the 
> ivtvfb framebuffer module with PAT enabled, so the user will check the log and 
> read the steps needed to obtain acceptable performance.
> 
> Maybe that's me just wanting to head off the "poor ivtvfb performance with 
> latest kernel" bug reports.
> 
> Whatever the decision, my stock response to bug reports related to this will 
> always be "What do the logs say?".

So what if that frame buffer is their only (working) frame buffer? A slow 
framebuffer is still much better at giving people logs to look at than a 
non-working one.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain July 7, 2015, 12:44 a.m. UTC | #6

On Mon, Jun 29, 2015 at 05:52:18AM -0400, Andy Walls wrote:
> On June 29, 2015 2:55:05 AM EDT, Ingo Molnar <[e-mail]> wrote:
> >
> >* Andy Walls <[e-mail]> wrote:
> >
> >> On Fri, 2015-06-26 at 10:45 +0200, Ingo Molnar wrote:
> >> > * Luis R. Rodriguez <[e-mail]> wrote:
> >> > 
> >> > > On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> >> > > > 
> >> > > > * Luis R. Rodriguez <[e-mail]> wrote:
> >> > > > 
> >> > > > > From: "Luis R. Rodriguez" <[e-mail]>
> >> > > > > 
> >> > > > > On built-in kernels this warning will always splat as this is
> >part
> >> > > > > of the module init. Fix that by shifting the PAT requirement
> >check
> >> > > > > out under the code that does the "quasi-probe" for the
> >device. This
> >> > > > > device driver relies on an existing driver to find its own
> >devices,
> >> > > > > it looks for that device driver and its own found devices,
> >then
> >> > > > > uses driver_for_each_device() to try to see if it can probe
> >each of
> >> > > > > those devices as a frambuffer device with ivtvfb_init_card().
> >We
> >> > > > > tuck the PAT requiremenet check then on the
> >ivtvfb_init_card()
> >> > > > > call making the check at least require an ivtv device present
> >> > > > > before complaining.
> >> > > > > 
> >> > > > > Reported-by: Fengguang Wu <[e-mail]> [0-day
> >test robot]
> >> > > > > Signed-off-by: Luis R. Rodriguez <[e-mail]>
> >> > > > > ---
> >> > > > >  drivers/media/pci/ivtv/ivtvfb.c | 15 +++++++++------
> >> > > > >  1 file changed, 9 insertions(+), 6 deletions(-)
> >> > > > > 
> >> > > > > diff --git a/drivers/media/pci/ivtv/ivtvfb.c
> >b/drivers/media/pci/ivtv/ivtvfb.c
> >> > > > > index 4cb365d..8b95eef 100644
> >> > > > > --- a/drivers/media/pci/ivtv/ivtvfb.c
> >> > > > > +++ b/drivers/media/pci/ivtv/ivtvfb.c
> >> > > > > @@ -38,6 +38,8 @@
> >> > > > >      Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> > 02111-1307  USA
> >> > > > >   */
> >> > > > >  
> >> > > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >> > > > > +
> >> > > > >  #include <linux/module.h>
> >> > > > >  #include <linux/kernel.h>
> >> > > > >  #include <linux/fb.h>
> >> > > > > @@ -1171,6 +1173,13 @@ static int ivtvfb_init_card(struct
> >ivtv *itv)
> >> > > > >  {
> >> > > > >  	int rc;
> >> > > > >  
> >> > > > > +#ifdef CONFIG_X86_64
> >> > > > > +	if (pat_enabled()) {
> >> > > > > +		pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel
> >parameter\n");
> >> > > > > +		return -ENODEV;
> >> > > > > +	}
> >> > > > > +#endif
> >> > > > > +
> >> > > > >  	if (itv->osd_info) {
> >> > > > >  		IVTVFB_ERR("Card %d already initialised\n",
> >ivtvfb_card_id);
> >> > > > >  		return -EBUSY;
> >> > > > 
> >> > > > Same argument as for ipath: why not make arch_phys_wc_add()
> >fail on PAT and 
> >> > > > return -1, and check it in arch_phys_wc_del()?
> >> > > 
> >> > > The arch_phys_wc_add() is a no-op for PAT systems but for PAT to
> >work we need 
> >> > > not only need to add this in where we replace the MTRR call but
> >we also need to 
> >> > > convert ioremap_nocache() calls to ioremap_wc() but only if
> >things were split up 
> >> > > already.
> >> > 
> >> 
> >> Hi Ingo,
> >> 
> >> > We don't need to do that: for such legacy drivers we can fall back
> >to UC just 
> >> > fine, and inform the user that by booting with 'nopat' the old
> >behavior will be 
> >> > back...
> >> 
> >> This is really a "user experience" decision.
> >> 
> >> IMO anyone who is still using ivtvfb and an old conventional PCI
> >PVR-350 to 
> >> render, at SDTV resolution, an X Desktop display or video playback on
> >a 
> >> television screen, isn't going to give a hoot about modern things
> >like PAT.  The 
> >> user will simply want the framebuffer performance they are accustomed
> >to having 
> >> with their system.  UC will probably yield unsatisfactory performance
> >for an 
> >> ivtvfb framebuffer.
> >> 
> >> With that in mind, I would think it better to obviously and clearly
> >disable the 
> >> ivtvfb framebuffer module with PAT enabled, so the user will check
> >the log and 
> >> read the steps needed to obtain acceptable performance.
> >> 
> >> Maybe that's me just wanting to head off the "poor ivtvfb performance
> >with 
> >> latest kernel" bug reports.
> >> 
> >> Whatever the decision, my stock response to bug reports related to
> >this will 
> >> always be "What do the logs say?".
> >
> >So what if that frame buffer is their only (working) frame buffer? A
> >slow 
> >framebuffer is still much better at giving people logs to look at than
> >a 
> >non-working one.
> >
> >Thanks,
> >
> >	Ingo
> 
> Hi Ingo,
> 
> For an old DVR setup, I can see that being the case.
> 
> So a sluggish framebuffer is better than none.

OK, so I knew I had considered this strategy before, I did and here's
the recap:

I originally had proposed this same exact thing with my first patch set but [0]
had set to require a new __arch_phys_wc_add() which forces the setting without
checking if pat_enabled() is true. This was set out as a transient API in hopes
that device drivers that require work but hadn't been converted to split the
ioremap'd calls would later be modified / fixed with the split. Here's an
update for the status of each driver:

Driver          File
------------------------------------------------------------
fusion          drivers/message/fusion/mptbase.c
  This code was commented out, the code was just removed, this longer applies.

ivtv            drivers/media/pci/ivtv/ivtvfb.c
  The firmware sucks, it does not expose the WC area, and likely we won't be able to
  do the split, the work for it is simply not worth the effort. This driver is old
  for an old DVR setup.

ipath           drivers/infiniband/hw/ipath/ipath_driver.c
   I did an evaluation of the work required and its significant, discussions
   are ongoing about just removing the driver from v4.3.

When originally proposed __arch_phys_wc_add() Andy Luto ended up requesting to
consider instead using ioremap_nocache() and then set_memory_wc() but Toshi
noted to me that was not possible as set_memory_wc() cannot work on IO memory
because:

--
1. __pa(addr) returns a fake address since there is no direct map.
2. reserve_memtype() tracks regular memory and I/O memory differently.

For regular memory, set_memory_*() can modify WB with a new type since
reserve_memtype() does not track WB.  For I/O memory, reserve_memtype()
detects a conflict when a given type is different from a tracked type.
--

Given the changes above and the issue with set_memory_wc() another
possibility is to do overlapping ioremap calls but one issue there was
the aliasing possible issues and no one seemed to know WTF would happen
for sure. After considering the overlapping ioremap() call strategy,
since only 3 drivers were affected (we have fixed atyfb with
iorenmap_uc() solution) since it really was only 2 drivers that needed
a work around and they were both for old devices it seems me and Andy
preferred to just treat them specially and I guess what we decided
in the end to do was to just not add transient APIs [1] but instead
that boot thing this thread talks about and which got merged.

So in light of all this review now: I am not sure its worth to remove
the pat_enabled() check from arch_phys_wc_add() or to just warn about
it. If we really wanted to we could consider arch_phys_wc_add() and
deal with that this will not check for pat_enabled() and forces MTRR...
I think Andy Luto won't like that very much though ? I at least don't
like it since we did all this work to finally leave only 1 piece of
code with direct MTRR access... Seems a bit sad. Since ipath will
be removed we'd have only ivtv driver using this API, I am not sure if
its worth it.

Thoughts?

[0] http://lkml.kernel.org/r/[e-mail]
[1] http://lkml.kernel.org/r/[e-mail]

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis R. Rodriguez July 7, 2015, 6:53 a.m. UTC | #7

On Mon, Jul 6, 2015 at 5:44 PM, Luis R. Rodriguez <[e-mail]> wrote:
> If we really wanted to we could consider arch_phys_wc_add()

I mean adding a __arch_phys_wc_add() which does not check for pat_enabled().

> and
> deal with that this will not check for pat_enabled() and forces MTRR...
> I think Andy Luto won't like that very much though ? I at least don't
> like it since we did all this work to finally leave only 1 piece of
> code with direct MTRR access... Seems a bit sad. Since ipath will
> be removed we'd have only ivtv driver using this API, I am not sure if
> its worth it.

Since ipath is going away soon we'd just have one driver with the icky
#ifdef code. I'd rather see that and then require semantics / grammer
rules to require ioremap_wc() when used with arch_phys_wc_add(). I
don't think ivtv is worth to consider breaking the semantics and
requirements.

> Thoughts?

 Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ingo Molnar July 7, 2015, 7:03 a.m. UTC | #8

* Luis R. Rodriguez <[e-mail]> wrote:

> On Mon, Jul 6, 2015 at 5:44 PM, Luis R. Rodriguez <[e-mail]> wrote:
> > If we really wanted to we could consider arch_phys_wc_add()
> 
> I mean adding a __arch_phys_wc_add() which does not check for pat_enabled().
> 
> > and
> > deal with that this will not check for pat_enabled() and forces MTRR...
> > I think Andy Luto won't like that very much though ? I at least don't
> > like it since we did all this work to finally leave only 1 piece of
> > code with direct MTRR access... Seems a bit sad. Since ipath will
> > be removed we'd have only ivtv driver using this API, I am not sure if
> > its worth it.
> 
> Since ipath is going away soon we'd just have one driver with the icky
> #ifdef code. I'd rather see that and then require semantics / grammer
> rules to require ioremap_wc() when used with arch_phys_wc_add(). I don't think 
> ivtv is worth to consider breaking the semantics and requirements.

Ok, let's keep your original approach with the warning then.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 4cb365d..8b95eef 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -38,6 +38,8 @@ 
     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/fb.h>
@@ -1171,6 +1173,13 @@  static int ivtvfb_init_card(struct ivtv *itv)
 {
 	int rc;
 
+#ifdef CONFIG_X86_64
+	if (pat_enabled()) {
+		pr_warn("ivtvfb needs PAT disabled, boot with nopat kernel parameter\n");
+		return -ENODEV;
+	}
+#endif
+
 	if (itv->osd_info) {
 		IVTVFB_ERR("Card %d already initialised\n", ivtvfb_card_id);
 		return -EBUSY;
@@ -1265,12 +1274,6 @@  static int __init ivtvfb_init(void)
 	int registered = 0;
 	int err;
 
-#ifdef CONFIG_X86_64
-	if (WARN(pat_enabled(),
-		 "ivtvfb needs PAT disabled, boot with nopat kernel parameter\n")) {
-		return -ENODEV;
-	}
-#endif
 
 	if (ivtvfb_card_id < -1 || ivtvfb_card_id >= IVTV_MAX_CARDS) {
 		printk(KERN_ERR "ivtvfb:  ivtvfb_card_id parameter is out of range (valid range: -1 - %d)\n",