From: "Luis R. Rodriguez" <[email protected]>
The 0-day robot found that the notpat requirement warning was
being triggered on the ivtv driver on the module init path,
that will always trigger on built-in devices. We want that warning
to trigger only if real hardware is found so this moves the ivtv
warning out under its quasi-probe routine. The ipath driver already
had the warning issued on its probe so no shift of code is needed
there. Upon further thought though we decided WARN() messages would
confuse people so instead just change these to sensible single
pr_warn() messages for both drivers.
This goes build and load tested.
Luis R. Rodriguez (2):
x86/mm/pat, drivers/infiniband/ipath: replace WARN() with pr_warn()
x86/mm/pat, drivers/media/ivtv: move pat warn and replace WARN() with
pr_warn()
drivers/infiniband/hw/ipath/ipath_driver.c | 6 ++++--
drivers/media/pci/ivtv/ivtvfb.c | 15 +++++++++------
2 files changed, 13 insertions(+), 8 deletions(-)
--
2.3.2.209.gd67f9d5.dirty
From: "Luis R. Rodriguez" <[email protected]>
WARN() may confuse users, fix that. ipath_init_one() is part the
device's probe so this would only be triggered if a corresponding
device was found.
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/infiniband/hw/ipath/ipath_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/hw/ipath/ipath_driver.c b/drivers/infiniband/hw/ipath/ipath_driver.c
index 2d7e503..871dbe5 100644
--- a/drivers/infiniband/hw/ipath/ipath_driver.c
+++ b/drivers/infiniband/hw/ipath/ipath_driver.c
@@ -31,6 +31,8 @@
* SOFTWARE.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/sched.h>
#include <linux/spinlock.h>
#include <linux/idr.h>
@@ -399,8 +401,8 @@ static int ipath_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
u32 bar0 = 0, bar1 = 0;
#ifdef CONFIG_X86_64
- if (WARN(pat_enabled(),
- "ipath needs PAT disabled, boot with nopat kernel parameter\n")) {
+ if (pat_enabled()) {
+ pr_warn("ipath needs PAT disabled, boot with nopat kernel parameter\n");
ret = -ENODEV;
goto bail;
}
--
2.3.2.209.gd67f9d5.dirty
From: "Luis R. Rodriguez" <[email protected]>
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 <[email protected]> [0-day test robot]
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
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;
@@ -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
* Luis R. Rodriguez <[email protected]> wrote:
> From: "Luis R. Rodriguez" <[email protected]>
>
> WARN() may confuse users, fix that. ipath_init_one() is part the
> device's probe so this would only be triggered if a corresponding
> device was found.
>
> Signed-off-by: Luis R. Rodriguez <[email protected]>
> ---
> drivers/infiniband/hw/ipath/ipath_driver.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/hw/ipath/ipath_driver.c b/drivers/infiniband/hw/ipath/ipath_driver.c
> index 2d7e503..871dbe5 100644
> --- a/drivers/infiniband/hw/ipath/ipath_driver.c
> +++ b/drivers/infiniband/hw/ipath/ipath_driver.c
> @@ -31,6 +31,8 @@
> * SOFTWARE.
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/sched.h>
> #include <linux/spinlock.h>
> #include <linux/idr.h>
> @@ -399,8 +401,8 @@ static int ipath_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> u32 bar0 = 0, bar1 = 0;
>
> #ifdef CONFIG_X86_64
> - if (WARN(pat_enabled(),
> - "ipath needs PAT disabled, boot with nopat kernel parameter\n")) {
> + if (pat_enabled()) {
> + pr_warn("ipath needs PAT disabled, boot with nopat kernel parameter\n");
> ret = -ENODEV;
> goto bail;
> }
So driver init will always fail with this on modern kernels.
Btw., on a second thought, ipath uses MTRRs to enable WC:
ret = ipath_enable_wc(dd);
if (ret)
ret = 0;
Note how it ignores any failures - the driver still works even if WC was not
enabled.
So why don't you simply extend ipath_enable_wc() to generate the warning message
and return -EINVAL - which still keeps the driver working on modern kernels?
Just inform the user about 'nopat' if he wants WC for this driver.
Thanks,
Ingo
* Luis R. Rodriguez <[email protected]> wrote:
> From: "Luis R. Rodriguez" <[email protected]>
>
> 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 <[email protected]> [0-day test robot]
> Signed-off-by: Luis R. Rodriguez <[email protected]>
> ---
> 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
On Thu, Jun 25, 2015 at 08:49:22AM +0200, Ingo Molnar wrote:
>
> * Luis R. Rodriguez <[email protected]> wrote:
>
> > From: "Luis R. Rodriguez" <[email protected]>
> >
> > WARN() may confuse users, fix that. ipath_init_one() is part the
> > device's probe so this would only be triggered if a corresponding
> > device was found.
> >
> > Signed-off-by: Luis R. Rodriguez <[email protected]>
> > ---
> > drivers/infiniband/hw/ipath/ipath_driver.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/ipath/ipath_driver.c b/drivers/infiniband/hw/ipath/ipath_driver.c
> > index 2d7e503..871dbe5 100644
> > --- a/drivers/infiniband/hw/ipath/ipath_driver.c
> > +++ b/drivers/infiniband/hw/ipath/ipath_driver.c
> > @@ -31,6 +31,8 @@
> > * SOFTWARE.
> > */
> >
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > #include <linux/sched.h>
> > #include <linux/spinlock.h>
> > #include <linux/idr.h>
> > @@ -399,8 +401,8 @@ static int ipath_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> > u32 bar0 = 0, bar1 = 0;
> >
> > #ifdef CONFIG_X86_64
> > - if (WARN(pat_enabled(),
> > - "ipath needs PAT disabled, boot with nopat kernel parameter\n")) {
> > + if (pat_enabled()) {
> > + pr_warn("ipath needs PAT disabled, boot with nopat kernel parameter\n");
> > ret = -ENODEV;
> > goto bail;
> > }
>
> So driver init will always fail with this on modern kernels.
Nope, I double checked this, ipath_init_one() is the PCI probe routine,
not the module init call. It should probably be renamed.
> Btw., on a second thought, ipath uses MTRRs to enable WC:
>
> ret = ipath_enable_wc(dd);
> if (ret)
> ret = 0;
>
> Note how it ignores any failures - the driver still works even if WC was not
> enabled.
Ah, well WC strategy requires a split of the MMIO registers and the desired
WC area, right now they are combined for some type of ipath devices. There
are two things to consider when thinking about whether or not we want to
do the work required to do the split:
1) The state of affairs of the ipath driver
2) The effort required to do the ipath MMIO register / WC split
As for 1): the ipath driver is deprecated, the folks who maintain it
haven't used the driver in testing for 3-4 years now. The ipath driver
powers the old HTX bus card that only work in AMD systems, the replacement
driver is the qib infiniband driver , it powers all PCI-E cards. Doug
was even talking about removing the driver from the kernel [0] [1].
As for 2): I looked into doing the split and what makes it really
hard is that the ipath driver has a character device that is used
for diagnostics which lets userspace poke at the PCI device's
ioremap'd space, for the split case some magic needs to be done to
ensure the driver uses the right offset. So apart from addressing the
split and driver's use the character device mapping calls also would
need to be fixed.
I did the work on the atyfb driver to demo work effort required to
address the split, I did look to doing it for both the ipath and
ivtv driver but for both 1) and 2) indicated it was not worth it.
[0] http://lkml.kernel.org/r/[email protected]
[1] http://lkml.kernel.org/r/[email protected]
> So why don't you simply extend ipath_enable_wc() to generate the warning message
> and return -EINVAL - which still keeps the driver working on modern kernels?
We would need to do the split.
> Just inform the user about 'nopat' if he wants WC for this driver.
If we had the split we could do this.
Luis
On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
>
> * Luis R. Rodriguez <[email protected]> wrote:
>
> > From: "Luis R. Rodriguez" <[email protected]>
> >
> > 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 <[email protected]> [0-day test robot]
> > Signed-off-by: Luis R. Rodriguez <[email protected]>
> > ---
> > 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/[email protected]
[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
* Luis R. Rodriguez <[email protected]> wrote:
> On Thu, Jun 25, 2015 at 08:49:22AM +0200, Ingo Molnar wrote:
> >
> > * Luis R. Rodriguez <[email protected]> wrote:
> >
> > > From: "Luis R. Rodriguez" <[email protected]>
> > >
> > > WARN() may confuse users, fix that. ipath_init_one() is part the
> > > device's probe so this would only be triggered if a corresponding
> > > device was found.
> > >
> > > Signed-off-by: Luis R. Rodriguez <[email protected]>
> > > ---
> > > drivers/infiniband/hw/ipath/ipath_driver.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/ipath/ipath_driver.c b/drivers/infiniband/hw/ipath/ipath_driver.c
> > > index 2d7e503..871dbe5 100644
> > > --- a/drivers/infiniband/hw/ipath/ipath_driver.c
> > > +++ b/drivers/infiniband/hw/ipath/ipath_driver.c
> > > @@ -31,6 +31,8 @@
> > > * SOFTWARE.
> > > */
> > >
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > > #include <linux/sched.h>
> > > #include <linux/spinlock.h>
> > > #include <linux/idr.h>
> > > @@ -399,8 +401,8 @@ static int ipath_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> > > u32 bar0 = 0, bar1 = 0;
> > >
> > > #ifdef CONFIG_X86_64
> > > - if (WARN(pat_enabled(),
> > > - "ipath needs PAT disabled, boot with nopat kernel parameter\n")) {
> > > + if (pat_enabled()) {
> > > + pr_warn("ipath needs PAT disabled, boot with nopat kernel parameter\n");
> > > ret = -ENODEV;
> > > goto bail;
> > > }
> >
> > So driver init will always fail with this on modern kernels.
>
> Nope, I double checked this, ipath_init_one() is the PCI probe routine,
> not the module init call. It should probably be renamed.
>
> > Btw., on a second thought, ipath uses MTRRs to enable WC:
> >
> > ret = ipath_enable_wc(dd);
> > if (ret)
> > ret = 0;
> >
> > Note how it ignores any failures - the driver still works even if WC was not
> > enabled.
>
> Ah, well WC strategy requires a split of the MMIO registers and the desired
> WC area, right now they are combined for some type of ipath devices. There
> are two things to consider when thinking about whether or not we want to
> do the work required to do the split:
But ... why doing the 'split'?
With my suggested approach the driver will behave in two ways:
- if booted with 'nopat' it will behave as always and have the WC MTRR entries
added
- if booted with a modern kernel without 'nopat' then instead of getting WC MTRR
entries it will not get them - we'll fall back to UC. No 'split' or any other
change is needed to the driver AFAICS: it might be slower, but it will still
be functional. It will _not_ get PAT WC mappings - it will fall back to UC -
which is still much better for any potential user than not working at all.
Same suggestion for the other affected driver.
what am I missing?
Thanks,
Ingo
* Luis R. Rodriguez <[email protected]> wrote:
> On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> >
> > * Luis R. Rodriguez <[email protected]> wrote:
> >
> > > From: "Luis R. Rodriguez" <[email protected]>
> > >
> > > 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 <[email protected]> [0-day test robot]
> > > Signed-off-by: Luis R. Rodriguez <[email protected]>
> > > ---
> > > 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
On Fri, 2015-06-26 at 10:45 +0200, Ingo Molnar wrote:
> * Luis R. Rodriguez <[email protected]> wrote:
>
> > On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> > >
> > > * Luis R. Rodriguez <[email protected]> wrote:
> > >
> > > > From: "Luis R. Rodriguez" <[email protected]>
> > > >
> > > > 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 <[email protected]> [0-day test robot]
> > > > Signed-off-by: Luis R. Rodriguez <[email protected]>
> > > > ---
> > > > 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
* Andy Walls <[email protected]> wrote:
> On Fri, 2015-06-26 at 10:45 +0200, Ingo Molnar wrote:
> > * Luis R. Rodriguez <[email protected]> wrote:
> >
> > > On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> > > >
> > > > * Luis R. Rodriguez <[email protected]> wrote:
> > > >
> > > > > From: "Luis R. Rodriguez" <[email protected]>
> > > > >
> > > > > 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 <[email protected]> [0-day test robot]
> > > > > Signed-off-by: Luis R. Rodriguez <[email protected]>
> > > > > ---
> > > > > 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
On Mon, Jun 29, 2015 at 05:52:18AM -0400, Andy Walls wrote:
> On June 29, 2015 2:55:05 AM EDT, Ingo Molnar <[email protected]> wrote:
> >
> >* Andy Walls <[email protected]> wrote:
> >
> >> On Fri, 2015-06-26 at 10:45 +0200, Ingo Molnar wrote:
> >> > * Luis R. Rodriguez <[email protected]> wrote:
> >> >
> >> > > On Thu, Jun 25, 2015 at 08:51:47AM +0200, Ingo Molnar wrote:
> >> > > >
> >> > > > * Luis R. Rodriguez <[email protected]> wrote:
> >> > > >
> >> > > > > From: "Luis R. Rodriguez" <[email protected]>
> >> > > > >
> >> > > > > 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 <[email protected]> [0-day
> >test robot]
> >> > > > > Signed-off-by: Luis R. Rodriguez <[email protected]>
> >> > > > > ---
> >> > > > > 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/[email protected]
[1] http://lkml.kernel.org/r/CALCETrVY-+aZU0mTp+BUtzTiWq8cL_rK7hymtUpwyLhyRaouZA@mail.gmail.com
Luis
On Mon, Jul 6, 2015 at 5:44 PM, Luis R. Rodriguez <[email protected]> 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
* Luis R. Rodriguez <[email protected]> wrote:
> On Mon, Jul 6, 2015 at 5:44 PM, Luis R. Rodriguez <[email protected]> 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