Messages in this thread | | | Date | Wed, 22 Feb 2023 13:19:13 +0530 | Subject | Re: [PATCH V5 5/5] firmware: scm: Modify only the DLOAD bit in TCSR register for download mode | From | 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@quicinc.com/ > > > 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
| |