| 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: <ZrunFEOV5/aM4G4U@lizhi-Precision-Tower-5810>
Date: Tue, 13 Aug 2024 14:33:56 -0400
From: Frank Li <Frank.li@....com>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Felipe Balbi <balbi@...nel.org>,
Wesley Cheng <quic_wcheng@...cinc.com>,
Saravana Kannan <saravanak@...gle.com>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Philipp Zabel <p.zabel@...gutronix.de>,
Konrad Dybcio <konrad.dybcio@...aro.org>, linux-usb@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org,
Bjorn Andersson <quic_bjorande@...cinc.com>
Subject: Re: [PATCH v2 6/7] usb: dwc3: qcom: Transition to flattened model
On Sun, Aug 11, 2024 at 08:12:03PM -0700, Bjorn Andersson wrote:
> From: Bjorn Andersson <quic_bjorande@...cinc.com>
>
> The USB IP-block found in most Qualcomm platforms is modelled in the
> Linux kernel as 3 different independent device drivers, but as shown by
> the already existing layering violations in the Qualcomm glue driver
> they can not be operated independently.
>
> With the current implementation, the glue driver registers the core and
> has no way to know when this is done. As a result, e.g. the suspend
> callbacks needs to guard against NULL pointer dereferences when trying
> to peek into the struct dwc3 found in the drvdata of the child.
> Even with these checks, there are no way to fully protect ourselves from
> the race conditions that occur if the DWC3 is unbound.
>
> Missing from the upstream Qualcomm USB support is handling of role
> switching, in which the glue needs to be notified upon DRD mode changes.
> Several attempts has been made through the years to register callbacks
> etc, but they always fall short when it comes to handling of the core's
> probe deferral on resources etc.
>
> Moving to a model where the DWC3 core is instantiated in a synchronous
> fashion avoids above described race conditions.
>
> It is however not feasible to do so without also flattening the
> DeviceTree binding, as assumptions are made in the DWC3 core and
> frameworks used that the device's associated of_node will the that of
> the core. Furthermore, the DeviceTree binding is a direct
> representation of the Linux driver model, and doesn't necessarily
> describe "the USB IP-block".
>
> The Qualcomm DWC3 glue driver is therefor transitioned to initialize and
> operate the DWC3 within the one device context, in synchronous fashion.
>
> To handle backwards compatibility, and to remove the two-device model,
> of_nodes of the old compatible are converted to the new one, early
> during probe.
>
> This happens in the event that a DWC3 core child node is present, the
> content of the reg and interrupt properties of this node are merged into
> the shared properties, all remaining properties are copied and the core
> node is dropped. Effectively transitioning the node from qcom,dwc3 to
> qcom,snps-dwc3.
>
> Signed-off-by: Bjorn Andersson <quic_bjorande@...cinc.com>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 310 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 251 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 33de03f2d782..27b5013cd411 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -6,6 +6,7 @@
>
> #include <linux/io.h>
> #include <linux/of.h>
> +#include <linux/cleanup.h>
> #include <linux/clk.h>
> #include <linux/irq.h>
> #include <linux/of_clk.h>
> @@ -13,6 +14,8 @@
> #include <linux/kernel.h>
> #include <linux/extcon.h>
> #include <linux/interconnect.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/phy/phy.h>
> @@ -22,6 +25,7 @@
> #include <linux/usb/hcd.h>
> #include <linux/usb.h>
> #include "core.h"
> +#include "glue.h"
>
> /* USB QSCRATCH Hardware registers */
> #define QSCRATCH_HS_PHY_CTRL 0x10
> @@ -72,7 +76,7 @@ struct dwc3_qcom_port {
> struct dwc3_qcom {
> struct device *dev;
> void __iomem *qscratch_base;
> - struct platform_device *dwc3;
> + struct dwc3 *dwc;
> struct clk **clks;
> int num_clocks;
> struct reset_control *resets;
> @@ -259,7 +263,7 @@ static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> goto put_path_ddr;
> }
>
> - max_speed = usb_get_maximum_speed(&qcom->dwc3->dev);
> + max_speed = usb_get_maximum_speed(qcom->dwc->dev);
> if (max_speed >= USB_SPEED_SUPER || max_speed == USB_SPEED_UNKNOWN) {
> ret = icc_set_bw(qcom->icc_path_ddr,
> USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
> @@ -302,25 +306,16 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
> /* Only usable in contexts where the role can not change. */
> static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
> {
> - struct dwc3 *dwc;
> -
> - /*
> - * FIXME: Fix this layering violation.
> - */
> - dwc = platform_get_drvdata(qcom->dwc3);
> -
> - /* Core driver may not have probed yet. */
> - if (!dwc)
> - return false;
> + struct dwc3 *dwc = qcom->dwc;
>
> return dwc->xhci;
dwc only use once.
return qcom->dwc->xhci?
> }
>
> static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, int port_index)
> {
> - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> struct usb_device *udev;
> struct usb_hcd __maybe_unused *hcd;
> + struct dwc3 *dwc = qcom->dwc;
>
> /*
> * FIXME: Fix this layering violation.
> @@ -497,7 +492,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
> static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
> {
> struct dwc3_qcom *qcom = data;
> - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> + struct dwc3 *dwc = qcom->dwc;
>
> /* If pm_suspended then let pm_resume take care of resuming h/w */
> if (qcom->pm_suspended)
> @@ -699,34 +694,198 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
> return 0;
> }
>
> -static int dwc3_qcom_of_register_core(struct dwc3_qcom *qcom, struct platform_device *pdev)
> +static struct property *dwc3_qcom_legacy_prop_concat(const char *name,
> + const void *a, size_t a_len,
> + const void *b, size_t b_len)
> {
> - struct device_node *np = pdev->dev.of_node, *dwc3_np;
> - struct device *dev = &pdev->dev;
> - int ret;
> + size_t len = a_len + b_len;
>
> - dwc3_np = of_get_compatible_child(np, "snps,dwc3");
> - if (!dwc3_np) {
> - dev_err(dev, "failed to find dwc3 core child\n");
> - return -ENODEV;
> - }
> + struct property *prop __free(kfree) = kzalloc(sizeof(*prop), GFP_KERNEL);
> + char *prop_name __free(kfree) = kstrdup(name, GFP_KERNEL);
> + void *value __free(kfree) = kzalloc(len, GFP_KERNEL);
> + if (!prop || !prop_name || !value)
> + return NULL;
>
> - ret = of_platform_populate(np, NULL, NULL, dev);
> - if (ret) {
> - dev_err(dev, "failed to register dwc3 core - %d\n", ret);
> - goto node_put;
> + prop->name = no_free_ptr(prop_name);
> + prop->value = no_free_ptr(value);
> + prop->length = len;
> +
> + memcpy(prop->value, a, a_len);
> + memcpy(prop->value + a_len, b, b_len);
> +
> + return_ptr(prop);
> +}
> +
> +/* Replace reg property with base address from dwc3 node and fixed length */
> +static int dwc3_qcom_legacy_update_reg(struct device_node *qcom,
> + struct device_node *dwc3)
> +{
> + int addr_cells;
> + int size_cells;
> + u64 dwc3_addr;
> + int ret;
> +
> + ret = of_property_read_reg(dwc3, 0, &dwc3_addr, NULL);
> + if (ret < 0)
> + return ret;
> +
> + addr_cells = of_n_addr_cells(qcom);
> + size_cells = of_n_addr_cells(qcom);
> +
> + __be32 *reg __free(kfree) = kcalloc(addr_cells + size_cells, sizeof(__be32), GFP_KERNEL);
> + if (!reg)
> + return -ENOMEM;
> +
> + reg[addr_cells - 1] = cpu_to_be32(dwc3_addr);
> + reg[addr_cells + size_cells - 1] = cpu_to_be32(SDM845_QSCRATCH_BASE_OFFSET + SDM845_QSCRATCH_SIZE);
> +
> + struct property *prop __free(kfree) = kzalloc(sizeof(*prop), GFP_KERNEL);
> + char *name __free(kfree) = kstrdup("reg", GFP_KERNEL);
> + if (!prop || !name)
> + return -ENOMEM;
> +
> + prop->name = no_free_ptr(name);
> + prop->value = no_free_ptr(reg);
> + prop->length = (addr_cells + size_cells) * sizeof(__be32);
> +
> + return of_update_property(qcom, no_free_ptr(prop));
> +}
> +
> +/* Prepend dwc_usb3 interrupt to relevant interrupt properties */
> +static int dwc3_qcom_legacy_convert_interrupts(struct device_node *qcom,
> + struct property *dwc3_irq)
> +{
> + const __be32 *interrupts;
> + struct property *new;
> + const void *names;
> + int interrupts_len;
> + int names_len;
> + int ret;
> +
> + if ((interrupts = of_get_property(qcom, "interrupts-extended", &interrupts_len)) != NULL) {
> + struct device_node *parent __free(device_node) = of_irq_find_parent(qcom);
> + if (!parent)
> + return -EINVAL;
> +
> + __be32 *value __free(kfree) = kzalloc(sizeof(__be32) + dwc3_irq->length, GFP_KERNEL);
> + if (!value)
> + return -ENOMEM;
> + value[0] = cpu_to_be32(parent->phandle);
> + memcpy(&value[1], dwc3_irq->value, dwc3_irq->length);
> +
> + new = dwc3_qcom_legacy_prop_concat("interrupts-extended",
> + value, sizeof(__be32) + dwc3_irq->length,
> + interrupts, interrupts_len);
> + } else if ((interrupts = of_get_property(qcom, "interrupts", &interrupts_len)) != NULL) {
> + new = dwc3_qcom_legacy_prop_concat("interrupts",
> + dwc3_irq->value, dwc3_irq->length,
> + interrupts, interrupts_len);
> + } else {
> + return -EINVAL;
> }
>
> - qcom->dwc3 = of_find_device_by_node(dwc3_np);
> - if (!qcom->dwc3) {
> - ret = -ENODEV;
> - dev_err(dev, "failed to get dwc3 platform device\n");
> - of_platform_depopulate(dev);
> + if (!new)
> + return -ENOMEM;
> +
> + ret = of_update_property(qcom, new);
> + if (ret < 0)
> + return ret;
> +
> + names = of_get_property(qcom, "interrupt-names", &names_len);
> + if (!names)
> + return -EINVAL;
> +
> + new = dwc3_qcom_legacy_prop_concat("interrupt-names",
> + "dwc_usb3", strlen("dwc_usb3") + 1,
> + names, names_len);
> + if (!new)
> + return -ENOMEM;
> +
> + return of_update_property(qcom, new);
> +}
> +
> +/* Copy property to qcom node */
> +static int dwc3_qcom_legacy_migrate_prop(struct device_node *qcom,
> + struct property *prop)
> +{
> + struct property *new __free(kfree) = kzalloc(sizeof(*new), GFP_KERNEL);
> + char *name __free(kfree) = kstrdup(prop->name, GFP_KERNEL);
> + void *value __free(kfree) = kmemdup(prop->value, prop->length, GFP_KERNEL);
> +
> + if (!new || !name || !value)
> + return -ENOMEM;
> +
> + new->name = no_free_ptr(name);
> + new->value = no_free_ptr(value);
> + new->length = prop->length;
> +
> + return of_update_property(qcom, no_free_ptr(new));
> +}
> +
> +/* Move a child node, with it's properties and children, from dwc3 to qcom node */
> +static int dwc3_qcom_legacy_migrate_child(struct device_node *qcom,
> + struct device_node *dwc3,
> + const char *name)
> +{
> + struct device_node *child;
> +
> + child = of_get_child_by_name(dwc3, name);
> + if (!child)
> + return 0;
> +
> + of_detach_node(child);
> + child->parent = qcom;
> + of_attach_node(child);
> + of_node_put(child);
> +
> + return 0;
> +}
> +
> +/* Convert dev's DeviceTree representation from qcom,dwc3 to qcom,snps-dwc3 binding */
> +static int dwc3_qcom_convert_legacy_dt(struct device *dev)
> +{
> + struct device_node *qcom = dev->of_node;
> + struct device_node *dwc3;
> + struct property *prop;
> + int ret = 0;
> +
> + dwc3 = of_get_compatible_child(qcom, "snps,dwc3");
> + if (!dwc3)
> + return 0;
> +
> + /* We have a child node, but no support for dynamic OF */
> + if (!IS_ENABLED(CONFIG_OF_DYNAMIC))
> + return -EINVAL;
> +
> + for_each_property_of_node(dwc3, prop) {
> + if (!strcmp(prop->name, "compatible"))
> + ;
> + else if (!strcmp(prop->name, "reg"))
> + ret = dwc3_qcom_legacy_update_reg(qcom, dwc3);
> + else if (!strcmp(prop->name, "interrupts"))
> + ret = dwc3_qcom_legacy_convert_interrupts(qcom, prop);
> + else
> + ret = dwc3_qcom_legacy_migrate_prop(qcom, prop);
> }
>
> -node_put:
> - of_node_put(dwc3_np);
> + if (ret < 0)
> + goto err_node_put;
> +
> + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "port");
> + if (ret)
> + goto err_node_put;
> +
> + ret = dwc3_qcom_legacy_migrate_child(qcom, dwc3, "ports");
> + if (ret)
> + goto err_node_put;
> +
> + of_detach_node(dwc3);
> + of_node_put(dwc3);
>
> + return 0;
> +
> +err_node_put:
> + of_node_put(dwc3);
> return ret;
> }
Look like you copy children dwc3's property into current glue node.
Can you passdown dwc3's node into dwc3_probe(), let dwc3_probe to handle
these, Or move it into dwc3-core. otherwise, if imx want to do the same
thing, the the same code will be dupicated.
>
> @@ -735,16 +894,21 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> struct device_node *np = pdev->dev.of_node;
> struct device *dev = &pdev->dev;
> struct dwc3_qcom *qcom;
> - struct resource *res;
> + struct resource res;
> int ret, i;
> bool ignore_pipe_clk;
> bool wakeup_source;
>
> + ret = dwc3_qcom_convert_legacy_dt(dev);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to convert legacy OF node\n");
> + return ret;
> + }
> +
> qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
> if (!qcom)
> return -ENOMEM;
>
> - platform_set_drvdata(pdev, qcom);
> qcom->dev = &pdev->dev;
>
> qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
> @@ -773,10 +937,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> goto reset_assert;
> }
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + ret = of_address_to_resource(np, 0, &res);
> + if (ret < 0)
> + goto clk_disable;
> + res.end = res.start + SDM845_QSCRATCH_BASE_OFFSET;
>
> - qcom->qscratch_base = devm_ioremap_resource(dev, res);
> + qcom->qscratch_base = devm_ioremap(dev, res.end, SDM845_QSCRATCH_SIZE);
> if (IS_ERR(qcom->qscratch_base)) {
> + dev_err(dev, "failed to map qscratch region: %pe\n", qcom->qscratch_base);
dev_err_probe()?
> ret = PTR_ERR(qcom->qscratch_base);
> goto clk_disable;
> }
> @@ -796,17 +964,17 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> if (ignore_pipe_clk)
> dwc3_qcom_select_utmi_clk(qcom);
>
> - ret = dwc3_qcom_of_register_core(qcom, pdev);
> - if (ret) {
> - dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
> + qcom->dwc = dwc3_probe(pdev, &res, true, qcom);
> + if (IS_ERR(qcom->dwc)) {
> + ret = dev_err_probe(dev, PTR_ERR(qcom->dwc), "failed to register DWC3 Core\n");
> goto clk_disable;
> }
>
> ret = dwc3_qcom_interconnect_init(qcom);
> if (ret)
> - goto depopulate;
> + goto remove_core;
>
> - qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
> + qcom->mode = usb_get_dr_mode(dev);
>
> /* enable vbus override for device mode */
> if (qcom->mode != USB_DR_MODE_HOST)
> @@ -819,20 +987,15 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>
> wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
> device_init_wakeup(&pdev->dev, wakeup_source);
> - device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
>
> qcom->is_suspended = false;
> - pm_runtime_set_active(dev);
> - pm_runtime_enable(dev);
> - pm_runtime_forbid(dev);
>
> return 0;
>
> interconnect_exit:
> dwc3_qcom_interconnect_exit(qcom);
> -depopulate:
> - of_platform_depopulate(&pdev->dev);
> - platform_device_put(qcom->dwc3);
> +remove_core:
> + dwc3_remove(qcom->dwc);
> clk_disable:
> for (i = qcom->num_clocks - 1; i >= 0; i--) {
> clk_disable_unprepare(qcom->clks[i]);
> @@ -846,13 +1009,11 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>
> static void dwc3_qcom_remove(struct platform_device *pdev)
> {
> - struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
> + struct dwc3 *dwc = platform_get_drvdata(pdev);
> + struct dwc3_qcom *qcom = dwc->glue;
> struct device *dev = &pdev->dev;
> int i;
>
> - of_platform_depopulate(&pdev->dev);
> - platform_device_put(qcom->dwc3);
> -
> for (i = qcom->num_clocks - 1; i >= 0; i--) {
> clk_disable_unprepare(qcom->clks[i]);
> clk_put(qcom->clks[i]);
> @@ -868,10 +1029,15 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>
> static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
> {
> - struct dwc3_qcom *qcom = dev_get_drvdata(dev);
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + struct dwc3_qcom *qcom = dwc->glue;
> bool wakeup = device_may_wakeup(dev);
> int ret;
>
> + ret = dwc3_suspend(qcom->dwc);
> + if (ret)
> + return ret;
> +
> ret = dwc3_qcom_suspend(qcom, wakeup);
> if (ret)
> return ret;
> @@ -883,7 +1049,8 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
>
> static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
> {
> - struct dwc3_qcom *qcom = dev_get_drvdata(dev);
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + struct dwc3_qcom *qcom = dwc->glue;
> bool wakeup = device_may_wakeup(dev);
> int ret;
>
> @@ -893,31 +1060,56 @@ static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
>
> qcom->pm_suspended = false;
>
> + ret = dwc3_suspend(qcom->dwc);
> + if (ret)
> + return ret;
> +
> return 0;
> }
>
> static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
> {
> - struct dwc3_qcom *qcom = dev_get_drvdata(dev);
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + struct dwc3_qcom *qcom = dwc->glue;
> + int ret;
> +
> + ret = dwc3_runtime_suspend(qcom->dwc);
> + if (ret)
> + return ret;
>
> return dwc3_qcom_suspend(qcom, true);
> }
>
> +static void __maybe_unused dwc3_qcom_complete(struct device *dev)
> +{
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> +
> + dwc3_complete(dwc);
> +}
> +
> static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev)
> {
> - struct dwc3_qcom *qcom = dev_get_drvdata(dev);
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + struct dwc3_qcom *qcom = dwc->glue;
> + int ret;
> +
> + ret = dwc3_qcom_resume(qcom, true);
> + if (ret)
> + return ret;
>
> - return dwc3_qcom_resume(qcom, true);
> + return dwc3_runtime_resume(qcom->dwc);
> }
>
> static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(dwc3_qcom_pm_suspend, dwc3_qcom_pm_resume)
> SET_RUNTIME_PM_OPS(dwc3_qcom_runtime_suspend, dwc3_qcom_runtime_resume,
> NULL)
> + .complete = dwc3_qcom_complete,
> };
>
> static const struct of_device_id dwc3_qcom_of_match[] = {
> { .compatible = "qcom,dwc3" },
> + { .compatible = "qcom,snps-dwc3" },
> { }
> };
> MODULE_DEVICE_TABLE(of, dwc3_qcom_of_match);
>
> --
> 2.45.2
>
Powered by blists - more mailing lists