| 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: <ZTJPBcyZ_zLXbgE5@hovoldconsulting.com>
Date: Fri, 20 Oct 2023 11:57:25 +0200
From: Johan Hovold <johan@...nel.org>
To: Krishna Kurapati <quic_kriskura@...cinc.com>
Cc: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Felipe Balbi <balbi@...nel.org>,
Wesley Cheng <quic_wcheng@...cinc.com>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
quic_pkondeti@...cinc.com, quic_ppratap@...cinc.com,
quic_jackp@...cinc.com, ahalaney@...hat.com,
quic_shazhuss@...cinc.com, Harsh Agarwal <quic_harshq@...cinc.com>,
kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support
Multiport Controller
On Sat, Oct 07, 2023 at 09:17:59PM +0530, Krishna Kurapati wrote:
> From: Harsh Agarwal <quic_harshq@...cinc.com>
>
> Currently the DWC3 driver supports only single port controller
> which requires at most one HS and one SS PHY.
Should that not be "at least one HS PHY and at most one SS PHY"?
> But the DWC3 USB controller can be connected to multiple ports and
> each port can have their own PHYs. Each port of the multiport
> controller can either be HS+SS capable or HS only capable
> Proper quantification of them is required to modify GUSB2PHYCFG
> and GUSB3PIPECTL registers appropriately.
>
> Add support for detecting, obtaining and configuring phy's supported
"PHYs" for consistency, no apostrophe
> by a multiport controller and. Limit the max number of ports
"and." what? Looks like part of the sentence is missing? Or just drop
" and"?
> supported to 4 as only SC8280 which is a quad port controller supports
s/4/four/
Just change this to
Limit support to multiport controllers with up to four ports for
now (e.g. as needed for SC8280XP).
> Multiport currently.
You use capitalised "Multiport" in several places it seems. Is this an
established term for these controllers or should it just be "multiport"
or "multiple ports"?
> Reported-by: kernel test robot <lkp@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202309200156.CxQ3yaLY-lkp@intel.com/
Drop these two lines, as people have already suggested.
> Co-developed-by: Harsh Agarwal <quic_harshq@...cinc.com>
> Signed-off-by: Harsh Agarwal <quic_harshq@...cinc.com>
> Co-developed-by:Krishna Kurapati <quic_kriskura@...cinc.com>
> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
Thinh pointed out the problems with the above which were also reported
by checkpatch.pl.
> ---
> Changes in v13:
> Compiler issues found by kernel test robot have been fixed and tags added.
> So removing maintainers reviewed-by tag as we have made a minor change
> in the patch.
In general this is the right thing to do when the change in question was
non-trivial. I'm not sure that's the case here, but the robots tend to
complain about smaller (but sometimes important) things.
> @@ -748,23 +781,32 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
> static int dwc3_phy_init(struct dwc3 *dwc)
> {
> int ret;
> + int i;
> + int j;
These could be declared on one line (same throughout).
> usb_phy_init(dwc->usb2_phy);
> usb_phy_init(dwc->usb3_phy);
>
> - ret = phy_init(dwc->usb2_generic_phy);
> - if (ret < 0)
> - goto err_shutdown_usb3_phy;
> + for (i = 0; i < dwc->num_usb2_ports; i++) {
> + ret = phy_init(dwc->usb2_generic_phy[i]);
> + if (ret < 0)
> + goto err_exit_phy;
>
> - ret = phy_init(dwc->usb3_generic_phy);
> - if (ret < 0)
> - goto err_exit_usb2_phy;
> + ret = phy_init(dwc->usb3_generic_phy[i]);
> + if (ret < 0) {
> + phy_exit(dwc->usb2_generic_phy[i]);
> + goto err_exit_phy;
> + }
> + }
>
> return 0;
>
> -err_exit_usb2_phy:
> - phy_exit(dwc->usb2_generic_phy);
> -err_shutdown_usb3_phy:
> +err_exit_phy:
> + for (j = i - 1; j >= 0; j--) {
> + phy_exit(dwc->usb2_generic_phy[j]);
> + phy_exit(dwc->usb3_generic_phy[j]);
Try to always unwind in reverse order so in this case move phy_exit()
for usb3 before usb2 here too.
> + }
> +
> usb_phy_shutdown(dwc->usb3_phy);
> usb_phy_shutdown(dwc->usb2_phy);
> @@ -783,23 +829,32 @@ static void dwc3_phy_exit(struct dwc3 *dwc)
> static int dwc3_phy_power_on(struct dwc3 *dwc)
> {
> int ret;
> + int i;
> + int j;
>
> usb_phy_set_suspend(dwc->usb2_phy, 0);
> usb_phy_set_suspend(dwc->usb3_phy, 0);
>
> - ret = phy_power_on(dwc->usb2_generic_phy);
> - if (ret < 0)
> - goto err_suspend_usb3_phy;
> + for (i = 0; i < dwc->num_usb2_ports; i++) {
> + ret = phy_power_on(dwc->usb2_generic_phy[i]);
> + if (ret < 0)
> + goto err_power_off_phy;
>
> - ret = phy_power_on(dwc->usb3_generic_phy);
> - if (ret < 0)
> - goto err_power_off_usb2_phy;
> + ret = phy_power_on(dwc->usb3_generic_phy[i]);
> + if (ret < 0) {
> + phy_power_off(dwc->usb2_generic_phy[i]);
> + goto err_power_off_phy;
> + }
> + }
>
> return 0;
>
> -err_power_off_usb2_phy:
> - phy_power_off(dwc->usb2_generic_phy);
> -err_suspend_usb3_phy:
> +err_power_off_phy:
> + for (j = i - 1; j >= 0; j--) {
> + phy_power_off(dwc->usb2_generic_phy[j]);
> + phy_power_off(dwc->usb3_generic_phy[j]);
Same here.
> + }
> +
> usb_phy_set_suspend(dwc->usb3_phy, 1);
> usb_phy_set_suspend(dwc->usb2_phy, 1);
> @@ -1346,7 +1410,9 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
> {
> struct device *dev = dwc->dev;
> struct device_node *node = dev->of_node;
> + char phy_name[13];
> int ret;
> + int i;
>
> if (node) {
> dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> @@ -1372,22 +1438,36 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
> return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> }
>
> - dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> - if (IS_ERR(dwc->usb2_generic_phy)) {
> - ret = PTR_ERR(dwc->usb2_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb2_generic_phy = NULL;
> + for (i = 0; i < dwc->num_usb2_ports; i++) {
> + if (dwc->num_usb2_ports == 1)
> + sprintf(phy_name, "usb2-phy");
> else
> - return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> - }
> + sprintf(phy_name, "usb2-port%d", i);
>
> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> - if (IS_ERR(dwc->usb3_generic_phy)) {
> - ret = PTR_ERR(dwc->usb3_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb3_generic_phy = NULL;
> + dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name);
> + if (IS_ERR(dwc->usb2_generic_phy[i])) {
> + ret = PTR_ERR(dwc->usb2_generic_phy[i]);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb2_generic_phy[i] = NULL;
> + else
> + return dev_err_probe(dev, ret,
> + "failed to lookup phy %s\n", phy_name);
Continuation lines should be intented at least two tabs further.
I generally suggest adding brackets around blocks with multiline
statements to improve readability but if you move the string to the
previous line and intend the continuation line (phy_name) one tab more I
guess that's fine.
> + }
> +
> + if (dwc->num_usb2_ports == 1)
> + sprintf(phy_name, "usb3-phy");
> else
> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + sprintf(phy_name, "usb3-port%d", i);
> +
> + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name);
> + if (IS_ERR(dwc->usb3_generic_phy[i])) {
> + ret = PTR_ERR(dwc->usb3_generic_phy[i]);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb3_generic_phy[i] = NULL;
> + else
> + return dev_err_probe(dev, ret,
> + "failed to lookup phy %s\n", phy_name);
Same here.
> + }
> }
>
> return 0;
> @@ -1892,9 +1975,12 @@ static int dwc3_read_port_info(struct dwc3 *dwc)
>
> dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
> dwc->num_usb2_ports, dwc->num_usb3_ports);
> -
Drop this random change.
> iounmap(base);
>
> + if ((dwc->num_usb2_ports > DWC3_MAX_PORTS) ||
> + (dwc->num_usb3_ports > DWC3_MAX_PORTS))
Again, continuation lines should be indented at least two tabs further.
> + return -ENOMEM;
> +
> return 0;
> }
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2ea6df7e6571..fc5d15edab1c 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -33,6 +33,9 @@
>
> #include <linux/power_supply.h>
>
> +/* Number of ports supported by a multiport controller */
/*
* Maximum number of ports currently supported for multiport
* controllers.
*/
> +#define DWC3_MAX_PORTS 4
> +
> #define DWC3_MSG_MAX 500
>
> /* Global constants */
> @@ -1029,8 +1032,8 @@ struct dwc3_scratchpad_array {
> * @usb_psy: pointer to power supply interface.
> * @usb2_phy: pointer to USB2 PHY
> * @usb3_phy: pointer to USB3 PHY
> - * @usb2_generic_phy: pointer to USB2 PHY
> - * @usb3_generic_phy: pointer to USB3 PHY
> + * @usb2_generic_phy: pointer to array of USB2 PHY
> + * @usb3_generic_phy: pointer to array of USB3 PHY
s/PHY/PHYs/
> * @num_usb2_ports: number of USB2 ports
> * @num_usb3_ports: number of USB3 ports
> * @phys_ready: flag to indicate that PHYs are ready
Johan
Powered by blists - more mailing lists