by [email protected]

[permalink] [raw]

Subject: RE: [PATCH 3.19.y-ckt 173/251] clocksource: exynos_mct: Avoid blocking calls in the cpu hotplug notifier

From: Krzysztof Kozlowski <[email protected]> Sent: Thursday, July 16, 2015 5:52 PM
> To: Duan Fugang-B38611
> Cc: Krzysztof Kozlowski; Kamal Mostafa; [email protected]; linux-
> [email protected]; [email protected]; Damian Eppel;
> [email protected]; [email protected]; Thomas Gleixner;
> [email protected]; [email protected]; kernel-
> [email protected]
> Subject: Re: [PATCH 3.19.y-ckt 173/251] clocksource: exynos_mct: Avoid
> blocking calls in the cpu hotplug notifier
>
> 2015-07-16 17:14 GMT+09:00 Duan Andy <[email protected]>:
> > From: Krzysztof Kozlowski <[email protected]> Sent: Thursday,
> > July 16, 2015 2:56 PM
> >> To: Duan Fugang-B38611
> >> Cc: Kamal Mostafa; [email protected];
> >> [email protected]; [email protected];
> >> [email protected]; Damian Eppel; [email protected];
> >> [email protected]; Thomas Gleixner; linux-arm-
> >> [email protected]; [email protected]
> >> Subject: Re: [PATCH 3.19.y-ckt 173/251] clocksource: exynos_mct:
> >> Avoid blocking calls in the cpu hotplug notifier
> >>
> >> 2015-07-16 15:37 GMT+09:00 Duan Andy <[email protected]>:
> >> > From: Kamal Mostafa <[email protected]> Sent: Thursday, July 16,
> >> > 2015 9:08 AM
> >> >> To: [email protected]; [email protected]; kernel-
> >> >> [email protected]
> >> >> Cc: Kamal Mostafa; [email protected];
> >> >> [email protected]; Damian Eppel; [email protected]; Thomas
> >> >> Gleixner; linux-arm- [email protected];
> >> >> [email protected]
> >> >> Subject: [PATCH 3.19.y-ckt 173/251] clocksource: exynos_mct: Avoid
> >> >> blocking calls in the cpu hotplug notifier
> >> >>
> >> >> 3.19.8-ckt4 -stable review patch. If anyone has any objections,
> >> >> please let me know.
> >> >>
> >> >> ------------------
> >> >>
> >> >> From: Damian Eppel <[email protected]>
> >> >>
> >> >> commit 56a94f13919c0db5958611b388e1581b4852f3c9 upstream.
> >> >>
> >> >> Whilst testing cpu hotplug events on kernel configured with
> >> >> DEBUG_PREEMPT and DEBUG_ATOMIC_SLEEP we get following BUG message,
> >> >> caused by calling
> >> >> request_irq() and free_irq() in the context of hotplug
> >> >> notification (which is in this case atomic context).
> >> >>
> >> >> [ 40.785859] CPU1: Software reset
> >> >> [ 40.786660] BUG: sleeping function called from invalid context
> at
> >> >> mm/slub.c:1241
> >> >> [ 40.786668] in_atomic(): 1, irqs_disabled(): 128, pid: 0, name:
> >> >> swapper/1
> >> >> [ 40.786678] Preemption disabled at:[< (null)>] (null)
> >> >> [ 40.786681]
> >> >> [ 40.786692] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.19.0-
> rc4-
> >> >> 00024-g7dca860 #36
> >> >> [ 40.786698] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> >> >> [ 40.786728] [<c0014a00>] (unwind_backtrace) from [<c0011980>]
> >> >> (show_stack+0x10/0x14)
> >> >> [ 40.786747] [<c0011980>] (show_stack) from [<c0449ba0>]
> >> >> (dump_stack+0x70/0xbc)
> >> >> [ 40.786767] [<c0449ba0>] (dump_stack) from [<c00c6124>]
> >> >> (kmem_cache_alloc+0xd8/0x170)
> >> >> [ 40.786785] [<c00c6124>] (kmem_cache_alloc) from [<c005d6f8>]
> >> >> (request_threaded_irq+0x64/0x128)
> >> >> [ 40.786804] [<c005d6f8>] (request_threaded_irq) from [<c0350b8c>]
> >> >> (exynos4_local_timer_setup+0xc0/0x13c)
> >> >> [ 40.786820] [<c0350b8c>] (exynos4_local_timer_setup) from
> >> [<c0350ca8>]
> >> >> (exynos4_mct_cpu_notify+0x30/0xa8)
> >> >> [ 40.786838] [<c0350ca8>] (exynos4_mct_cpu_notify) from
> [<c003b330>]
> >> >> (notifier_call_chain+0x44/0x84)
> >> >> [ 40.786857] [<c003b330>] (notifier_call_chain) from [<c0022fd4>]
> >> >> (__cpu_notify+0x28/0x44)
> >> >> [ 40.786873] [<c0022fd4>] (__cpu_notify) from [<c0013714>]
> >> >> (secondary_start_kernel+0xec/0x150)
> >> >> [ 40.786886] [<c0013714>] (secondary_start_kernel) from
> [<40008764>]
> >> >> (0x40008764)
> >> >>
> >> >> Interrupts cannot be requested/freed in the CPU_STARTING/CPU_DYING
> >> >> notifications which run on the hotplugged cpu with interrupts and
> >> >> preemption disabled.
> >> >>
> >> >> To avoid the issue, request the interrupts for all possible cpus
> >> >> in the boot code. The interrupts are marked NO_AUTOENABLE to avoid
> >> >> a racy
> >> >> request_irq/disable_irq() sequence. The flag prevents the
> >> >> request_irq() code from enabling the interrupt immediately.
> >> >>
> >> >> The interrupt is then enabled in the CPU_STARTING notifier of the
> >> >> hotplugged cpu and again disabled with disable_irq_nosync() in the
> >> >> CPU_DYING notifier.
> >> >>
> >> >> [ tglx: Massaged changelog to match the patch ]
> >> >>
> >> >> Fixes: 7114cd749a12 ("clocksource: exynos_mct: use
> >> >> (request/free)_irq calls for local timer registration")
> >> >> Reported-by: Krzysztof Kozlowski <[email protected]>
> >> >> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> >> >> Tested-by: Krzysztof Kozlowski <[email protected]>
> >> >> Tested-by: Marcin Jabrzyk <[email protected]>
> >> >> Signed-off-by: Damian Eppel <[email protected]>
> >> >> Cc: [email protected]
> >> >> Cc: [email protected]
> >> >> Cc: [email protected]
> >> >> Cc: [email protected]
> >> >> Cc: [email protected]
> >> >> Link: http://lkml.kernel.org/r/1435324984-7328-1-git-send-email-
> >> >> [email protected]
> >> >> Signed-off-by: Thomas Gleixner <[email protected]>
> >> >> Signed-off-by: Kamal Mostafa <[email protected]>
> >> >> ---
> >> >> drivers/clocksource/exynos_mct.c | 43
> >> >> ++++++++++++++++++++++++++++------
> >> >> ------
> >> >> 1 file changed, 30 insertions(+), 13 deletions(-)
> >> >>
> >> >> diff --git a/drivers/clocksource/exynos_mct.c
> >> >> b/drivers/clocksource/exynos_mct.c
> >> >> index 83564c9..c844616 100644
> >> >> --- a/drivers/clocksource/exynos_mct.c
> >> >> +++ b/drivers/clocksource/exynos_mct.c
> >> >> @@ -466,15 +466,12 @@ static int exynos4_local_timer_setup(struct
> >> >> clock_event_device *evt)
> >> >> exynos4_mct_write(TICK_BASE_CNT, mevt->base +
> >> >> MCT_L_TCNTB_OFFSET);
> >> >>
> >> >> if (mct_int_type == MCT_INT_SPI) {
> >> >> - evt->irq = mct_irqs[MCT_L0_IRQ + cpu];
> >> >> - if (request_irq(evt->irq, exynos4_mct_tick_isr,
> >> >> - IRQF_TIMER | IRQF_NOBALANCING,
> >> >> - evt->name, mevt)) {
> >> >> - pr_err("exynos-mct: cannot register IRQ %d\n",
> >> >> - evt->irq);
> >> >> +
> >> >> + if (evt->irq == -1)
> >> >> return -EIO;
> >> >> - }
> >> >> - irq_force_affinity(mct_irqs[MCT_L0_IRQ + cpu],
> >> >> cpumask_of(cpu));
> >> >> +
> >> >> + irq_force_affinity(evt->irq, cpumask_of(cpu));
> >> >> + enable_irq(evt->irq);
> >> >> } else {
> >> >> enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0);
> >> >
> >> > In here, why not use enable_percpu_irq(evt->irq) ?
> >> >
> >> >> }
> >> >> @@ -487,10 +484,12 @@ static int exynos4_local_timer_setup(struct
> >> >> clock_event_device *evt) static void
> >> >> exynos4_local_timer_stop(struct clock_event_device *evt) {
> >> >> evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt);
> >> >> - if (mct_int_type == MCT_INT_SPI)
> >> >> - free_irq(evt->irq, this_cpu_ptr(&percpu_mct_tick));
> >> >> - else
> >> >> + if (mct_int_type == MCT_INT_SPI) {
> >> >> + if (evt->irq != -1)
> >> >> + disable_irq_nosync(evt->irq);
> >> >> + } else {
> >> >> disable_percpu_irq(mct_irqs[MCT_L0_IRQ]);
> >> >
> >> > In here, why not use disable_percpu_irq(evt->irq) ?
> >>
> >> You know this is just a semi-automatic stable backport and this was
> >> already merged to mainline?
> >>
> >> Best regards,
> >> Krzysztof
> >
> > Yes, I know. Do you know the reason why not enable other cpus irq ?
> > I want to confirm whether it is right after below change ?
> > enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0); ->
> > enable_percpu_irq(evt->irq, 0);
> > disable_percpu_irq(mct_irqs[MCT_L0_IRQ]) ->
> > disable_percpu_irq(evt->irq)
>
> This is a backport so are there any objections about this patch? Do you
> think the patch is invalid so it should not be backported to stable?
>
No, I just have some questions about the original code design.
Your patch has no problem.

Regards,
Andy
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?