Hi,
On Thu, May 23, 2019 at 11:05 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Doug Anderson (2019-05-23 09:38:13)
> > Hi,
> >
> > On Tue, Apr 30, 2019 at 9:38 PM Bjorn Andersson
> > <bjorn.andersson@xxxxxxxxxx> wrote:
> >
> > > +static int qmp_qdss_clk_add(struct qmp *qmp)
> > > +{
> > > + struct clk_init_data qdss_init = {
> > > + .ops = &qmp_qdss_clk_ops,
> > > + .name = "qdss",
> > > + };
> >
> > Can't qdss_init be "static const"? That had the advantage of not
> > needing to construct it on the stack and also of it having a longer
> > lifetime. It looks like clk_register() stores the "hw" pointer in its
> > structure and the "hw" structure will have a pointer here. While I
> > can believe that it never looks at it again, it's nice if that pointer
> > doesn't point somewhere on an old stack.
> >
> > I suppose we could go the other way and try to mark more stuff in this
> > module as __init and __initdata, but even then at least the pointer
> > won't be onto a stack. ;-)
> >
>
> Const would be nice, but otherwise making it static isn't a good idea.
Even aside from the whole "not having it store a pointer to the
stack", "static const" is likely to reduce overall memory consumption
/ number of instructions by a tiny bit because we don't need to copy
this structure onto the stack--we can just use it in place.
As written (or by just adding const but not static const): qmp_probe()
is 1840 bytes long.
...and has this snippet:
0xffffff80084a58d4 <+1152>: adrp x1, 0xffffff8008a5b000
<video_cc_sdm845_match_table+280>
0xffffff80084a58d8 <+1156>: add x1, x1, #0x600
0xffffff80084a58dc <+1160>: add x0, sp, #0x10
0xffffff80084a58e0 <+1164>: mov w2, #0x28 // #40
0xffffff80084a58e4 <+1168>: add x22, sp, #0x10
0xffffff80084a58e8 <+1172>: bl 0xffffff800896e800 <memcpy>
With this as static const: qmp_probe is 1820 bytes long.
...and has this snippet:
0xffffff80084a58dc <+1160>: adrp x8, 0xffffff8008a5b000
<video_cc_sdm845_match_table+280>
0xffffff80084a58e0 <+1164>: add x8, x8, #0x550
> The clk_init_data structure is all copied over, although we do leave a
> dangling pointer to it stored inside the clk_hw structure we don't use
> it after clk registration. Maybe we should overwrite the pointer with
> NULL once we're done in clk_register() so that clk providers can't use
> it. It might break somebody but would at least clarify this point.
Setting it to NULL seems like it would be a good idea. Now that I
think on it I believe I've actually tripped over this before trying to
read the '.name' from here... :-P
> > > +static void qmp_pd_remove(struct qmp *qmp)
> > > +{
> > > + struct genpd_onecell_data *data = &qmp->pd_data;
> > > + struct device *dev = qmp->dev;
> > > + int i;
> > > +
> > > + of_genpd_del_provider(dev->of_node);
> > > +
> > > + for (i = 0; i < data->num_domains; i++)
> > > + pm_genpd_remove(data->domains[i]);
> >
> > Still feels like the above loop would be better as:
> > for (i = data->num_domains - 1; i >= 0; i--)
> >
>
> Reason being to remove in reverse order? Otherwise this looks like an
> opinion.
1. Matches the order of the error handling case above (see unroll_genpds label)
2. In general you avoid more unexpected problems by un-initting in the
reverse order you initted.
-Doug
