Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1596767378-27241-1-git-send-email-zhaoqianligood@gmail.com> <87h7t6lt76.fsf@nanos.tec.linutronix.de>
In-Reply-To: <87h7t6lt76.fsf@nanos.tec.linutronix.de>
From:   qianli zhao <zhaoqianligood@gmail.com>
Date:   Thu, 13 Aug 2020 23:17:16 +0800
Message-ID: <CAPx_LQEPRk1UnOkfPaU_Lfp-nQFe9bybeLFJ3X4wybGMzkhtmQ@mail.gmail.com>
Subject: Re: [PATCH] timer: mask unnecessary set of flags in do_init_timer
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     John Stultz <john.stultz@linaro.org>, sboyd@kernel.org,
        linux-kernel@vger.kernel.org, zhaoqianli@xiaomi.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thomas Gleixner <tglx@linutronix.de> =E4=BA=8E2020=E5=B9=B48=E6=9C=8813=E6=
=97=A5=E5=91=A8=E5=9B=9B =E4=B8=8B=E5=8D=886:46=E5=86=99=E9=81=93=EF=BC=9A
>
> Qianli Zhao <zhaoqianligood@gmail.com> writes:
>
> Please start the first word after the colon with an upper case letter.
>
> > do_init_timer can specify flags of timer_list,
>
> Please write do_init_timer() so it's entirely clear that this is about a
> function.
>
> > but this function does not expect to specify the CPU or idx.
>
> or idx does not mean anything unless someone looks at the
> code. Changelogs want to explain things so they can be understood
> without staring at the code.

will update changelog

> > If user invoking do_init_timer and specify CPU,
> > The result may not what we expected.
>
> Right. And which caller exactly hands in crappy flags?

This change is more of a sanity check to avoid these wrong use

> > E.g:
> > do_init_timer invoked in core2,and specify flags 0x1
> > final result flags is 0x3.If the specified CPU number
> > exceeds the actual number,more serious problems will happen
>
> More serious problems is not a really helpful technical explanation and
> 0x3 does not make sense for a changelog reader either because it again
> requires to look up the code.
>
> >       timer->entry.pprev =3D NULL;
> >       timer->function =3D func;
> > -     timer->flags =3D flags | raw_smp_processor_id();
> > +     timer->flags =3D (flags & ~TIMER_BASEMASK & ~TIMER_ARRAYMASK) |
> > raw_smp_processor_id();
>
> If the caller hands in invalid flags then silently fixing them up is
> fundamentally wrong. So this wants to be:
>
>    if (WARN_ON(flags & ~TIMER_INIT_FLAGS))
>         flags &=3D TIMER_INIT_FLAGS;
>
> and TIMER_INIT_FLAGS wants to be exactly the set of flags which are
> valid for being handed in by a caller, i.e.:
>
>       TIMER_DEFFERABLE, TIMER_PINNED, TIMER_IRQSAFE

This change is very good=EF=BC=8Cthanks for teaching

> Guess what happens when the caller hands in TIMER_MIGRATING?

If TIMER_MIGRATING is set in timer_setup, lock_timer_base will fall
into a dead loop

> If we do sanity checking then we do it correct and not just silently
> papering over the particular problem which you ran into.

Thanks for teaching.

I have updated patchset,please review.

> Thanks,
>
>         tglx