MIME-Version: 1.0
References: <1675419435-30726-1-git-send-email-quic_mojha@quicinc.com>
 <1bca9111-1ad4-ad1d-b30a-9d2fbddf9e3a@linaro.org> <3ca6d220-447e-0578-b525-3fb470be7edf@quicinc.com>
In-Reply-To: <3ca6d220-447e-0578-b525-3fb470be7edf@quicinc.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Fri, 3 Feb 2023 17:34:56 +0200
Message-ID: <CAA8EJpoVByj28GFNUKydONsS8UZqO-5eSqSsvZHcEHaJbW+Gig@mail.gmail.com>
Subject: Re: [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()
To:     Mukesh Ojha <quic_mojha@quicinc.com>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 3 Feb 2023 at 16:53, Mukesh Ojha <quic_mojha@quicinc.com> wrote:
>
>
>
> On 2/3/2023 8:21 PM, Srinivas Kandagatla wrote:
> >
> >
> > On 03/02/2023 10:17, Mukesh Ojha wrote:
> >> Modify qcom_scm_set_download_mode() such that it can support
> >> multiple modes. There is no functional change with this change.
> >>
> >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> >> ---
> >> Changes in v2:
> >>    - Stop changing legacy scm id for dload mode.
> >>
> >>   drivers/firmware/qcom_scm.c | 15 +++++++--------
> >>   include/linux/qcom_scm.h    |  5 +++++
> >>   2 files changed, 12 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> >> index cdbfe54..6245b97 100644
> >> --- a/drivers/firmware/qcom_scm.c
> >> +++ b/drivers/firmware/qcom_scm.c
> >> @@ -400,7 +400,7 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
> >>   }
> >>   EXPORT_SYMBOL(qcom_scm_set_remote_state);
> >> -static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
> >> +static int __qcom_scm_set_dload_mode(struct device *dev, enum
> >> qcom_download_mode mode)
> >>   {
> >>       struct qcom_scm_desc desc = {
> >>           .svc = QCOM_SCM_SVC_BOOT,
> >> @@ -410,12 +410,12 @@ static int __qcom_scm_set_dload_mode(struct
> >> device *dev, bool enable)
> >>           .owner = ARM_SMCCC_OWNER_SIP,
> >>       };
> >> -    desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> >> +    desc.args[1] = mode ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> >
> > I think this line should be:
> >
> >      desc.args[1] = mode;
> >
>
> Should be fine..for backward compatibility as we want to avoid what is
> passed to trust zone without check and since this is legacy code, i
> would like to avoid.

I'd second Srini here. Please remove the ternary operator and pass
mode directly. If you'd like to limit the 'mode' argument, do so
directly (and return an error if the mode is not supported).

However there still exists a bigger problem in my opinion. You are
changing an API. Please provide a user for this API. 'The user will be
provided separately/later/whatever' is usually not enough.


-- 
With best wishes
Dmitry