[RESEND PATCH v2 2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()
From: "Luis R. Rodriguez"
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 [0-day test robot]
Signed-off-by: Luis R. Rodriguez
---
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 4cb365d4ffdc..8b95eefb610b 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
#include
#include
@@ -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",
--
2.3.2.209.gd67f9d5.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()
* Luis R. Rodriguez wrote: > On Mon, Jul 6, 2015 at 5:44 PM, Luis R. Rodriguez 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-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()
On Mon, Jul 6, 2015 at 5:44 PM, Luis R. Rodriguez 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-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()
On Mon, Jun 29, 2015 at 05:52:18AM -0400, Andy Walls wrote:
> On June 29, 2015 2:55:05 AM EDT, Ingo Molnar wrote:
> >
> >* Andy Walls wrote:
> >
> >> On Fri, 2015-06-26 at 10:45 +0200, Ingo Molnar wrote:
> >> > * Luis R. Rodriguez wrote:
> >> >
> >> > > On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> >> > > >
> >> > > > * Luis R. Rodriguez wrote:
> >> > > >
> >> > > > > From: "Luis R. Rodriguez"
> >> > > > >
> >> > > > > 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 [0-day
> >test robot]
> >> > > > > Signed-off-by: Luis R. Rodriguez
> >> > > > > ---
> >> > > > > 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
> >> > > > > #include
> >> > > > > #include
> >> > > > > @@ -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 devi
Re: [PATCH v2 2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()
* Andy Walls wrote:
> On Fri, 2015-06-26 at 10:45 +0200, Ingo Molnar wrote:
> > * Luis R. Rodriguez wrote:
> >
> > > On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> > > >
> > > > * Luis R. Rodriguez wrote:
> > > >
> > > > > From: "Luis R. Rodriguez"
> > > > >
> > > > > 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 [0-day test robot]
> > > > > Signed-off-by: Luis R. Rodriguez
> > > > > ---
> > > > > 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
> > > > > #include
> > > > > #include
> > > > > @@ -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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()
On Fri, 2015-06-26 at 10:45 +0200, Ingo Molnar wrote:
> * Luis R. Rodriguez wrote:
>
> > On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> > >
> > > * Luis R. Rodriguez wrote:
> > >
> > > > From: "Luis R. Rodriguez"
> > > >
> > > > 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 [0-day test robot]
> > > > Signed-off-by: Luis R. Rodriguez
> > > > ---
> > > > 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
> > > > #include
> > > > #include
> > > > @@ -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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()
* Luis R. Rodriguez wrote:
> On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> >
> > * Luis R. Rodriguez wrote:
> >
> > > From: "Luis R. Rodriguez"
> > >
> > > 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 [0-day test robot]
> > > Signed-off-by: Luis R. Rodriguez
> > > ---
> > > 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
> > > #include
> > > #include
> > > @@ -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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()
On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
>
> * Luis R. Rodriguez wrote:
>
> > From: "Luis R. Rodriguez"
> >
> > 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 [0-day test robot]
> > Signed-off-by: Luis R. Rodriguez
> > ---
> > 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
> > #include
> > #include
> > @@ -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/1429146457.1899.99.ca...@palomino.walls.org
[1] https://marc.info/?t=14289474115&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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()
* Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez"
>
> 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 [0-day test robot]
> Signed-off-by: Luis R. Rodriguez
> ---
> 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
> #include
> #include
> @@ -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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with pr_warn()
From: "Luis R. Rodriguez"
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 [0-day test robot]
Signed-off-by: Luis R. Rodriguez
---
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
#include
#include
@@ -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",
--
2.3.2.209.gd67f9d5.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
