Hi Bartlomiej, On 11/09/14 12:01, Bartlomiej Zolnierkiewicz wrote: > > Hi, > > On Wednesday, September 10, 2014 07:11:10 PM Marc Zyngier wrote: >> Hi Russell, >> >> On 10/09/14 18:41, Russell King - ARM Linux wrote: >>> 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. >> >> Thanks for looping me in. I indeed introduced this as an alternative to >> an utterly broken patch that was submitted at the time. >> >> As far as I can tell, and by reading your analysis, this patch looks >> perfectly sensible. >> >> Now, I have long given up on trying to run *anything* on a Samsung >> platform other than my Chromebook - the various maintainers don't seem >> to care at all. I may be able to revive an Origen board though (I think >> I have one collecting the proverbial dust in a cupboard), assuming I can >> locate a bootloader for it. > > Well, I'm not a maintainer but I try keep linux-next working on at least: > > Origen (Exynos4210) > Origen Quad (Exynos4412) > ODROID U3 (Exynos4412) > Trats2 (Exynos4412) > Arndale (Exynos5250) > > If you have problems booting linux-next on any of the above boards please > let me know. My first problem is getting mainline u-boot to work on the Origen (the 4210 flavour). I compiled "something", but how you get that to run is a mystery. If I can get that to work, then I'll try to move on to the kernel... Thanks, M. -- Jazz is not dead. It just smells funny... -- 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