linux-kernel - Re: [PATCH v2 6/7] usb: dwc3: qcom: Transition to flattened model

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]

Message-ID: <ZsO5EnMgKj1GxlQA@hu-bjorande-lv.qualcomm.com>
Date: Mon, 19 Aug 2024 14:28:50 -0700
From: Bjorn Andersson <quic_bjorande@...cinc.com>
To: Frank Li <Frank.li@....com>
CC: Bjorn Andersson <andersson@...nel.org>,
        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>
Subject: Re: [PATCH v2 6/7] usb: dwc3: qcom: Transition to flattened model

On Tue, Aug 13, 2024 at 02:33:56PM -0400, Frank Li wrote:
> On Sun, Aug 11, 2024 at 08:12:03PM -0700, Bjorn Andersson wrote:
> > From: Bjorn Andersson <quic_bjorande@...cinc.com>
> >  drivers/usb/dwc3/dwc3-qcom.c | 310 +++++++++++++++++++++++++++++++++++--------
[..]
> > @@ -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?
> 

I like it, thanks for the suggestion.

> >  }
> >
[..]
> > +/* 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.
> 

I tried that, as it would have saved me from having to do the dynamic
rewrite.

But the dwc3 core and host are full of device_property_read*(),
phy_get(), platform_get_irq() etc which operates on the dwc->dev.

I think it can be done, but this felt like a cleaner outcome, in
particular once we transition the DeviceTree source.

As you say, there should be a fair amount of room for duplication here,
so perhaps we can move that to a "glue.c" and share it?

[..]
> > @@ -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()?
> 

Sounds good.

Thank you,
Bjorn