linux-kernel - Re: [PATCH v6 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] [day] [month] [year] [list]

Message-ID: <6654879e-f577-4e0c-a00a-0ee45d379b51@wanadoo.fr>
Date:   Mon, 30 Oct 2023 09:05:42 +0100
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     stanley_chang@...ltek.com
Cc:     Thinh.Nguyen@...opsys.com, conor+dt@...nel.org,
        devicetree@...r.kernel.org, gregkh@...uxfoundation.org,
        krzysztof.kozlowski+dt@...aro.org, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org, robh+dt@...nel.org
Subject: Re: [PATCH v6 1/2] usb: dwc3: add Realtek DHC RTD SoC dwc3 glue layer
 driver

Le 26/08/2023 à 05:10, Stanley Chang a écrit :
> Realtek DHC RTD SoCs integrate dwc3 IP and has some customizations to
> support different generations of SoCs.
> 
> 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/.
> 
> Signed-off-by: Stanley Chang <stanley_chang-Rasf1IRRPZFBDgjK7y7TUQ@...lic.gmane.org>
> Acked-by: Thinh Nguyen <Thinh.Nguyen-HKixBCOQz3hWk0Htik3J/w@...lic.gmane.org>
> ---

...

> +static int dwc3_rtk_probe_dwc3_core(struct dwc3_rtk *rtk)
> +{
> +	struct device *dev = rtk->dev;
> +	struct device_node *node = dev->of_node;
> +	struct platform_device *dwc3_pdev;
> +	struct device *dwc3_dev;
> +	struct device_node *dwc3_node;
> +	enum usb_dr_mode dr_mode;
> +	int ret = 0;
> +
> +	ret = dwc3_rtk_init(rtk);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = of_platform_populate(node, NULL, NULL, dev);
> +	if (ret) {
> +		dev_err(dev, "failed to add dwc3 core\n");
> +		return ret;
> +	}
> +
> +	dwc3_node = of_get_compatible_child(node, "snps,dwc3");
> +	if (!dwc3_node) {
> +		dev_err(dev, "failed to find dwc3 core node\n");
> +		ret = -ENODEV;
> +		goto depopulate;
> +	}
> +
> +	dwc3_pdev = of_find_device_by_node(dwc3_node);
> +	if (!dwc3_pdev) {
> +		dev_err(dev, "failed to find dwc3 core platform_device\n");
> +		ret = -ENODEV;
> +		goto err_node_put;
> +	}
> +
> +	dwc3_dev = &dwc3_pdev->dev;
> +	rtk->dwc = platform_get_drvdata(dwc3_pdev);
> +	if (!rtk->dwc) {
> +		dev_err(dev, "failed to find dwc3 core\n");
> +		ret = -ENODEV;
> +		goto err_pdev_put;
> +	}
> +
> +	dr_mode = usb_get_dr_mode(dwc3_dev);
> +	if (dr_mode != rtk->dwc->dr_mode) {
> +		dev_info(dev, "dts set dr_mode=%d, but dwc3 set dr_mode=%d\n",
> +			 dr_mode, rtk->dwc->dr_mode);
> +		dr_mode = rtk->dwc->dr_mode;
> +	}
> +
> +	switch (dr_mode) {
> +	case USB_DR_MODE_PERIPHERAL:
> +		rtk->cur_role = USB_ROLE_DEVICE;
> +		break;
> +	case USB_DR_MODE_HOST:
> +		rtk->cur_role = USB_ROLE_HOST;
> +		break;
> +	default:
> +		dev_dbg(rtk->dev, "%s: dr_mode=%d\n", __func__, dr_mode);
> +		break;
> +	}
> +
> +	if (device_property_read_bool(dwc3_dev, "usb-role-switch")) {
> +		ret = dwc3_rtk_setup_role_switch(rtk);
> +		if (ret) {
> +			dev_err(dev, "dwc3_rtk_setup_role_switch fail=%d\n", ret);
> +			goto err_pdev_put;
> +		}
> +		rtk->cur_role = dwc3_rtk_get_role(rtk);
> +	}
> +
> +	switch_usb2_role(rtk, rtk->cur_role);
> +
> +	return 0;
> +
> +err_pdev_put:
> +	platform_device_put(dwc3_pdev);
> +err_node_put:
> +	of_node_put(dwc3_node);
> +depopulate:
> +	of_platform_depopulate(dev);
> +
> +	return ret;
> +}
> +
> +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;
> +
> +	rtk = devm_kzalloc(dev, sizeof(*rtk), GFP_KERNEL);
> +	if (!rtk) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	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 out;
> +	}
> +
> +	regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(regs)) {
> +		ret = PTR_ERR(regs);
> +		goto out;
> +	}
> +
> +	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 out;
> +		}
> +	}
> +
> +	ret = dwc3_rtk_probe_dwc3_core(rtk);
> +
> +out:
> +	return ret;
> +}
> +
> +static void dwc3_rtk_remove(struct platform_device *pdev)
> +{
> +	struct dwc3_rtk *rtk = platform_get_drvdata(pdev);
> +
> +	rtk->dwc = NULL;
> +
> +	dwc3_rtk_remove_role_switch(rtk);
> +

Hi,

Is something like
	platform_device_put(dwc3_pdev);
	of_node_put(dwc3_node);
needed in the remove function?

(as done in the error handling path of dwc3_rtk_probe_dwc3_core())

Or should it be added at the end of dwc3_rtk_probe_dwc3_core() if the 
reference are nor needed anymore when we leave the function?

CJ

> +	of_platform_depopulate(rtk->dev);
> +}
> +

...