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

From: Nicolas Boichat <drinkcat@chromium.org>
To: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc: Matthias Brugger <mbrugger@suse.com>,
	Weiyi Lu <weiyi.lu@mediatek.com>,
	lkml <linux-kernel@vger.kernel.org>,
	Fabien Parent <fparent@baylibre.com>,
	"moderated list:ARM/Mediatek SoC support"
	<linux-mediatek@lists.infradead.org>,
	Hsin-Yi Wang <hsinyi@chromium.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Collabora Kernel ML <kernel@collabora.com>,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 02/12] soc: mediatek: Add MediaTek SCPSYS power domains
Date: Tue, 27 Oct 2020 08:19:00 +0800	[thread overview]
Message-ID: <CANMq1KBfKK9-RfMK89hRCGzhqZVqs6+YRdw8o2K+jA+3VN1_gw@mail.gmail.com> (raw)
In-Reply-To: <ebba9def-e394-c183-dd80-6dc3716a7bd1@collabora.com>

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 **`?
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-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek