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

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

Hi Nicolas,

On 28/10/20 2:13, Nicolas Boichat wrote:
> 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?)
> 

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

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

Thanks,
  Enric


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