On Fri, Oct 30, 2020 at 6:30 PM Enric Balletbo i Serra
[off-list ref] wrote:

Hi Nicolas,

On 28/10/20 2:13, Nicolas Boichat wrote:

quoted

On Wed, Oct 28, 2020 at 12:25 AM Enric Balletbo i Serra
[off-list ref] wrote:

quoted

Hi Nicolas,

On 27/10/20 1:19, Nicolas Boichat wrote:

quoted

Hi Enric,

On Mon, Oct 26, 2020 at 11:17 PM Enric Balletbo i Serra
[off-list ref] wrote:

quoted

Hi Nicolas,

Many thanks for looking at this.

Thanks to you ,-)

[snip]

quoted

quoted

quoted

+       if (id >= scpsys->soc_data->num_domains) {
+               dev_err_probe(scpsys->dev, -EINVAL, "%pOFn: invalid domain id %d\n", node, id);
+               return -EINVAL;
+       }
+
+       domain_data = &scpsys->soc_data->domains[id];
+       if (!domain_data) {

Is that even possible at all? I mean, even if
scpsys->soc_data->domains is NULL, as long as id != 0, this will no
happen.

I think could happen with a bad DT definition. I.e if for the definition of the
MT8173 domains you use a wrong value for the reg property, a value that is not
present in the SoC data. It is unlikely if you use the defines but could happen
if you hardcore the value. We cannot check this with the DT json-schema.

I wasn't clear in my explanation, and looking further there is more
that looks wrong.

This expression &scpsys->soc_data->domains[id] is a pointer to element
"id" of the array domains. So if you convert to integer arithmetic,
it'll be something like `(long)scpsys->soc_data->domains +
(sizeof(struct generic_pm_domain *)) * id`. The only way this can be
NULL is if scpsys->soc_data->domains pointer is NULL, which, actually,
can't really happen as it's the 5th element of a struct scpsys
structure `(long)scpsys->soc_data + offset_of(domains, struct scpsys)
+ (sizeof(struct generic_pm_domain *)) * id`.

I think what you mean is either:
domain_data = &scpsys->soc_data->domains[id];
if (!*domain_data)
[but then domain_data type should be `struct generic_pm_domain **`?

I think you're confusing the field `struct generic_pm_domain *domains[]`from the
`struct scpsys` with `const struct scpsys_domain_data *domains` from `struct
scpsys_soc_data`. My bad they have the same name, I should probably rename the
second one as domain_info or domain_data to avoid that confusion.

Oh, okay, get it, thanks for clarifying, I got myself confused indeed ,-P

But, still, part of my integer arithmetics still holds...

&scpsys->soc_data->domains[id] = (long)scpsys->soc_data->domains +
(sizeof(struct generic_pm_domain *)) * id. The only way domain_data
can be NULL is if scpsys->soc_data->domains pointer is NULL (it can't
be, really, assuming scpsys_soc_data structures are well defined) AND
id is 0.

Now, if I understand what you want to check here. If a domain id is
not specified in scpsys_domain_data (e.g. if there is a gap in
MT8XXX_POWER_DOMAIN_YYY indices and if `id` points at one of those
gaps), you'll get an all-zero entry in domain_data. So maybe you can
just check that domain_data->sta_mask != 0? Would that be enough? (I
expect that sta_mask would always need to be set?)

Yes, that would be enough. I'll change for the next version.

quoted

But then again, are there ever gaps in MT8XXX_POWER_DOMAIN_YYY indices?

AFAIK, there is no gaps, but one could make gaps when filling that info.  I
still think is worth have this check although is "unlikely" to happen due an
human error :-). I'll maintain for the next version, but I don't really care to
remove it if all you prefer I remove it.

I'm fine with the sta_mask check. Thanks!

Thanks,
  Enric

quoted

quoted

diff --git a/drivers/soc/mediatek/mtk-pm-domains.h

b/drivers/soc/mediatek/mtk-pm-domains.h
index 7c8efcb3cef2..6ff095db8a27 100644

--- a/drivers/soc/mediatek/mtk-pm-domains.h
+++ b/drivers/soc/mediatek/mtk-pm-domains.h

@@ -56,7 +56,7 @@ struct scpsys_domain_data {
 };

 struct scpsys_soc_data {
-       const struct scpsys_domain_data *domains;
+       const struct scpsys_domain_data *domain_data;
        int num_domains;
        int pwr_sta_offs;
        int pwr_sta2nd_offs;

---

struct scpsys {
    ...
    const struct scpsys_soc_data *soc_data;
    ...
    struct generic_pm_domain *domains[];
}


domain_data = &scpsys->soc_data->domain_data[id];
if (!domain_data)

Thanks,
  Enric

quoted

Does your code compile with warnings enabled?]
or:
domain_data = scpsys->soc_data->domains[id];
if (!domain_data)
[then the test makes sense]

[snip]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel