* [PATCH v1] clk: mediatek: assign the initial value to clk_init_data of mtk_mux @ 2020-05-25 6:41 Weiyi Lu 2020-05-25 9:08 ` Matthias Brugger 0 siblings, 1 reply; 4+ messages in thread From: Weiyi Lu @ 2020-05-25 6:41 UTC (permalink / raw) To: Nicolas Boichat, Stephen Boyd Cc: James Liao, Fan Chen, linux-arm-kernel, linux-kernel, linux-mediatek, linux-clk, srv_heupstream, stable, Weiyi Lu, Owen Chen It'd be dangerous when struct clk_core have new memebers. Add the missing initial value to clk_init_data. Fixes: a3ae549917f1 ("clk: mediatek: Add new clkmux register API") Cc: <stable@vger.kernel.org> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com> --- drivers/clk/mediatek/clk-mux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c index 76f9cd0..14e127e 100644 --- a/drivers/clk/mediatek/clk-mux.c +++ b/drivers/clk/mediatek/clk-mux.c @@ -160,7 +160,7 @@ struct clk *mtk_clk_register_mux(const struct mtk_mux *mux, spinlock_t *lock) { struct mtk_clk_mux *clk_mux; - struct clk_init_data init; + struct clk_init_data init = {}; struct clk *clk; clk_mux = kzalloc(sizeof(*clk_mux), GFP_KERNEL); -- 1.8.1.1.dirty ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1] clk: mediatek: assign the initial value to clk_init_data of mtk_mux 2020-05-25 6:41 [PATCH v1] clk: mediatek: assign the initial value to clk_init_data of mtk_mux Weiyi Lu @ 2020-05-25 9:08 ` Matthias Brugger 2020-05-26 2:43 ` Weiyi Lu 0 siblings, 1 reply; 4+ messages in thread From: Matthias Brugger @ 2020-05-25 9:08 UTC (permalink / raw) To: Weiyi Lu, Nicolas Boichat, Stephen Boyd Cc: James Liao, srv_heupstream, linux-kernel, stable, Fan Chen, linux-mediatek, Owen Chen, linux-clk, linux-arm-kernel On 25/05/2020 08:41, Weiyi Lu wrote: > It'd be dangerous when struct clk_core have new memebers. > Add the missing initial value to clk_init_data. > Sorry I don't really understand this commit message, can please explain. In any case if this is a problem, then we probably we should fix it for all drivers. Apart from drivers/clk/mediatek/clk-cpumux.c It's a widely used pattern: $ git grep "struct clk_init_data init;"| wc -l 235 Regards, Matthias > Fixes: a3ae549917f1 ("clk: mediatek: Add new clkmux register API") > Cc: <stable@vger.kernel.org> > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com> > --- > drivers/clk/mediatek/clk-mux.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c > index 76f9cd0..14e127e 100644 > --- a/drivers/clk/mediatek/clk-mux.c > +++ b/drivers/clk/mediatek/clk-mux.c > @@ -160,7 +160,7 @@ struct clk *mtk_clk_register_mux(const struct mtk_mux *mux, > spinlock_t *lock) > { > struct mtk_clk_mux *clk_mux; > - struct clk_init_data init; > + struct clk_init_data init = {}; > struct clk *clk; > > clk_mux = kzalloc(sizeof(*clk_mux), GFP_KERNEL); > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] clk: mediatek: assign the initial value to clk_init_data of mtk_mux 2020-05-25 9:08 ` Matthias Brugger @ 2020-05-26 2:43 ` Weiyi Lu 2020-05-26 15:42 ` Matthias Brugger 0 siblings, 1 reply; 4+ messages in thread From: Weiyi Lu @ 2020-05-26 2:43 UTC (permalink / raw) To: Matthias Brugger Cc: Nicolas Boichat, Stephen Boyd, James Liao, srv_heupstream, linux-kernel, stable, Fan Chen, linux-mediatek, Owen Chen, linux-clk, linux-arm-kernel On Mon, 2020-05-25 at 11:08 +0200, Matthias Brugger wrote: > > On 25/05/2020 08:41, Weiyi Lu wrote: > > It'd be dangerous when struct clk_core have new memebers. > > Add the missing initial value to clk_init_data. > > > > Sorry I don't really understand this commit message, can please explain. > In any case if this is a problem, then we probably we should fix it for all drivers. > Apart from drivers/clk/mediatek/clk-cpumux.c > Actually, we were looking into an android kernel patch "ANDROID: GKI: clk: Add support for voltage voting" [1] In this patch, there adds a new member struct clk_vdd_class *vdd_class; in struct clk_init_data and struct clk_core And then in clk_register(...) core->vdd_class = hw->init->vdd_class; In many clock APIs, it will check the core->vdd_class to select the correct control flow. So, if we don't assign an initial value to clk_init_data of mtk_mux clock type, something might go wrong. And assigning an initial value might be the easiest and good way to avoid such problem if any new clock support added in the future. [1] https://android-review.googlesource.com/c/kernel/common/+/1278046 > It's a widely used pattern: > $ git grep "struct clk_init_data init;"| wc -l > 235 > > Regards, > Matthias > > > Fixes: a3ae549917f1 ("clk: mediatek: Add new clkmux register API") > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com> > > --- > > drivers/clk/mediatek/clk-mux.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c > > index 76f9cd0..14e127e 100644 > > --- a/drivers/clk/mediatek/clk-mux.c > > +++ b/drivers/clk/mediatek/clk-mux.c > > @@ -160,7 +160,7 @@ struct clk *mtk_clk_register_mux(const struct mtk_mux *mux, > > spinlock_t *lock) > > { > > struct mtk_clk_mux *clk_mux; > > - struct clk_init_data init; > > + struct clk_init_data init = {}; > > struct clk *clk; > > > > clk_mux = kzalloc(sizeof(*clk_mux), GFP_KERNEL); > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] clk: mediatek: assign the initial value to clk_init_data of mtk_mux 2020-05-26 2:43 ` Weiyi Lu @ 2020-05-26 15:42 ` Matthias Brugger 0 siblings, 0 replies; 4+ messages in thread From: Matthias Brugger @ 2020-05-26 15:42 UTC (permalink / raw) To: Weiyi Lu Cc: Nicolas Boichat, Stephen Boyd, James Liao, srv_heupstream, linux-kernel, stable, Fan Chen, linux-mediatek, Owen Chen, linux-clk, linux-arm-kernel Hi Weiyi, On 26/05/2020 04:43, Weiyi Lu wrote: > On Mon, 2020-05-25 at 11:08 +0200, Matthias Brugger wrote: >> >> On 25/05/2020 08:41, Weiyi Lu wrote: >>> It'd be dangerous when struct clk_core have new memebers. >>> Add the missing initial value to clk_init_data. >>> >> >> Sorry I don't really understand this commit message, can please explain. >> In any case if this is a problem, then we probably we should fix it for all drivers. >> Apart from drivers/clk/mediatek/clk-cpumux.c >> > > Actually, we were looking into an android kernel patch "ANDROID: GKI: > clk: Add support for voltage voting" [1] > > In this patch, there adds a new member struct clk_vdd_class *vdd_class; > in struct clk_init_data and struct clk_core > > And then in clk_register(...) > core->vdd_class = hw->init->vdd_class; > > In many clock APIs, it will check the core->vdd_class to select the > correct control flow. > So, if we don't assign an initial value to clk_init_data of mtk_mux > clock type, something might go wrong. And assigning an initial value > might be the easiest and good way to avoid such problem if any new clock > support added in the future. > Thanks for your explanation. You mean that as clk_init_data is on the stack it can have random values, and would lead to the fact that vdd_class is interpreted as allocated although it should be NULL. Sounds reasonable to me. You might want to resend with a better commit message. Feel free to add my Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com> Regards, Matthias > [1] https://android-review.googlesource.com/c/kernel/common/+/1278046 > >> It's a widely used pattern: >> $ git grep "struct clk_init_data init;"| wc -l >> 235 >> >> Regards, >> Matthias >> >>> Fixes: a3ae549917f1 ("clk: mediatek: Add new clkmux register API") >>> Cc: <stable@vger.kernel.org> >>> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com> >>> --- >>> drivers/clk/mediatek/clk-mux.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c >>> index 76f9cd0..14e127e 100644 >>> --- a/drivers/clk/mediatek/clk-mux.c >>> +++ b/drivers/clk/mediatek/clk-mux.c >>> @@ -160,7 +160,7 @@ struct clk *mtk_clk_register_mux(const struct mtk_mux *mux, >>> spinlock_t *lock) >>> { >>> struct mtk_clk_mux *clk_mux; >>> - struct clk_init_data init; >>> + struct clk_init_data init = {}; >>> struct clk *clk; >>> >>> clk_mux = kzalloc(sizeof(*clk_mux), GFP_KERNEL); >>> > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-26 15:42 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-25 6:41 [PATCH v1] clk: mediatek: assign the initial value to clk_init_data of mtk_mux Weiyi Lu 2020-05-25 9:08 ` Matthias Brugger 2020-05-26 2:43 ` Weiyi Lu 2020-05-26 15:42 ` Matthias Brugger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).