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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



<..>

      };
-    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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux