| 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: <c23453b3-4f91-aa0a-c45a-de99ac369048@quicinc.com>
Date: Sat, 11 Mar 2023 08:24:01 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC: 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>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"quic_pkondeti@...cinc.com" <quic_pkondeti@...cinc.com>,
"quic_ppratap@...cinc.com" <quic_ppratap@...cinc.com>,
"quic_wcheng@...cinc.com" <quic_wcheng@...cinc.com>,
"quic_jackp@...cinc.com" <quic_jackp@...cinc.com>,
"quic_harshq@...cinc.com" <quic_harshq@...cinc.com>,
"ahalaney@...hat.com" <ahalaney@...hat.com>,
"quic_shazhuss@...cinc.com" <quic_shazhuss@...cinc.com>
Subject: Re: [PATCH 2/8] usb: dwc3: core: Access XHCI address space
temporarily to read port info
On 3/11/2023 5:25 AM, Thinh Nguyen wrote:
> On Fri, Mar 10, 2023, Krishna Kurapati wrote:
>> Currently host-only capable DWC3 controllers support Multiport. Temporarily
>> map XHCI address space for host-only controllers and parse XHCI Extended
>> Capabilities registers to read number of physical usb ports connected to the
>> multiport controller (presuming each port is at least HS capable) and extract
>> info on how many of these ports are Super Speed capable.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
>> ---
>> drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++
>> drivers/usb/dwc3/core.h | 9 +++++
>> 2 files changed, 84 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 476b63618511..076c0f8a4441 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -37,6 +37,7 @@
>> #include "core.h"
>> #include "gadget.h"
>> #include "io.h"
>> +#include "../host/xhci.h"
>
> I think better to duplicate some of the logic in dwc3 driver and avoid
> any direct dependency with the xhci driver.
>
>>
>> #include "debug.h"
>>
>> @@ -1750,6 +1751,65 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
>> return edev;
>> }
>>
>> +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
>> +{
>> + void __iomem *regs;
>> + struct resource dwc_res;
>> + u32 offset;
>> + u32 temp;
>> + u8 major_revision;
>> + int ret = 0;
>> +
>> + /*
>> + * Remap xHCI address space to access XHCI ext cap regs,
>> + * since it is needed to get port info.
>> + */
>> + dwc_res = *res;
>> + dwc_res.start += 0;
>> + dwc_res.end = dwc->xhci_resources[0].start +
>> + DWC3_XHCI_REGS_END;
>
> Isn't dwc->xhci_resources[0] already setup at this point? Can we use
> dwc->xhci_resources[0] directly without copy the setting in dwc_res?
>
>> +
>> + regs = ioremap(dwc_res.start, resource_size(&dwc_res));
>> + if (IS_ERR(regs))
>> + return PTR_ERR(regs);
>> +
>> + offset = xhci_find_next_ext_cap(regs, 0,
>> + XHCI_EXT_CAPS_PROTOCOL);
>> + while (offset) {
>> + temp = readl(regs + offset);
>> + major_revision = XHCI_EXT_PORT_MAJOR(temp);
>> +
>> + temp = readl(regs + offset + 0x08);
>> + if (major_revision == 0x03) {
>> + dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
>> + } else if (major_revision <= 0x02) {
>> + dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
>> + } else {
>> + dev_err(dwc->dev, "port revision seems wrong\n");
>> + ret = -EINVAL;
>> + goto unmap_reg;
>> + }
>> +
>> + offset = xhci_find_next_ext_cap(regs, offset,
>> + XHCI_EXT_CAPS_PROTOCOL);
>> + }
>> +
>> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
>> + if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
>> + dev_err(dwc->dev, "inconsistency in port info\n");
>> + ret = -EINVAL;
>> + goto unmap_reg;
>> + }
>> +
>> + dev_info(dwc->dev,
>> + "num-ports: %d ss-capable: %d\n", dwc->num_ports, dwc->num_ss_ports);
>
> The end user doesn't need to know this info. This should be a debug
> message. Perhaps it can be a tracepoint if needed?
>
>> +
>> +unmap_reg:
>> + iounmap(regs);
>> + return ret;
>> +}
>> +
>> static int dwc3_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -1757,6 +1817,7 @@ static int dwc3_probe(struct platform_device *pdev)
>> struct dwc3 *dwc;
>>
>> int ret;
>> + unsigned int hw_mode;
>>
>> void __iomem *regs;
>>
>> @@ -1880,6 +1941,20 @@ static int dwc3_probe(struct platform_device *pdev)
>> goto disable_clks;
>> }
>>
>> + /*
>> + * Currently DWC3 controllers that are host-only capable
>> + * support Multiport.
>> + */
>> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
>> + ret = dwc3_read_port_info(dwc, res);
>> + if (ret)
>> + goto disable_clks;
>> + } else {
>> + dwc->num_ports = 1;
>> + dwc->num_ss_ports = 1;
>> + }
>> +
>> spin_lock_init(&dwc->lock);
>> mutex_init(&dwc->mutex);
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 582ebd9cf9c2..74386d6a0277 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -35,6 +35,9 @@
>>
>> #define DWC3_MSG_MAX 500
>>
>> +/* XHCI Reg constants */
>> +#define DWC3_XHCI_HCSPARAMS1 0x04
>> +
>> /* Global constants */
>> #define DWC3_PULL_UP_TIMEOUT 500 /* ms */
>> #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */
>> @@ -1023,6 +1026,10 @@ struct dwc3_scratchpad_array {
>> * @usb_psy: pointer to power supply interface.
>> * @usb2_phy: pointer to USB2 PHY
>> * @usb3_phy: pointer to USB3 PHY
>> + * @num_ports: Indicates the number of physical USB ports present on HW
>> + * presuming each port is at least HS capable
>
> This isn't the number of physical USB ports right? That's the number of
> usb2 ports the controller is configured with right?. Perhaps we can use
> num_usb2_ports and num_usb3_ports?
>
Hi Thinh,
Yes, naming this might have created a little confusion.
num_ports is supposed to indicate number of usb2 ports in the controller.
Incase of sa8295 (4 port controller with first two ports having ss
capability), num_ports would be 4 and num_ss_ports would be 2. (and not
6 as what num_ports usually sounds).
I can rename them accordingly in the next version and update the
description as well.
Regards,
Krishna,
>> + * @num_ss_ports: Indicates the number of USB ports present on HW that are
>> + * SS Capable
>> * @usb2_generic_phy: pointer to USB2 PHY
>> * @usb3_generic_phy: pointer to USB3 PHY
>> * @phys_ready: flag to indicate that PHYs are ready
>> @@ -1158,6 +1165,8 @@ struct dwc3 {
>> struct usb_phy *usb2_phy;
>> struct usb_phy *usb3_phy;
>>
>> + u32 num_ports;
>> + u32 num_ss_ports;
>> struct phy *usb2_generic_phy;
>> struct phy *usb3_generic_phy;
>>
>> --
>> 2.39.0
>>
>
> Thanks,
> Thinh
Powered by blists - more mailing lists