Subject: Re: [PATCH v5 2/6] powercap/drivers/dtpm: Add hierarchy creation
To:     Ulf Hansson <ulf.hansson@linaro.org>
Cc:     rjw@rjwysocki.net, lukasz.luba@arm.com, robh@kernel.org,
        heiko@sntech.de, arnd@linaro.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, "Rafael J. Wysocki" <rafael@kernel.org>,
        Daniel Lezcano <daniel.lezcano@kernel.org>
References: <20211218130014.4037640-1-daniel.lezcano@linaro.org>
 <20211218130014.4037640-3-daniel.lezcano@linaro.org>
 <CAPDyKFrLTsUxG8JHdK33h2BT8pxeHk6kiU-4uGrvxUhcQKg3Sw@mail.gmail.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
Message-ID: <8fcc0ef8-b0c7-da73-434f-31c88896aed5@linaro.org>
Date:   Wed, 5 Jan 2022 17:00:16 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <CAPDyKFrLTsUxG8JHdK33h2BT8pxeHk6kiU-4uGrvxUhcQKg3Sw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 31/12/2021 14:45, Ulf Hansson wrote:

[ ... ]

>> +static struct dtpm *dtpm_setup_dt(const struct dtpm_node *hierarchy,
>> +                                 struct dtpm *parent)
>> +{
>> +       struct dtpm_descr *dtpm_descr;
>> +       struct device_node *np;
>> +       int ret;
>> +
>> +       np = of_find_node_by_path(hierarchy->name);
>> +       if (!np) {
>> +               pr_err("Failed to find '%s'\n", hierarchy->name);
>> +               return ERR_PTR(-ENXIO);
>> +       }
>> +
>> +       for_each_dtpm_table(dtpm_descr) {
>> +
>> +               ret = dtpm_descr->setup(parent, np);
> 
> This will unconditionally call the ->setup callback() for each dtpm
> desc in the dtpm table. At this point the ->setup() callback has not
> been assigned by anyone that uses DTPM_DECLARE(), so if this would be
> called, it would trigger a NULL pointer dereference error.
> 
> On the other hand, we don't have someone calling
> dtpm_create_hierarchy() yet, so this code doesn't get exercised, but

Yes, that is the reason why the test is not here.

> it still looks a bit odd to me. Maybe squashing patch2 and patch3 is
> an option?
Sure

>> +               if (ret) {
>> +                       pr_err("Failed to setup '%s': %d\n", hierarchy->name, ret);
>> +                       of_node_put(np);
>> +                       return ERR_PTR(ret);
>> +               }
>> +
>> +               of_node_put(np);
> 
> This will be called for every loop in the dtpm table. This is wrong,
> you only want to call it once, outside the loop.

Right, good catch

>> +       }
>> +
>> +       /*
>> +        * By returning a NULL pointer, we let know the caller there
>> +        * is no child for us as we are a leaf of the tree
>> +        */
>> +       return NULL;
>> +}
>> +
>> +typedef struct dtpm * (*dtpm_node_callback_t)(const struct dtpm_node *, struct dtpm *);
>> +
>> +dtpm_node_callback_t dtpm_node_callback[] = {
>> +       [DTPM_NODE_VIRTUAL] = dtpm_setup_virtual,
>> +       [DTPM_NODE_DT] = dtpm_setup_dt,
>> +};
>> +
>> +static int dtpm_for_each_child(const struct dtpm_node *hierarchy,
>> +                              const struct dtpm_node *it, struct dtpm *parent)
>> +{
>> +       struct dtpm *dtpm;
>> +       int i, ret;
>> +
>> +       for (i = 0; hierarchy[i].name; i++) {
>> +
>> +               if (hierarchy[i].parent != it)
>> +                       continue;
>> +
>> +               dtpm = dtpm_node_callback[hierarchy[i].type](&hierarchy[i], parent);
>> +               if (!dtpm || IS_ERR(dtpm))
>> +                       continue;
>> +
>> +               ret = dtpm_for_each_child(hierarchy, &hierarchy[i], dtpm);
> 
> Why do you need to recursively call dtpm_for_each_child() here?
> 
> Is there a restriction on how the dtpm core code manages adding
> children/parents?

[ ... ]

The recursive call is needed given the structure of the tree in an array
in order to connect with the parent.


>> + *
>> + * struct dtpm_node hierarchy[] = {
>> + *     [0] { .name = "topmost" },
> 
> For clarity, I think we should also specify DTPM_NODE_VIRTUAL here.
> 
>> + *      [1] { .name = "package", .parent = &hierarchy[0] },
> 
> Ditto.

Sure

[ ... ]

>> +static int __init init_dtpm(void)
>> +{
>>         pct = powercap_register_control_type(NULL, "dtpm", NULL);
>>         if (IS_ERR(pct)) {
>>                 pr_err("Failed to register control type\n");
>>                 return PTR_ERR(pct);
>>         }
> 
> It looks like powercap_register_control_type() should be able to be
> called from dtpm_create_hierarchy(). In this way we can simply drop
> the initcall below, altogether.
>
> Of course, that assumes that dtpm_create_hierachy() is being called
> from a regular module_platform_driver() path - or at least from a
> later initcall than fs_initcall(), which is when the "powercap_class"
> is being registered. But that sounds like a reasonable assumption we
> should be able to make, no?

Yes, agree. Good suggestion, I will do the change.

[ ... ]

>>  int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent);
>>
>> +int dtpm_create_hierarchy(struct of_device_id *dtpm_match_table);
> 
> To start simple, I think dtpm_create_hiearchy() is the sufficient
> interface to add at this point.
> 
> However, it's quite likely that it's going to be called from a regular
> module (SoC specific platform driver), which means it needs to manage
> ->remove() operations too. Anyway, I am fine if we look into that as
> improvements on top of the $subject series.

Yes, ATM, the modules can not be unloaded on purpose. The removal can be
added later


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog