Re: [PATCH v7] usb: misc: qcom_eud: Access EUD_MODE_MANAGER2 through secure calls

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

On 28/07/2025 13:19, Komal Bajaj wrote:
> On Fri, Jul 25, 2025 at 7:07 PM Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:
>>
>> On Fri, Jul 25, 2025 at 06:19:21PM +0530, Komal Bajaj wrote:
>>> On Thu, Jul 24, 2025 at 3:06 PM Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> wrote:
>>>>
>>>> On Tue, Jul 22, 2025 at 05:01:53PM +0530, Komal Bajaj wrote:
>>>>> EUD_MODE_MANAGER2 register is mapped to a memory region that is marked
>>>>> as read-only for operating system running at EL1, enforcing access
>>>>> restrictions that prohibit direct memory-mapped writes via writel().
>>>>>
>>>>> Attempts to write to this region from HLOS can result in silent failures
>>>>> or memory access violations, particularly when toggling EUD (Embedded
>>>>> USB Debugger) state. To ensure secure register access, modify the driver
>>>>> to use qcom_scm_io_writel(), which routes the write operation to Qualcomm
>>>>> Secure Channel Monitor (SCM). SCM has the necessary permissions to access
>>>>> protected memory regions, enabling reliable control over EUD state.
>>>>>
>>>>> SC7280, the only user of EUD is also affected, indicating that this could
>>>>> never have worked on a properly fused device.
>>>>>
>>>>> Fixes: 9a1bf58ccd44 ("usb: misc: eud: Add driver support for Embedded USB Debugger(EUD)")
>>>>> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com>
>>>>> Signed-off-by: Komal Bajaj <komal.bajaj@oss.qualcomm.com>
>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> [...]
>>
>>>>> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
>>>>> index 6497c4e81e951a14201ad965dadc29f9888f8254..73ebd3257625e4567f33636cdfd756344b9ed4e7 100644
>>>>> --- a/drivers/usb/misc/Kconfig
>>>>> +++ b/drivers/usb/misc/Kconfig
>>>>> @@ -147,6 +147,7 @@ config USB_APPLEDISPLAY
>>>>>   config USB_QCOM_EUD
>>>>>        tristate "QCOM Embedded USB Debugger(EUD) Driver"
>>>>>        depends on ARCH_QCOM || COMPILE_TEST
>>>>> +     depends on QCOM_SCM
>>>>
>>>> You now are preventing this code from ever being able to be built in any
>>>> testing systems, including mine, so I don't even know if this patch
>>>> builds or not.
>>>>
>>>> You did not even document this in the changelog :(
>>>
>>> QCOM_SCM is essential for QCOM_EUD for its functionality.
>>> One option I'm considering is using select QCOM_SCM, which ensures
>>> dependency is enabled when QCOM_EUD is selected. QCOM_SCM facilitates
>>> communication with secure world, this approach should not cause issues even
>>> when COMPILE_TEST is enabled on non-ARCH_QCOM platforms.
>>
>> QCOM_SCM is not user-selectable, to it is not correct to depend on it.
>> Have you had used `git grep`, you'd have seen that absolute majority of
>> the drivers uses `select QCOM_SCM`.
> 
> I had initially used select QCOM_SCM in an earlier patch, but based on
> the concern
> raised about enabling it under COMPILE_TEST on non-ARCH_QCOM platforms,
> I revised it to use a depends on condition.

QCOM_SCM can be selected on non-QCOM platforms, so there should be no 
concerns for that.

> 
>>
>>> Alternatively, I could use a conditional depends expression like:
>>> depends on (ARCH_QCOM && QCOM_SCM) || COMPILE_TEST
>>>
>>> This would allow the driver to be built under COMPILE_TEST while ensuring
>>> QCOM_SCM is present on actual QCOM platforms. However, this would
>>> require proper stubbing in the qcom_scm driver to avoid build failures during
>>> compile testing.
>>
>> --
>> With best wishes
>> Dmitry


-- 
With best wishes
Dmitry