Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 17 May 2023 09:35:11 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
Cc:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Felipe Balbi <balbi@kernel.org>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, quic_pkondeti@quicinc.com,
        quic_ppratap@quicinc.com, quic_wcheng@quicinc.com,
        quic_jackp@quicinc.com, quic_harshq@quicinc.com,
        ahalaney@redhat.com
Subject: Re: [PATCH v8 3/9] usb: dwc3: core: Access XHCI address space
 temporarily to read port info
Message-ID: <ZGSDr08h3Go1Dk8C@hovoldconsulting.com>
References: <20230514054917.21318-1-quic_kriskura@quicinc.com>
 <20230514054917.21318-4-quic_kriskura@quicinc.com>
 <ZGNy6FvVrBjYmorz@hovoldconsulting.com>
 <b2954b92-8b12-700a-af50-b914af7b0ace@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b2954b92-8b12-700a-af50-b914af7b0ace@quicinc.com>
Precedence: bulk

Hi Krishna,

Please try to remember to trim unneeded context when replying so that
it's easier to find your replies and also to catch up on threads (e.g.
when later reading the mail archives).

On Tue, May 16, 2023 at 08:32:00PM +0530, Krishna Kurapati PSSNV wrote:
> On 5/16/2023 5:41 PM, Johan Hovold wrote:
> > On Sun, May 14, 2023 at 11:19:11AM +0530, Krishna Kurapati wrote:

> > You should not make another copy of xhci_find_next_ext_cap(), but rather
> > use it directly.
> > 
> > We already have drivers outside of usb/host using this function so it
> > should be fine to do the same for now:
> > 
> > 	#include "../host/xhci-ext-caps.h"

>    This was the approach which we followed when we first introduced the 
> patch [1]. But Thinh suggested to duplicate code so that we can avoid 
> any dependency on xhci (which seems to be right). So since its just one 
> function, I duplicated it here.

Ok, fair enough. I still think we should not be duplicating the
xhci definitions like this even if we were to copy the helper to avoid
any future dependencies on xhci (it's currently an inline function,
which is also not very nice).

I'll take closer look at the rest of the series as there are a few more
of these layering violations which we should try to avoid.

> >> +	offset = dwc3_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_usb3_ports += XHCI_EXT_PORT_COUNT(temp);
> >> +		} else if (major_revision <= 0x02) {
> >> +			dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp);
> >> +		} else {
> >> +			dev_err(dwc->dev,
> >> +				"Unrecognized port major revision %d\n", major_revision);

> > Perhaps this should be handles as in xhci core by simply warning and
> > continuing instead.
> > 
> I broke the loop and went to unmap as we are not sure what values would 
> be read. Any use of continuing ?

Mostly to align with xhci core which currently handles this case. It
would not not work unless you get rid of the max-ports check below
though.
 
> >> +			ret = -EINVAL;
> >> +			goto unmap_reg;
> >> +		}
> >> +
> >> +		offset = dwc3_xhci_find_next_ext_cap(regs, offset,
> >> +						XHCI_EXT_CAPS_PROTOCOL);
> >> +	}
> >> +
> >> +	temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
> >> +	if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) {
> >> +		dev_err(dwc->dev,
> >> +			"Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp));
> >> +		ret = -EINVAL;
> >> +		goto unmap_reg;
> >> +	}
> > 
> > Not sure this is needed either.
> > 
> > Could this risk regressing platforms which does not have currently have
> > all PHYs described in DT?
> > 
> No, it doesn't. AFAIK, this only tells how many ports are present as per 
> the core consultant configuration of the device. I tried to explain what 
> would happen incase phy's are not present in DT in [2] & [3].

Right, whether the PHYs are described in DT is not directly related to
this.

As long as HCS_MAX_PORTS by definition (assumption) is always
(dwc->num_usb3_ports + dwc->num_usb2_ports) any such machines would
continue to work.

But if you want to catch machines where this assumption does not hold,
you could also end up regressing machines which have so far been working
despite these numbers not adding up.

That may be acceptable, but I'm still not sure what the value of this
check is (e.g. as xhci core will handle basic sanity checks like usb2 +
usb3 <= max_ports).

Johan