| 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
| ||
|
Date: Mon, 23 Oct 2023 00:11:45 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Johan Hovold <johan@...nel.org>
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 10/20/2023 6:53 PM, Johan Hovold wrote:
> First, drop "QCOM Glue driver" from Subject, you already have the "qcom"
> prefix.
>
> On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote:
>> Refactor setup_irq call to facilitate reading multiport IRQ's along
>
> "IRQs" or just "interrupts"
>
>> with non mulitport ones. Read through the interrupt-names property
>
> "multiport"
>
> Please spell check all your patches (commit messages and code) before
> posting, it's not the reviewers job.
>
>> to figure out, the type of interrupt (DP/DM/HS/SS) and to which port
>> it belongs. Also keep track of port index to calculate port count
>> based on interrupts provided as input in DT.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
>> ---
>> drivers/usb/dwc3/dwc3-qcom.c | 210 +++++++++++++++++++++++++----------
>> 1 file changed, 154 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index ef2006db7601..863892284146 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -53,14 +53,25 @@
>> #define APPS_USB_AVG_BW 0
>> #define APPS_USB_PEAK_BW MBps_to_icc(40)
>>
>> +#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.
AFAIK, there is only one of this per controller.
>> -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.
>> +static int dwc3_qcom_get_port_index(const char *irq_name, int irq_index)
>> +{
>> + int port_index = -1;
>> +
>> + switch (irq_index) {
>> + case DP_HS_PHY_IRQ_INDEX:
>> + if (strcmp(irq_name, "dp_hs_phy_irq") == 0)
>> + port_index = 1;
>> + else
>> + sscanf(irq_name, "dp_hs_phy_%d", &port_index);
>> + break;
>> +
>
> No need for newlines after break.
>
>> + case DM_HS_PHY_IRQ_INDEX:
>> + if (strcmp(irq_name, "dm_hs_phy_irq") == 0)
>> + port_index = 1;
>> + else
>> + sscanf(irq_name, "dm_hs_phy_%d", &port_index);
>> + break;
>> +
>> + case SS_PHY_IRQ_INDEX:
>> + if (strcmp(irq_name, "ss_phy_irq") == 0)
>> + port_index = 1;
>> + else
>> + sscanf(irq_name, "ss_phy_%d", &port_index);
>> + break;
>> +
>> + case HS_PHY_IRQ_INDEX:
>> + port_index = 1;
>> + break;
>> + }
>> +
>> + if (port_index > DWC3_MAX_PORTS)
>> + port_index = -1;
>> +
>> + return port_index;
>> +}
>
>> static int dwc3_qcom_setup_irq(struct platform_device *pdev)
>> {
>> struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
>> - const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
>> + struct device_node *np = pdev->dev.of_node;
>> + const char **irq_names;
>> + int port_index;
>> + int acpi_index;
>> + int irq_count;
>> + int irq_index;
>> int irq;
>> int ret;
>> + int i;
>>
>> - irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
>> - pdata ? pdata->hs_phy_irq_index : -1);
>> - if (irq > 0) {
>> - ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS", irq);
>> - if (ret)
>> - return ret;
>> - qcom->hs_phy_irq = irq;
>> - }
>> + irq_count = of_property_count_strings(np, "interrupt-names");
>
> of_property_count_strings() can return negative errnos and you don't
> have any sanity checks for the return value...
>
> Please slow down, and also make sure to get your patches reviewed
> internally before posting new revisions.
>
It did go through internal review but probably went un-noticed. Will add
sanity checks here and below.
>> + irq_names = devm_kzalloc(&pdev->dev, sizeof(*irq_names) * irq_count, GFP_KERNEL);
>
> devm_kcalloc()
>
>> + if (!irq_names)
>> + return -ENOMEM;
>>
>> - irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
>> - pdata ? pdata->dp_hs_phy_irq_index : -1);
>> - if (irq > 0) {
>> - ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq", "qcom_dwc3 DP_HS", irq);
>> - if (ret)
>> - return ret;
>> - qcom->dp_hs_phy_irq = irq;
>> - }
>> + ret = of_property_read_string_array(np, "interrupt-names",
>> + irq_names, irq_count);
>
> No sanity check here either?
>
>> + for (i = 0; i < irq_count; i++) {
>> + irq_index = dwc3_qcom_get_irq_index(irq_names[i]);
>> + if (irq_index == -1) {
>> + dev_dbg(&pdev->dev, "Invalid IRQ not handled");
>> + continue;
>> + }
>
> I'll just stop reviewing here. This is a waste of my time.
>
Regards,
Krishna,
Powered by blists - more mailing lists