| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090716175554.GE4791@ovro.caltech.edu>
Date: Thu, 16 Jul 2009 10:55:54 -0700
From: "Ira W. Snyder" <iws@...o.caltech.edu>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: dougthompson@...ssion.com, bluesmoke-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] edac: mpc85xx add mpc83xx support
On Wed, Jul 15, 2009 at 12:52:49PM -0700, Andrew Morton wrote:
> On Wed, 15 Jul 2009 11:38:49 -0600
> dougthompson@...ssion.com wrote:
>
> >
> > Add support for the Freescale MPC83xx memory controller to the existing
> > driver for the Freescale MPC85xx memory controller. The only difference
> > between the two processors are in the CS_BNDS register parsing code, which
> > has been changed so it will work on both processors.
> >
> > The L2 cache controller does not exist on the MPC83xx, but the OF subsystem
> > will not use the driver if the device is not present in the OF device tree.
> >
> >
> > Kumar, I had to change the nr_pages calculation to make the math work
> > out. I checked it on my board and did the math by hand for a 64GB 85xx
> > using 64K pages. In both cases, nr_pages * PAGE_SIZE comes out to the
> > correct value. Thanks for the help.
> >
> > v1 -> v2:
> > * Use PAGE_SHIFT to parse cs_bnds regardless of board type
> > * Remove special-casing for the 83xx processor
> >
> > ...
> >
> > @@ -789,19 +791,20 @@ static void __devinit mpc85xx_init_csrow
> > csrow = &mci->csrows[index];
> > cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
> > (index * MPC85XX_MC_CS_BNDS_OFS));
> > - start = (cs_bnds & 0xfff0000) << 4;
> > - end = ((cs_bnds & 0xfff) << 20);
> > - if (start)
> > - start |= 0xfffff;
> > - if (end)
> > - end |= 0xfffff;
> > +
> > + start = (cs_bnds & 0xffff0000) >> 16;
> > + end = (cs_bnds & 0x0000ffff);
> >
> > if (start == end)
> > continue; /* not populated */
> >
> > + start <<= (24 - PAGE_SHIFT);
> > + end <<= (24 - PAGE_SHIFT);
> > + end |= (1 << (24 - PAGE_SHIFT)) - 1;
>
> <stares for a while>
>
> That looks like the original code was really really wrong.
>
Looking at the original code again, I agree. It does look wrong. For the
new code, I've run the math by hand for a number of different cases:
83xx, 256MB of RAM, 4K pages (I've tested this on my board too)
85xx, 64GB of RAM, 4K pages
85xx, 64GB of RAM, 64K pages
Let me describe the CS_BNDS register layout for you:
85xx: 0x0XXX0YYY
X's: start address for chip select N: the 12msb's of the 36-bit address
Y's: end address for chip select N: the 12msb's of the 36-bit address
83xx: 0x00XX00YY
X's: start address for chip select N: the 8 msb's of the 32-bit address
Y's: end address for chip select N: the 8 msb's of the 32-bit address
So, on an 83xx with 256MB of RAM in a single bank:
CS_BNDS0 is 0x0000000f. 83xx uses 4K pages, so PAGE_SHIFT = 12
start = 0x0;
end = 0xf;
/* Now the shifts happen */
start == 0x0;
end == 0xffff;
The pfn for the csrow->first_page and csrow->last_page seem correct to
me. The csrow->nr_pages seems correct too: 0x10000 pages * 4K == 256MB.
> The setting of all the lower bits in `end' is funny-looking. What's
> happening here? Should it be commented?
>
That's exactly what's happening, the lower bits are all set. That's so
you can get the end address of the memory bank.
>
> > csrow->first_page = start >> PAGE_SHIFT;
> > csrow->last_page = end >> PAGE_SHIFT;
> > - csrow->nr_pages = csrow->last_page + 1 - csrow->first_page;
> > + csrow->nr_pages = end + 1 - start;
> > csrow->grain = 8;
> > csrow->mtype = mtype;
> > csrow->dtype = DEV_UNKNOWN;
> > @@ -985,6 +988,7 @@ static struct of_device_id mpc85xx_mc_er
> > { .compatible = "fsl,mpc8560-memory-controller", },
> > { .compatible = "fsl,mpc8568-memory-controller", },
> > { .compatible = "fsl,mpc8572-memory-controller", },
> > + { .compatible = "fsl,mpc8349-memory-controller", },
> > { .compatible = "fsl,p2020-memory-controller", },
> > {},
> > };
> > @@ -1001,13 +1005,13 @@ static struct of_platform_driver mpc85xx
> > },
> > };
> >
> > -
> > +#ifdef CONFIG_MPC85xx
> > static void __init mpc85xx_mc_clear_rfxe(void *data)
> > {
> > orig_hid1[smp_processor_id()] = mfspr(SPRN_HID1);
> > mtspr(SPRN_HID1, (orig_hid1[smp_processor_id()] & ~0x20000));
> > }
> > -
> > +#endif
> >
> > static int __init mpc85xx_mc_init(void)
> > {
> > @@ -1040,26 +1044,32 @@ static int __init mpc85xx_mc_init(void)
> > printk(KERN_WARNING EDAC_MOD_STR "PCI fails to register\n");
> > #endif
> >
> > +#ifdef CONFIG_MPC85xx
> > /*
> > * need to clear HID1[RFXE] to disable machine check int
> > * so we can catch it
> > */
> > if (edac_op_state == EDAC_OPSTATE_INT)
> > on_each_cpu(mpc85xx_mc_clear_rfxe, NULL, 0);
> > +#endif
> >
> > return 0;
> > }
>
> The patch adds lots of ifdefs :(
>
Yeah, it does. The HID1 register is a processor model specific register.
It isn't valid on the 83xx. The register exists, but the bits in it have
different meanings. I would have moved this code to the mc_probe()
routine, but I was advised against it.
> > module_init(mpc85xx_mc_init);
> >
> > +#ifdef CONFIG_MPC85xx
> > static void __exit mpc85xx_mc_restore_hid1(void *data)
> > {
> > mtspr(SPRN_HID1, orig_hid1[smp_processor_id()]);
> > }
> > +#endif
>
> afacit this will run smp_processor_id() from within preemptible code,
> which is often buggy on preemptible kernels and will cause runtime
> warnings on at least some architectures.
>
I don't know much about smp_processor_id(). You'll have to talk with
Kumar/Dave Jiang about the reasons for this code.
Hopefully that helps to clear up some of the concerns about the patch.
Thanks,
Ira
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists