Re: [PATCH v2] firmware: qcom_scm: modify qcom_scm_set_download_mode()

[Bjorn Andersson]

On Fri, Feb 03, 2023 at 08:23:29PM +0530, Mukesh Ojha 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@xxxxxxxxxxx>
> > > ---
> > > 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.
> 

Afaict you touch every caller of this function, so there is no concerns
about backwards compatibility.

With some use of imagination, this patch is likely intended to be
followed by some other patch which adds more values to the enum, at
which point the state you leave this in would be wrong.

> -Mukesh
> > 
> > --srini
> > 
> > >       return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
> > >   }
> > > -static void qcom_scm_set_download_mode(bool enable)
> > > +static void qcom_scm_set_download_mode(enum qcom_download_mode mode)
> > >   {
> > >       bool avail;
> > >       int ret = 0;
> > > @@ -424,10 +424,9 @@ static void qcom_scm_set_download_mode(bool enable)
> > >                            QCOM_SCM_SVC_BOOT,
> > >                            QCOM_SCM_BOOT_SET_DLOAD_MODE);
> > >       if (avail) {
> > > -        ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
> > > +        ret = __qcom_scm_set_dload_mode(__scm->dev, mode);
> > >       } else if (__scm->dload_mode_addr) {
> > > -        ret = qcom_scm_io_writel(__scm->dload_mode_addr,
> > > -                enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
> > > +        ret = qcom_scm_io_writel(__scm->dload_mode_addr, mode);
> > >       } else {
> > >           dev_err(__scm->dev,
> > >               "No available mechanism for setting download mode\n");
> > > @@ -1410,7 +1409,7 @@ static int qcom_scm_probe(struct
> > > platform_device *pdev)
> > >        * disabled below by a clean shutdown/reboot.
> > >        */
> > >       if (download_mode)
> > > -        qcom_scm_set_download_mode(true);
> > > +        qcom_scm_set_download_mode(QCOM_DOWNLOAD_FULLDUMP);
> > >       return 0;
> > >   }
> > > @@ -1419,7 +1418,7 @@ static void qcom_scm_shutdown(struct
> > > platform_device *pdev)
> > >   {
> > >       /* Clean shutdown, disable download mode to allow normal restart */
> > >       if (download_mode)
> > > -        qcom_scm_set_download_mode(false);
> > > +        qcom_scm_set_download_mode(QCOM_DOWNLOAD_NODUMP);
> > >   }
> > >   static const struct of_device_id qcom_scm_dt_match[] = {
> > > diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> > > index f833564..f9bc84e 100644
> > > --- a/include/linux/qcom_scm.h
> > > +++ b/include/linux/qcom_scm.h
> > > @@ -14,6 +14,11 @@
> > >   #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF    0x1
> > >   #define QCOM_SCM_HDCP_MAX_REQ_CNT    5
> > > +enum qcom_download_mode {

This is not an enumeration, it's a set of constants. Please use #define.

> > > +    QCOM_DOWNLOAD_NODUMP    = 0x00,
> > > +    QCOM_DOWNLOAD_FULLDUMP  = 0x10,
> > > +};

Regards,
Bjorn

> > > +
> > >   struct qcom_scm_hdcp_req {
> > >       u32 addr;
> > >       u32 val;