Martin Peres
2014-Mar-25 22:11 UTC
[Nouveau] [PATCH 4/4] vbios/prom: fetch the vbios using only aligned 32-bit accesses
Other kind of accesses are unreliable on Maxwell cards. As advised by NVIDIA,
let's only use 32-bit accesses to fetch the vbios from PROM.
This fixes vbios fetching on my nve7 which failed in certain specific
conditions.
I suggest we Cc stable, for all kernels they still maintain after the big
rewrite.
Suggested-by: Christian Zander <czander at nvidia.com>
Signed-off-by: Martin Peres <martin.peres at free.fr>
---
nvkm/subdev/bios/base.c | 35 +++++++++++++++++++++++------------
1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/nvkm/subdev/bios/base.c b/nvkm/subdev/bios/base.c
index 5608530..baa5687 100644
--- a/nvkm/subdev/bios/base.c
+++ b/nvkm/subdev/bios/base.c
@@ -157,6 +157,10 @@ nouveau_bios_shadow_prom(struct nouveau_bios *bios)
pcireg = 0x001850;
access = nv_mask(bios, pcireg, 0x00000001, 0x00000000);
+ /* WARNING: PROM accesses should always be 32-bits aligned. Other
+ * accesses work on most chipset but do not on Kepler chipsets
+ */
+
/* bail if no rom signature, with a workaround for a PROM reading
* issue on some chipsets. the first read after a period of
* inactivity returns the wrong result, so retry the first header
@@ -164,31 +168,38 @@ nouveau_bios_shadow_prom(struct nouveau_bios *bios)
*/
i = 16;
do {
- if (nv_rd08(bios, 0x300000) == 0x55)
+ if ((nv_rd32(bios, 0x300000) & 0xffff) == 0xaa55)
break;
} while (i--);
- if (!i || nv_rd08(bios, 0x300001) != 0xaa)
+ if (!i)
goto out;
- /* additional check (see note below) - read PCI record header */
- pcir = nv_rd08(bios, 0x300018) |
- nv_rd08(bios, 0x300019) << 8;
- if (nv_rd08(bios, 0x300000 + pcir) != 'P' ||
- nv_rd08(bios, 0x300001 + pcir) != 'C' ||
- nv_rd08(bios, 0x300002 + pcir) != 'I' ||
- nv_rd08(bios, 0x300003 + pcir) != 'R')
+ /* check the PCI record header ("PCIR") if its address is aligned */
+ pcir = nv_rd32(bios, 0x300018) & 0xffff;
+ if ((pcir % 4) == 0 && nv_rd32(bios, 0x300000 + pcir) != 0x52494350)
goto out;
/* read entire bios image to system memory */
- bios->size = nv_rd08(bios, 0x300002) * 512;
+ bios->size = ((nv_rd32(bios, 0x300000) >> 16) & 0xff) * 512;
if (!bios->size)
goto out;
bios->data = kmalloc(bios->size, GFP_KERNEL);
if (bios->data) {
- for (i = 0; i < bios->size; i++)
- nv_wo08(bios, i, nv_rd08(bios, 0x300000 + i));
+ for (i = 0; i < bios->size; i+=4)
+ nv_wo32(bios, i, nv_rd32(bios, 0x300000 + i));
+ }
+
+ /* check the PCI record header again, now that we can make
+ * un-aligned accesses
+ */
+ if (bios->data[pcir + 0] != 'P' ||
+ bios->data[pcir + 1] != 'C' ||
+ bios->data[pcir + 2] != 'I' ||
+ bios->data[pcir + 3] != 'R') {
+ bios->size = 0;
+ kfree(bios->data);
}
out:
--
1.9.1
Ilia Mirkin
2014-Mar-25 22:24 UTC
[Nouveau] [PATCH 4/4] vbios/prom: fetch the vbios using only aligned 32-bit accesses
On Tue, Mar 25, 2014 at 6:11 PM, Martin Peres <martin.peres at free.fr> wrote:> Other kind of accesses are unreliable on Maxwell cards. As advised by NVIDIA,Maxwell or Kepler?> let's only use 32-bit accesses to fetch the vbios from PROM. > > This fixes vbios fetching on my nve7 which failed in certain specific > conditions. > > I suggest we Cc stable, for all kernels they still maintain after the big > rewrite.Just throw in the Cc :) Easier to just have it there than to have to go in and edit the commit description to remove this comment and add the Cc line...> > Suggested-by: Christian Zander <czander at nvidia.com> > Signed-off-by: Martin Peres <martin.peres at free.fr> > --- > nvkm/subdev/bios/base.c | 35 +++++++++++++++++++++++------------ > 1 file changed, 23 insertions(+), 12 deletions(-) > > diff --git a/nvkm/subdev/bios/base.c b/nvkm/subdev/bios/base.c > index 5608530..baa5687 100644 > --- a/nvkm/subdev/bios/base.c > +++ b/nvkm/subdev/bios/base.c > @@ -157,6 +157,10 @@ nouveau_bios_shadow_prom(struct nouveau_bios *bios) > pcireg = 0x001850; > access = nv_mask(bios, pcireg, 0x00000001, 0x00000000); > > + /* WARNING: PROM accesses should always be 32-bits aligned. Other > + * accesses work on most chipset but do not on Kepler chipsets > + */ > + > /* bail if no rom signature, with a workaround for a PROM reading > * issue on some chipsets. the first read after a period of > * inactivity returns the wrong result, so retry the first header > @@ -164,31 +168,38 @@ nouveau_bios_shadow_prom(struct nouveau_bios *bios) > */ > i = 16; > do { > - if (nv_rd08(bios, 0x300000) == 0x55) > + if ((nv_rd32(bios, 0x300000) & 0xffff) == 0xaa55) > break; > } while (i--); > > - if (!i || nv_rd08(bios, 0x300001) != 0xaa) > + if (!i) > goto out; > > - /* additional check (see note below) - read PCI record header */ > - pcir = nv_rd08(bios, 0x300018) | > - nv_rd08(bios, 0x300019) << 8; > - if (nv_rd08(bios, 0x300000 + pcir) != 'P' || > - nv_rd08(bios, 0x300001 + pcir) != 'C' || > - nv_rd08(bios, 0x300002 + pcir) != 'I' || > - nv_rd08(bios, 0x300003 + pcir) != 'R') > + /* check the PCI record header ("PCIR") if its address is aligned */ > + pcir = nv_rd32(bios, 0x300018) & 0xffff; > + if ((pcir % 4) == 0 && nv_rd32(bios, 0x300000 + pcir) != 0x52494350) > goto out;So if pcir % 4 != 0, it's all OK? Perhaps you meant if (pcir & 0x3 || nv_rd32(...) != ...) goto out> > /* read entire bios image to system memory */ > - bios->size = nv_rd08(bios, 0x300002) * 512; > + bios->size = ((nv_rd32(bios, 0x300000) >> 16) & 0xff) * 512;You already read this out once, might as well save it. BTW, is CPU endianness affected here? i.e. does nv_rd32 do a le32 -> be32 conversion on a be cpu? I would assume it must since everything assumes that 32-bit reads read in integer values... but then your logic is all wrong, since the bytes are laid out in a different order on be.> if (!bios->size) > goto out; > > bios->data = kmalloc(bios->size, GFP_KERNEL); > if (bios->data) { > - for (i = 0; i < bios->size; i++) > - nv_wo08(bios, i, nv_rd08(bios, 0x300000 + i)); > + for (i = 0; i < bios->size; i+=4) > + nv_wo32(bios, i, nv_rd32(bios, 0x300000 + i)); > + } > + > + /* check the PCI record header again, now that we can make > + * un-aligned accessesWhy bother with the two separate mechanisms? In practice, is PCIR ever at a non-mod4 location? (Should be easy to check our bios collection...)> + */ > + if (bios->data[pcir + 0] != 'P' || > + bios->data[pcir + 1] != 'C' || > + bios->data[pcir + 2] != 'I' || > + bios->data[pcir + 3] != 'R') { > + bios->size = 0; > + kfree(bios->data); > } > > out: > -- > 1.9.1 > > _______________________________________________ > Nouveau mailing list > Nouveau at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/nouveau
Martin Peres
2014-Mar-25 22:30 UTC
[Nouveau] [PATCH 4/4] vbios/prom: fetch the vbios using only aligned 32-bit accesses
On 25/03/2014 23:24, Ilia Mirkin wrote:> On Tue, Mar 25, 2014 at 6:11 PM, Martin Peres <martin.peres at free.fr> wrote: >> Other kind of accesses are unreliable on Maxwell cards. As advised by NVIDIA, > Maxwell or Kepler?Damn, I meant Kepler. I updated the patch in my git tree: http://cgit.freedesktop.org/~mperes/nouveau/commit/?id=661a5c599565686f72e729600b0dd6aa6e472f0c> >> let's only use 32-bit accesses to fetch the vbios from PROM. >> >> This fixes vbios fetching on my nve7 which failed in certain specific >> conditions. >> >> I suggest we Cc stable, for all kernels they still maintain after the big >> rewrite. > Just throw in the Cc :) Easier to just have it there than to have to > go in and edit the commit description to remove this comment and add > the Cc line...I don't want git send-email to send an email to stable, the patch won't apply on any kernel.