linux-kernel - Re: [PATCH v5 3/5] usb: misc: eud: Add driver support for SM6115 / SM4250

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 17 May 2023 12:03:51 +0530
From:   Bhupesh Sharma <bhupesh.sharma@...aro.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-usb@...r.kernel.org, agross@...nel.org, andersson@...nel.org,
        konrad.dybcio@...aro.org, linux-kernel@...r.kernel.org,
        bhupesh.linux@...il.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, krzysztof.kozlowski@...aro.org
Subject: Re: [PATCH v5 3/5] usb: misc: eud: Add driver support for SM6115 / SM4250

Hi Greg,

On Wed, 17 May 2023 at 10:21, Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Wed, May 17, 2023 at 03:03:06AM +0530, Bhupesh Sharma wrote:
> > Add SM6115 / SM4250 SoC EUD support in qcom_eud driver.
>
> Why is the subject line duplicated here?
>
> > On some SoCs (like the SM6115 / SM4250 SoC), the mode manager
> > needs to be accessed only via the secure world (through 'scm'
> > calls).
> >
> > Also, the enable bit inside 'tcsr_check_reg' needs to be set
> > first to set the eud in 'enable' mode on these SoCs.
> >
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@...aro.org>
> > ---
> >  drivers/usb/misc/Kconfig    |  1 +
> >  drivers/usb/misc/qcom_eud.c | 69 +++++++++++++++++++++++++++++++++----
>
> Given that you didn't cc the usb maintainer, I'm guessing you don't want
> this patch applied?

Oops, I will do that in the next version.

> >  2 files changed, 63 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> > index 99b15b77dfd5..fe1b5fec1dfc 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
> > +     select QCOM_SCM
>
> How well is that going to work on building on non-QCOM systems?  Can
> QCOM_SCM build if COMPILE_TEST is enabled?  select is rough to get
> right, are you sure it's correct here?  If so, some documentation in the
> changelog would be appreciated.

Ok, I will double check.

> >       select USB_ROLE_SWITCH
> >       help
> >         This module enables support for Qualcomm Technologies, Inc.
> > diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> > index b7f13df00764..10d194604d4c 100644
> > --- a/drivers/usb/misc/qcom_eud.c
> > +++ b/drivers/usb/misc/qcom_eud.c
> > @@ -5,12 +5,14 @@
> >
> >  #include <linux/bitops.h>
> >  #include <linux/err.h>
> > +#include <linux/firmware/qcom/qcom_scm.h>
>
> There's no rule to keep these sorted, but it's your choice...

Sure.

> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/iopoll.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/sysfs.h>
> > @@ -22,23 +24,33 @@
> >  #define EUD_REG_VBUS_INT_CLR 0x0080
> >  #define EUD_REG_CSR_EUD_EN   0x1014
> >  #define EUD_REG_SW_ATTACH_DET        0x1018
> > -#define EUD_REG_EUD_EN2        0x0000
> > +#define EUD_REG_EUD_EN2              0x0000
>
> Why the coding style cleanup in the same patch?  Remember, changes only
> do one thing, and you have already listed 2 things in your commit
> message :(

Sure, will spin a separate patch for cleanups.

> >  #define EUD_ENABLE           BIT(0)
> > -#define EUD_INT_PET_EUD      BIT(0)
> > +#define EUD_INT_PET_EUD              BIT(0)
>
> Again, why this change?

Ack.
Will send a v6 shortly.

Thanks,
Bhupesh