| 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: <966c1001-6d64-9163-0c07-96595156fc8c@quicinc.com>
Date: Sat, 11 Mar 2023 08:34:04 +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 8:24 AM, Krishna Kurapati PSSNV wrote:
>
>
> 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,
>
Hi Thinh,
One reason I didn't mention something like num_hs_ports and sticked to
num_ports is because in core driver, wherever we need to do phy
operations like:
for (i = 0; i < num_ports; i++)
{
phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
}
The intention is as follows:
If number of usb2 ports is 4, the loop can go from 0-3 and its fine.
If number of usb3-ports is 2, we don't know for sure, if the first 2
ports are SS capable or some other ports like (3 and 4) are SS capable.
So instead, I looped all phy operations around all usb2_generic_phy's
and usb3_generic_phy's. If they are NULL, we just bail out inside phy
operation.
While doing so, looping SS Phy operations around num_usb2_ports didn't
sound good. From code view, it would be like we are looping usb3_phy ops
around num_usb2_ports value (logically it is still correct as each port
is atleast HS capable). So to avoid this, I named the variable as
num_ports instead of num_usb2_ports
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