| 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
| ||
|
Message-ID: <8b4f8ba8685c43df9f297fefcc53edb1@realtek.com>
Date: Fri, 18 Aug 2023 07:43:26 +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 v4 1/2] usb: dwc3: add Realtek DHC RTD SoC dwc3 glue layer driver
Hi Thinh,
>
> On Tue, Aug 15, 2023, Stanley Chang wrote:
> > Realtek DHC RTD SoCs integrate dwc3 IP and has some customizations to
> > support different generations of SoCs.
>
> Please provide a summary of what "customizations" are done here.
>
I will add description:
The RTD1619b subclass SoC only supports USB 2.0 from dwc3.
The driver can set a maximum speed to support this.
Add role switching function, that can switch USB roles through other drivers,
or switch USB roles through user space through set /sys/class/usb_role/.
> > +struct dwc3_rtk {
> > + struct device *dev;
> > + void __iomem *regs;
> > + size_t regs_size;
> > + void __iomem *pm_base;
> > +
> > + struct dwc3 *dwc;
> > +
> > + int cur_dr_mode; /* current dr mode */
>
> Why not use enum for dr_mode? And I don't think you need the comment.
I will remove comment.
I will modify to use enumeration and define usb_role uniformly instead of
dr_mode to avoid confusing dr_mode and usb_role.
> > + struct usb_role_switch *role_switch; };
> > +
> > +static void switch_usb2_dr_mode(struct dwc3_rtk *rtk, int dr_mode) {
> > + switch (dr_mode) {
> > + case USB_DR_MODE_PERIPHERAL:
> > + writel(USB2_PHY_SWITCH_DEVICE |
> > + (~USB2_PHY_SWITCH_MASK &
> > + readl(rtk->regs + WRAP_USB2_PHY_REG)),
> > + rtk->regs + WRAP_USB2_PHY_REG);
>
> Please split the register read and write to separate operations here and
> everywhere else. ie:
> val = readl(offset);
> writel(val | mod, offset);
Okay.
> > + break;
> > + case USB_DR_MODE_HOST:
> > + writel(USB2_PHY_SWITCH_HOST |
> > + (~USB2_PHY_SWITCH_MASK &
> > + readl(rtk->regs + WRAP_USB2_PHY_REG)),
> > + rtk->regs + WRAP_USB2_PHY_REG);
> > + break;
> > + default:
> > + dev_dbg(rtk->dev, "%s: dr_mode=%d\n", __func__,
> dr_mode);
> > + break;
> > + }
> > +}
> > +
> > +static void switch_dwc3_dr_mode(struct dwc3_rtk *rtk, int dr_mode) {
> > + if (!rtk->dwc->role_sw)
> > + goto out;
> > +
> > + switch (dr_mode) {
> > + case USB_DR_MODE_PERIPHERAL:
> > + usb_role_switch_set_role(rtk->dwc->role_sw,
> USB_ROLE_DEVICE);
> > + break;
> > + case USB_DR_MODE_HOST:
> > + usb_role_switch_set_role(rtk->dwc->role_sw,
> USB_ROLE_HOST);
> > + break;
> > + default:
> > + dev_dbg(rtk->dev, "%s dr_mode=%d\n", __func__,
> dr_mode);
> > + break;
> > + }
> > +
> > +out:
> > + return;
> > +}
> > +
> > +static int dwc3_rtk_get_dr_mode(struct dwc3_rtk *rtk) {
> > + enum usb_role role;
> > +
> > + role = rtk->cur_dr_mode;
> > +
> > + if (rtk->dwc && rtk->dwc->role_sw)
> > + role = usb_role_switch_get_role(rtk->dwc->role_sw);
> > + else
> > + dev_dbg(rtk->dev, "%s not usb_role_switch role=%d\n",
> > + __func__, role);
> > +
> > + return role;
> > +}
> > +
> > +static void dwc3_rtk_set_dr_mode(struct dwc3_rtk *rtk, int dr_mode) {
> > + rtk->cur_dr_mode = dr_mode;
> > +
> > + switch_dwc3_dr_mode(rtk, dr_mode);
> > + mdelay(10);
> > + switch_usb2_dr_mode(rtk, dr_mode); }
> > +
> > +#if IS_ENABLED(CONFIG_USB_ROLE_SWITCH)
> > +static int dwc3_usb_role_switch_set(struct usb_role_switch *sw, enum
> > +usb_role role) {
> > + struct dwc3_rtk *rtk = usb_role_switch_get_drvdata(sw);
> > +
> > + switch (role) {
> > + case USB_ROLE_HOST:
> > + dwc3_rtk_set_dr_mode(rtk, USB_DR_MODE_HOST);
> > + break;
> > + case USB_ROLE_DEVICE:
> > + dwc3_rtk_set_dr_mode(rtk, USB_DR_MODE_PERIPHERAL);
> > + break;
> > + default:
> > + dwc3_rtk_set_dr_mode(rtk, 0);
>
> Any other value should be invalid and should not invoke
> dwc3_rtk_set_dr_mode().
I will remove it.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch
> > +*sw) {
> > + struct dwc3_rtk *rtk = usb_role_switch_get_drvdata(sw);
> > + enum usb_role role = USB_ROLE_NONE;
> > + int dr_mode;
> > +
> > + dr_mode = dwc3_rtk_get_dr_mode(rtk);
>
> dwc3_rtk_get_dr_mode() returns int converted from enum usb_role. Now
> you're mixing dr_mode with usb_role. Please use enum and avoid casting.
This is my fault. cur_dr_mode mixes dr_mode and usb_role, although they have the same value.
I will use cur_role and enum usb_role types uniformly.
> > + switch (dr_mode) {
> > + case USB_DR_MODE_HOST:
> > + role = USB_ROLE_HOST;
> > + break;
> > + case USB_DR_MODE_PERIPHERAL:
> > + role = USB_ROLE_DEVICE;
> > + break;
> > + default:
> > + dev_dbg(rtk->dev, "%s dr_mode=%d", __func__, dr_mode);
> > + break;
> > + }
> > + return role;
> > +}
> > +
> > +static int dwc3_rtk_setup_role_switch(struct dwc3_rtk *rtk) {
> > + struct usb_role_switch_desc dwc3_role_switch = {NULL};
> > +
> > + dwc3_role_switch.name = strchrnul(dev_name(rtk->dev), '.') + 1;
>
> Why not just use dev_name(rtk->dev)?
>
I want to remove the address.
For example,
Original:
98020000.dwc3_u2drd-role-switch
I want:
dwc3_u2drd-role-switch
> > + dwc3_role_switch.driver_data = rtk;
> > + dwc3_role_switch.allow_userspace_control = true;
> > + dwc3_role_switch.fwnode = dev_fwnode(rtk->dev);
> > + dwc3_role_switch.set = dwc3_usb_role_switch_set;
> > + dwc3_role_switch.get = dwc3_usb_role_switch_get;
> > + rtk->role_switch = usb_role_switch_register(rtk->dev,
> &dwc3_role_switch);
> > + if (IS_ERR(rtk->role_switch))
> > + return PTR_ERR(rtk->role_switch);
> > +
> > + return 0;
> > +}
> > +
> > +static int dwc3_rtk_remove_role_switch(struct dwc3_rtk *rtk) {
> > + if (rtk->role_switch)
> > + usb_role_switch_unregister(rtk->role_switch);
> > +
> > + rtk->role_switch = NULL;
> > +
> > + return 0;
> > +}
> > +#else
> > +#define dwc3_rtk_setup_role_switch(x) 0 #define
> > +dwc3_rtk_remove_role_switch(x) 0 #endif
> > +
> > +static int dwc3_rtk_probe(struct platform_device *pdev) {
> > + struct dwc3_rtk *rtk;
> > + struct device *dev = &pdev->dev;
> > + struct resource *res;
> > + void __iomem *regs;
> > + int ret = 0;
> > + unsigned long probe_time = jiffies;
> > +
> > + rtk = devm_kzalloc(dev, sizeof(*rtk), GFP_KERNEL);
> > + if (!rtk) {
> > + ret = -ENOMEM;
> > + goto err1;
> > + }
> > +
> > + platform_set_drvdata(pdev, rtk);
> > +
> > + rtk->dev = dev;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res) {
> > + dev_err(dev, "missing memory resource\n");
> > + ret = -ENODEV;
> > + goto err1;
> > + }
> > +
> > + regs = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(regs)) {
> > + ret = PTR_ERR(regs);
> > + goto err1;
> > + }
> > +
> > + rtk->regs = regs;
> > + rtk->regs_size = resource_size(res);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > + if (res) {
> > + rtk->pm_base = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(rtk->pm_base)) {
> > + ret = PTR_ERR(rtk->pm_base);
> > + goto err1;
> > + }
> > + }
> > +
> > + ret = dwc3_rtk_probe_dwc3_core(rtk);
> > + if (ret)
> > + goto err1;
> > +
> > + dev_dbg(dev, "%s ok! (take %d ms)\n", __func__,
> > + jiffies_to_msecs(jiffies - probe_time));
>
> This debug message doesn't look like it should be here unless it's early in the
> development cycle. Do we need this debug message?
I only want to print time of probe.
I will remove it.
> > +
> > + return 0;
> > +
> > +err1:
>
> Where's err2? If there are multiple gotos, provide more descriptive names
> instead of just numbers.
Okay I will revise this.
>
> > + return ret;
> > +}
> > +
Thanks,
Stanley
Powered by blists - more mailing lists