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

[Dmitry Baryshkov]

On 28/07/2025 13:19, Komal Bajaj wrote:
> On Fri, Jul 25, 2025 at 7:07 PM Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > > Signed-off-by: Komal Bajaj <komal.bajaj@xxxxxxxxxxxxxxxx>
> > > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> >
> > [...]
> >
> > > > > 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