Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <647f915e-b08a-1bc9-2082-70140521a644@quicinc.com>
Date:   Thu, 20 Apr 2023 12:11:22 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH v3 2/3] cpuidle: psci: Move enabling OSI mode after power
 domains creation
To:     Ulf Hansson <ulf.hansson@linaro.org>
CC:     <andersson@kernel.org>, <dianders@chromium.org>,
        <swboyd@chromium.org>, <wingers@google.com>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-pm@vger.kernel.org>, <sudeep.holla@arm.com>,
        <jwerner@chromium.org>, <quic_lsrao@quicinc.com>,
        <quic_rjendra@quicinc.com>
References: <20230414055502.23920-1-quic_mkshah@quicinc.com>
 <20230414055502.23920-3-quic_mkshah@quicinc.com>
 <CAPDyKFo5UhOca-DvOKOZWcBrcK8ONdSO8-M=8fE6kkwkPuoCww@mail.gmail.com>
Content-Language: en-US
From:   "Maulik Shah (mkshah)" <quic_mkshah@quicinc.com>
In-Reply-To: <CAPDyKFo5UhOca-DvOKOZWcBrcK8ONdSO8-M=8fE6kkwkPuoCww@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Ulf,

On 4/18/2023 1:42 PM, Ulf Hansson wrote:
> O
>
>          /* Bail out if not using the hierarchical CPU topology. */
>          if (!pd_count)
> -               goto no_pd;
> +               goto remove_pd;
> We should return 0 here instead, right?
right. will fix in next revision.
>>          /* Link genpd masters/subdomains to model the CPU topology. */
>>          ret = dt_idle_pd_init_topology(np);
>>          if (ret)
>> -               goto remove_pd;
>> +               goto remove_pd_topology;
> This looks wrong to me. Shouldn't we continue to goto the "remove_pd"
> label for this error path?
We should need to remove already added subdomains via 
of_genpd_add_subdomain() if one of them fails.
So this look ok to me.
>
>> +
>> +       /* let's try to enable OSI. */
>> +       ret = psci_set_osi_mode(use_osi);
>> +       if (ret)
>> +               goto remove_pd_topology;
>>
>>          pr_info("Initialized CPU PM domain topology using %s mode\n",
>>                  use_osi ? "OSI" : "PC");
>>          return 0;
>>
>> -put_node:
>> -       of_node_put(node);
>> +remove_pd_topology:
>> +       dt_idle_pd_remove_topology(np);
>>   remove_pd:
>>          psci_pd_remove();
>> +put_node:
>> +       of_node_put(node);
> This of_node_put() should only be called if we break the
> "for_each_child_of_node" loop above because of an error, I think.
> Perhaps it's cleaner to just move this within the loop?
yes will move inside loop.

Thanks,
Maulik