Date:	Thu, 11 Sep 2014 13:01:43 +0200
From:	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
To:	Marc Zyngier <marc.zyngier@....com>
Cc:	Russell King - ARM Linux <linux@....linux.org.uk>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Christoph Lameter <cl@...ux.com>,
	Mark Brown <broonie@...nel.org>,
	"linux-next@...r.kernel.org" <linux-next@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Tejun Heo <tj@...nel.org>,
	Kyungmin Park <kyungmin.park@...sung.com>
Subject: Re: linux-next: Tree for Sep 1


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@...ux.com>
> >>>
> >>> 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.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

--
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