linux-kernel - RE: [PATCH v2 1/2] usb: dwc3: add Realtek DHC RTD SoC dwc3 glue layer driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 2 Aug 2023 08:26:50 +0000
From:   Stanley Chang[昌育德] 
        <stanley_chang@...ltek.com>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 1/2] usb: dwc3: add Realtek DHC RTD SoC dwc3 glue layer driver

Hi Thinh,

> > +struct dwc3_rtk {
> > +     struct device *dev;
> > +     void __iomem *regs;
> > +     size_t regs_size;
> > +
> > +     struct dwc3 *dwc;
> > +
> > +     int cur_dr_mode; /* current dr mode */
> > +     bool support_drd_mode; /* if support Host/device switch */
> 
> I think we may not need this and do away the boolean support_drd_mode.

Yes, the initial flow should be simplified as
@@ -346,12 +342,7 @@ static int dwc3_rtk_probe_dwc3_core(struct dwc3_rtk *rtk)

        rtk->cur_dr_mode = dr_mode;

-       if (device_property_read_bool(dwc3_dev, "usb-role-switch"))
-               rtk->support_drd_mode = true;
-       else
-               rtk->support_drd_mode = false;
-
-       if (rtk->support_drd_mode) {
+       if (device_property_read_bool(dwc3_dev, "usb-role-switch")) {
                dwc3_rtk_setup_role_switch(rtk);
                rtk->cur_dr_mode = dwc3_rtk_get_dr_mode(rtk);
        }

> > +static int dwc3_rtk_set_dr_mode(struct dwc3_rtk *rtk, int dr_mode)
> 
> Why return the mode rather than status if the setting? You're not checking the
> return of this function in the caller anyway.

You are right, this return value is unnecessary.
I will remove it.

> > +{
> > +     if (!rtk->support_drd_mode)
> > +             return rtk->cur_dr_mode;
> > +
> > +     rtk->cur_dr_mode = dr_mode;
> > +
> > +     switch_dwc3_dr_mode(rtk, dr_mode);
> > +     mdelay(10);
> > +     switch_usb2_dr_mode(rtk, dr_mode);
> > +
> > +     return rtk->cur_dr_mode;
> > +}
> > +

> > +static int dwc3_rtk_setup_role_switch(struct dwc3_rtk *rtk)
> 
> Any reason why we're doing the role switch here and not what's implemented
> from the core?
> 
Because we have to set the usb 2.0 phy mode through switch_usb2_dr_mode in the function dwc3_rtk_set_dr_mode.
In fact, switch_dwc3_dr_mode will use the role switching implemented by core.

> > +
> > +module_platform_driver(dwc3_rtk_driver);
> > +
> > +MODULE_AUTHOR("Stanley Chang <stanley_chang@...ltek.com>");
> > +MODULE_DESCRIPTION("DesignWare USB3 Realtek Glue Layer");
> > +MODULE_ALIAS("platform:rtk-dwc3");
> > +MODULE_LICENSE("GPL");
> 
> I'm not familiar with licensing much, but can the SPDX header indicates
> different version than the module license?
> 
Thanks Greg for your comment.
Either GPL or GPL v2 are suitable for our source code.

Thanks,
Stanley