linux-kernel - Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

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: <ZTY7Lwjd3_8NlfEi@hovoldconsulting.com>
Date:   Mon, 23 Oct 2023 11:21:51 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Krishna Kurapati PSSNV <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
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM
 Glue driver

On Mon, Oct 23, 2023 at 12:11:45AM +0530, Krishna Kurapati PSSNV wrote:
> On 10/20/2023 6:53 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote:

> >> +#define NUM_PHY_IRQ		4
> >> +
> >> +enum dwc3_qcom_ph_index {
> > 
> > "phy_index"
> > 
> >> +	DP_HS_PHY_IRQ_INDEX = 0,
> >> +	DM_HS_PHY_IRQ_INDEX,
> >> +	SS_PHY_IRQ_INDEX,
> >> +	HS_PHY_IRQ_INDEX,
> >> +};
> >> +
> >>   struct dwc3_acpi_pdata {
> >>   	u32			qscratch_base_offset;
> >>   	u32			qscratch_base_size;
> >>   	u32			dwc3_core_base_size;
> >> +	/*
> >> +	 * The phy_irq_index corresponds to ACPI indexes of (in order) DP/DM/SS
> >> +	 * IRQ's respectively.
> >> +	 */
> >> +	int			phy_irq_index[NUM_PHY_IRQ - 1];
> >>   	int			hs_phy_irq_index;
> >> -	int			dp_hs_phy_irq_index;
> >> -	int			dm_hs_phy_irq_index;
> >> -	int			ss_phy_irq_index;
> >>   	bool			is_urs;
> >>   };
> >>   
> >> @@ -73,10 +84,12 @@ struct dwc3_qcom {
> >>   	int			num_clocks;
> >>   	struct reset_control	*resets;
> >>   
> >> +	/*
> >> +	 * The phy_irq corresponds to IRQ's registered for (in order) DP/DM/SS
> >> +	 * respectively.
> >> +	 */
> >> +	int			phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
> >>   	int			hs_phy_irq;
> >> -	int			dp_hs_phy_irq;
> >> -	int			dm_hs_phy_irq;
> >> -	int			ss_phy_irq;
> > 
> > I'm not sure using arrays like this is a good idea (and haven't you
> > switched the indexes above?).
> > 
> > Why not add a port structure instead?
> > 
> > 	struct dwc3_qcom_port {
> > 		int hs_phy_irq;
> > 		int dp_hs_phy_irq;
> > 		int dm_hs_phy_irq;
> > 		int ss_phy_irq;
> > 	};
> > 
> > and then have
> > 
> > 	struct dwc3_qcom_port ports[DWC3_MAX_PORTS];
> > 
> > in dwc3_qcom. The port structure can the later also be amended with
> > whatever other additional per-port data there is need for.
> > 
> > This should make the implementation cleaner.
> > 
> > I also don't like the special handling of hs_phy_irq; if this is really
> > just another name for the pwr_event_irq then this should be cleaned up
> > before making the code more complicated than it needs to be.
> > 
> > Make sure to clarify this before posting a new revision.
> 
> hs_phy_irq is different from pwr_event_irq.

How is it different and how are they used?

> AFAIK, there is only one of this per controller.

But previous controllers were all single port so this interrupt is
likely also per-port, even if your comment below seems to suggest even
SC8280XP has one, which is unexpected (and not described in the updated
binding):

	Yes, all targets have the same IRQ's. Just that MP one's have
	multiple IRQ's of each type. But hs-phy_irq is only one in
	SC8280 as well.

	https://lore.kernel.org/lkml/70b2495f-1305-05b1-2039-9573d171fe24@quicinc.com/

Please clarify.

> >> -static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
> >> -				char *disp_name, int irq)
> >> +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name,
> >> +				const char *disp_name, int irq)
> > 
> > Ok, here you did drop the second name parameter, but without renaming
> > the first and hidden in a long diff without any mention anywhere.
> > 
> I didn't understand the comment. Can you please elaborate.
> I didn't drop the second parameter. In the usage of this call, I passed 
> same value to both irq_name and disp_name:
> 
> "dwc3_qcom_prep_irq(qcom, irq_names[i], irq_names[i], irq);"
> 
> I mentioned in cover-letter that I am using same name as obtained from 
> DT to register the interrupts as well. Should've mentioned that in 
> commit text of this patch. Will do it in next version.

Ah, sorry I misread the diff. You never drop the second name even though
it is now redundant as I pointed on in a comment to one of the earlier
patches.

Johan