lkml.org 
[lkml]   [2014]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: linux-next: Tree for Sep 1
    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@linux.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.

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

    That's an alternative. I personally wouldn't shed a tear.

    Thanks,

    M.
    --
    Jazz is not dead. It just smells funny...



    \
     
     \ /
      Last update: 2014-09-10 20:41    [W:3.153 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site