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

[Enric Balletbo i Serra <enric.balletbo@collabora.com>]

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.


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]
>