Re: [PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains

[Nicolas Boichat <drinkcat@chromium.org>]

On Wed, Oct 28, 2020 at 12:25 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Nicolas,
>
> On 27/10/20 1:19, Nicolas Boichat wrote:
> > Hi Enric,
> >
> > On Mon, Oct 26, 2020 at 11:17 PM Enric Balletbo i Serra
> > <enric.balletbo@collabora.com> wrote:
> >>
> >> Hi Nicolas,
> >>
> >> Many thanks for looking at this.
> >
> > Thanks to you ,-)
> >
> > [snip]
> >>>> +       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?)

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

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