On Fri, Oct 30, 2020 at 6:30 PM Enric Balletbo i Serra <enric.balle...@collabora.com> wrote: > > 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.balle...@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.balle...@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.
I'm fine with the sta_mask check. Thanks! > > 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] > >>>