On Fri, Sep 05, 2014 at 03:27:51PM -0400, Nicolas Pitre wrote:
> On Tue, 2 Sep 2014, Christoph Lameter wrote:
>
> > On Tue, 2 Sep 2014, Christoph Lameter wrote:
> >
> > > Oww.. This is double indirection deal there. A percpu offset pointing to
> > > a pointer?
> > >
> > > Generally the following is true (definition from
> > > include/asm-generic/percpu.h that is used for ARM for raw_cpu_read):
> > >
> > > #define raw_cpu_read_4(pcp) (*raw_cpu_ptr(&(pcp)))
> >
> > I think what the issue is that we dropped the fetch of the percpu offset
> > in the patch. Instead we are using the address of the variable that
> > contains the offset. Does this patch fix it?
> >
> >
> > Subject: irqchip: Properly fetch the per cpu offset
> >
> > The raw_cpu_read() conversion dropped the fetch of the offset
> > from base->percpu_base in gic_get_percpu_base.
> >
> > Signed-off-by: Christoph Lameter <cl@xxxxxxxxx>
> >
> > Index: linux/drivers/irqchip/irq-gic.c
> > ===================================================================
> > --- linux.orig/drivers/irqchip/irq-gic.c
> > +++ linux/drivers/irqchip/irq-gic.c
> > @@ -102,7 +102,7 @@ static struct gic_chip_data gic_data[MAX
> > #ifdef CONFIG_GIC_NON_BANKED
> > static void __iomem *gic_get_percpu_base(union gic_base *base)
> > {
> > - return raw_cpu_read(base->percpu_base);
> > + return raw_cpu_read(*base->percpu_base);
>
> Isn't the pointer dereference supposed to be performed _outside_ the per
> CPU accessor?
I think this is correct.
Let's start from the depths of raw_cpu_read(), where the pointer is
verified to be the correct type:
#define __verify_pcpu_ptr(ptr) \
do { \
const void __percpu *__vpp_verify = (typeof((ptr) + 0))NULL; \
(void)__vpp_verify; \
} while (0)
So, "ptr" should be of type "const void __percpu *" (note the __percpu
annotation there, which makes it sparse-checkable.)
The next level up is this:
#define __pcpu_size_call_return(stem, variable) \
({ \
typeof(variable) pscr_ret__; \
__verify_pcpu_ptr(&(variable)); \
So, we pass the address of the variable to the verification function.
That makes it a void-typed variable - "const void __percpu".
#define raw_cpu_read(pcp) __pcpu_size_call_return(raw_cpu_read_, pcp)
So this also makes "pcp" a "const void __percpu".
Now, what type is base->percpu_base?
void __percpu * __iomem *percpu_base;
The thing we want to be per-cpu is a "void __iomem *" pointer. However,
we have a pointer to the per-cpu instance. That's the "void __percpu *"
bit.
So, for this to match the requirements for raw_cpu_read(), we need to
do one dereference to end up with "void __percpu".
Hence, to me, the patch looks correct.
Whether it works or not is a /completely/ different matter. As has been
pointed out, the only place this code gets used is on a very small number
of platforms, which I don't have, and that gives me zero way to test it.
If it's Exynos which is affected by this, we need to call on Samsung to
test this patch.
Now, this code was introduced by Marc Zyngier in order to support Exynos,
probably the result of another patch on the mailing list from Samsung.
(I've added Marc and another Samsung guy to the Cc list.) Whatever,
*someone* needs to verify this but it needs to be done with the affected
hardware. Whether Marc can, or whether it has to be someone from Samsung,
I don't care which.
/Or/ we deem the code unmaintained, broken, and untestable, and we start
considering ripping it out of the mainline kernel on the basis that no
one cares about it anymore.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-next" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
