Re: [PATCH V5 5/5] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode

[Sricharan Ramabadhran]

<..>

> > > > > >       };
> > > > > > -    desc.args[1] = enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0;
> > > > > > +    desc.args[1] = enable ? val | QCOM_SCM_BOOT_SET_DLOAD_MODE :
> > > > > > +                val & ~(QCOM_SCM_BOOT_SET_DLOAD_MODE);
> > > > > >       return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
> > > > > >   }
> > > > > > @@ -426,15 +427,25 @@ static void qcom_scm_set_download_mode(bool 
> > > > > > enable)
> > > > > >   {
> > > > > >       bool avail;
> > > > > >       int ret = 0;
> > > > > > +    u32 dload_addr_val;
> > > > > >       avail = __qcom_scm_is_call_available(__scm->dev,
> > > > > >                            QCOM_SCM_SVC_BOOT,
> > > > > >                            QCOM_SCM_BOOT_SET_DLOAD_MODE);
> > > > > > +    ret = qcom_scm_io_readl(__scm->dload_mode_addr, 
> > > > > > &dload_addr_val);
> > > > > > +
> > > > > > +    if (ret) {
> > > > > > +        dev_err(__scm->dev,
> > > > > > +            "failed to read dload mode address value: %d\n", ret);
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > >       if (avail) {
> > > > > > -        ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
> > > > > > +        ret = __qcom_scm_set_dload_mode(__scm->dev, 
> > > > > > dload_addr_val, enable);
> > > > >
> > > > > Did you test this on a target where it comes under this if 
> > > > > statement? does it really need to know dload_mode_addr for this 
> > > > > target ?
> > > >
> > > >
> > > > Can we do something like this? I would let other review as well.
> > > >
> > > > --------------------------------------->0------------------------------------------- 
> > > >
> > > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > > > index cdbfe54..26b7eda 100644
> > > > --- a/drivers/firmware/qcom_scm.c
> > > > +++ b/drivers/firmware/qcom_scm.c
> > > > @@ -419,6 +419,7 @@ static void qcom_scm_set_download_mode(bool enable)
> > > >  {
> > > >         bool avail;
> > > >         int ret = 0;
> > > > +       u32 dload_addr_val;
> > > >
> > > >         avail = __qcom_scm_is_call_available(__scm->dev,
> > > >                                              QCOM_SCM_SVC_BOOT,
> > > > @@ -426,8 +427,16 @@ static void qcom_scm_set_download_mode(bool 
> > > > enable)
> > > >         if (avail) {
> > > >                 ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
> > > >         } 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_readl(__scm->dload_mode_addr, 
> > > > &dload_addr_val);
> > > > +               if (ret) {
> > > > +                       dev_err(__scm->dev,
> > > > +                               "failed to read dload mode address 
> > > > value: %d\n", ret);
> > > > +                       return;
> > > > +               }
> > > > +
> > > > +               ret = qcom_scm_io_writel(__scm->dload_mode_addr, 
> > > > enable ?
> > > > +                               dload_addr_val | 
> > > > QCOM_SCM_BOOT_SET_DLOAD_MODE :
> > > > +                               dload_addr_val & 
> > > > ~(QCOM_SCM_BOOT_SET_DLOAD_MODE));
> > > >         } else {
> > > >                 dev_err(__scm->dev,
> > > >                         "No available mechanism for setting download 
> > > > mode\n");
> > > >
> > > > -Mukesh
> > >
> > > Okay sure..Agreed, will address this in the next patch.
> >
> >    Also, not sure, if its better to keep the old behavior working for
> >    targets that does not support 'READ' of this address. If one such
> >    thing exists, that will be broken now. In such a case, we should
> >    ignore if scm_io_readl fails, still write and dload_addr_val should
> >    be '0' initialised.
>
> Why would a secure read of this register would fail, if one is allowed 
> to do secure write ?
>
> Honestly, i was not understanding the purpose of this bitwise handling
> of this patch, i thought it is trying to fix existing issue for
> some target.
>
> For some of the upstream target(e.g sm8450, i verified it myself), it is 
> not an issue.
>
> arch/arm64/boot/dts/qcom/msm8916.dtsi:                  qcom,dload-mode 
> = <&tcsr 0x6100>;
> arch/arm64/boot/dts/qcom/msm8976.dtsi:                  qcom,dload-mode 
> = <&tcsr 0x6100>;
> arch/arm64/boot/dts/qcom/msm8996.dtsi:                  qcom,dload-mode 
> = <&tcsr_2 0x13000>;
> arch/arm64/boot/dts/qcom/sm8450.dtsi:                   qcom,dload-mode 
> = <&tcsr 0x13000>;
>
>
> However, it looks valid to handle only the effective bits. I have worked 
> on top of this patch and tested it and posted here.
>
> https://lore.kernel.org/lkml/1676990381-18184-1-git-send-email-quic_mojha@xxxxxxxxxxx/ 
>
>
> Do you have any example of any upstream target where this would fail ?

  Actually not sure. I was saying just based on the fact that,
  previously it was 'unconditional write' on all targets, now its a
  'conditional write'. If 'read' would never fail from secure,
  then your patch looks fine.

Regards,
 Sricharan
 y