diff mbox series

[v12,6/8] PCI: dwc: Implement special ISR handler for split MSI IRQ setup

Message ID 20220523181836.2019180-7-dmitry.baryshkov@linaro.org (mailing list archive)
State Superseded
Headers show
Series PCI: qcom: Fix higher MSI vectors handling | expand

Commit Message

Dmitry Baryshkov May 23, 2022, 6:18 p.m. UTC 
If the PCIe DWC controller uses split MSI IRQs for reporting MSI
vectors, it is possible to detect, which group triggered the interrupt.
Provide an optimized version of MSI ISR handler that will handle just a
single MSI group instead of handling all of them.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../pci/controller/dwc/pcie-designware-host.c | 86 ++++++++++++++-----
 1 file changed, 65 insertions(+), 21 deletions(-)

Comments

kernel test robot May 24, 2022, 12:18 a.m. UTC | #1 
Hi Dmitry,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on helgaas-pci/next]
[cannot apply to robh/for-next v5.18]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Dmitry-Baryshkov/PCI-qcom-Fix-higher-MSI-vectors-handling/20220524-024956
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20220524/202205240801.7l6SF2Hv-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/35aa0471e2ee4f0f21b01fbcfe6a4a425e968596
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Dmitry-Baryshkov/PCI-qcom-Fix-higher-MSI-vectors-handling/20220524-024956
        git checkout 35aa0471e2ee4f0f21b01fbcfe6a4a425e968596
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/pci/controller/dwc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/pci/controller/dwc/pcie-designware-host.c: In function 'dw_split_msi_isr':
>> drivers/pci/controller/dwc/pcie-designware-host.c:116:25: warning: variable 'pci' set but not used [-Wunused-but-set-variable]
     116 |         struct dw_pcie *pci;
         |                         ^~~


vim +/pci +116 drivers/pci/controller/dwc/pcie-designware-host.c

   108	
   109	static void dw_split_msi_isr(struct irq_desc *desc)
   110	{
   111		struct irq_chip *chip = irq_desc_get_chip(desc);
   112		int irq = irq_desc_get_irq(desc);
   113		struct pcie_port *pp;
   114		int i;
   115		u32 num_ctrls;
 > 116		struct dw_pcie *pci;
   117	
   118		chained_irq_enter(chip, desc);
   119	
   120		pp = irq_desc_get_handler_data(desc);
   121		pci = to_dw_pcie_from_pp(pp);
   122	
   123		/*
   124		 * Unlike generic dw_handle_msi_irq(), we can determine which group of
   125		 * MSIs triggered the IRQ, so process just that group.
   126		 */
   127		num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
   128	
   129		for (i = 0; i < num_ctrls; i++) {
   130			if (pp->msi_irq[i] == irq) {
   131				dw_handle_single_msi_group(pp, i);
   132				break;
   133			}
   134		}
   135	
   136		WARN_ON_ONCE(i == num_ctrls);
   137	
   138		chained_irq_exit(chip, desc);
   139	}
   140
Rob Herring (Arm) May 26, 2022, 6:42 p.m. UTC | #2 
On Mon, May 23, 2022 at 09:18:34PM +0300, Dmitry Baryshkov wrote:
> If the PCIe DWC controller uses split MSI IRQs for reporting MSI
> vectors, it is possible to detect, which group triggered the interrupt.
> Provide an optimized version of MSI ISR handler that will handle just a
> single MSI group instead of handling all of them.

A lot more complexity to save 7 register reads...

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 86 ++++++++++++++-----
>  1 file changed, 65 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 98a57249ecaf..2b2de517301a 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -52,34 +52,42 @@ static struct msi_domain_info dw_pcie_msi_domain_info = {
>  	.chip	= &dw_pcie_msi_irq_chip,
>  };
>  
> +static inline irqreturn_t dw_handle_single_msi_group(struct pcie_port *pp, int i)
> +{
> +	int pos;
> +	unsigned long val;
> +	u32 status;
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS +
> +				   (i * MSI_REG_CTRL_BLOCK_SIZE));
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	val = status;
> +	pos = 0;
> +	while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
> +				    pos)) != MAX_MSI_IRQS_PER_CTRL) {

for_each_set_bit() doesn't work here?

> +		generic_handle_domain_irq(pp->irq_domain,
> +					  (i * MAX_MSI_IRQS_PER_CTRL) +
> +					  pos);
> +		pos++;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  /* MSI int handler */
>  irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
>  {
> -	int i, pos;
> -	unsigned long val;
> -	u32 status, num_ctrls;
> +	int i;
> +	u32 num_ctrls;
>  	irqreturn_t ret = IRQ_NONE;
> -	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  
>  	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
>  
> -	for (i = 0; i < num_ctrls; i++) {
> -		status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS +
> -					   (i * MSI_REG_CTRL_BLOCK_SIZE));
> -		if (!status)
> -			continue;
> -
> -		ret = IRQ_HANDLED;
> -		val = status;
> -		pos = 0;
> -		while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
> -					    pos)) != MAX_MSI_IRQS_PER_CTRL) {
> -			generic_handle_domain_irq(pp->irq_domain,
> -						  (i * MAX_MSI_IRQS_PER_CTRL) +
> -						  pos);
> -			pos++;
> -		}
> -	}
> +	for (i = 0; i < num_ctrls; i++)
> +		ret |= dw_handle_single_msi_group(pp, i);
>  
>  	return ret;
>  }
> @@ -98,6 +106,38 @@ static void dw_chained_msi_isr(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> +static void dw_split_msi_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	int irq = irq_desc_get_irq(desc);
> +	struct pcie_port *pp;
> +	int i;
> +	u32 num_ctrls;
> +	struct dw_pcie *pci;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	pp = irq_desc_get_handler_data(desc);
> +	pci = to_dw_pcie_from_pp(pp);
> +
> +	/*
> +	 * Unlike generic dw_handle_msi_irq(), we can determine which group of
> +	 * MSIs triggered the IRQ, so process just that group.
> +	 */
> +	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
> +
> +	for (i = 0; i < num_ctrls; i++) {
> +		if (pp->msi_irq[i] == irq) {
> +			dw_handle_single_msi_group(pp, i);
> +			break;
> +		}
> +	}
> +
> +	WARN_ON_ONCE(i == num_ctrls);
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
>  static void dw_pci_setup_msi_msg(struct irq_data *d, struct msi_msg *msg)
>  {
>  	struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> @@ -336,6 +376,7 @@ static int dw_pcie_msi_host_init(struct pcie_port *pp)
>  	struct platform_device *pdev = to_platform_device(dev);
>  	int ret;
>  	u32 ctrl, num_ctrls;
> +	bool has_split_msi_irq = false;
>  
>  	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++)
>  		pp->irq_mask[ctrl] = ~0;
> @@ -344,6 +385,8 @@ static int dw_pcie_msi_host_init(struct pcie_port *pp)
>  		ret = dw_pcie_parse_split_msi_irq(pp);
>  		if (ret < 0 && ret != -ENXIO)
>  			return ret;
> +		else if (!ret)
> +			has_split_msi_irq = true;
>  	}
>  
>  	if (!pp->num_vectors)
> @@ -372,6 +415,7 @@ static int dw_pcie_msi_host_init(struct pcie_port *pp)
>  	for (ctrl = 0; ctrl < num_ctrls; ctrl++)
>  		if (pp->msi_irq[ctrl] > 0)
>  			irq_set_chained_handler_and_data(pp->msi_irq[ctrl],
> +							 has_split_msi_irq ? dw_split_msi_isr :
>  							 dw_chained_msi_isr,
>  							 pp);
>  
> -- 
> 2.35.1
>
Dmitry Baryshkov May 26, 2022, 8:29 p.m. UTC | #3 
On Thu, 26 May 2022 at 21:42, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, May 23, 2022 at 09:18:34PM +0300, Dmitry Baryshkov wrote:
> > If the PCIe DWC controller uses split MSI IRQs for reporting MSI
> > vectors, it is possible to detect, which group triggered the interrupt.
> > Provide an optimized version of MSI ISR handler that will handle just a
> > single MSI group instead of handling all of them.
>
> A lot more complexity to save 7 register reads...

Thus it is a separate patch. It can be dropped w/o any issues.

>
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  .../pci/controller/dwc/pcie-designware-host.c | 86 ++++++++++++++-----
> >  1 file changed, 65 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 98a57249ecaf..2b2de517301a 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -52,34 +52,42 @@ static struct msi_domain_info dw_pcie_msi_domain_info = {
> >       .chip   = &dw_pcie_msi_irq_chip,
> >  };
> >
> > +static inline irqreturn_t dw_handle_single_msi_group(struct pcie_port *pp, int i)
> > +{
> > +     int pos;
> > +     unsigned long val;
> > +     u32 status;
> > +     struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +
> > +     status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS +
> > +                                (i * MSI_REG_CTRL_BLOCK_SIZE));
> > +     if (!status)
> > +             return IRQ_NONE;
> > +
> > +     val = status;
> > +     pos = 0;
> > +     while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
> > +                                 pos)) != MAX_MSI_IRQS_PER_CTRL) {
>
> for_each_set_bit() doesn't work here?

Good question, I just moved the existing DWC code.

>
> > +             generic_handle_domain_irq(pp->irq_domain,
> > +                                       (i * MAX_MSI_IRQS_PER_CTRL) +
> > +                                       pos);
> > +             pos++;
> > +     }
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> >  /* MSI int handler */
> >  irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
> >  {
> > -     int i, pos;
> > -     unsigned long val;
> > -     u32 status, num_ctrls;
> > +     int i;
> > +     u32 num_ctrls;
> >       irqreturn_t ret = IRQ_NONE;
> > -     struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >
> >       num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
> >
> > -     for (i = 0; i < num_ctrls; i++) {
> > -             status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS +
> > -                                        (i * MSI_REG_CTRL_BLOCK_SIZE));
> > -             if (!status)
> > -                     continue;
> > -
> > -             ret = IRQ_HANDLED;
> > -             val = status;
> > -             pos = 0;
> > -             while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
> > -                                         pos)) != MAX_MSI_IRQS_PER_CTRL) {
> > -                     generic_handle_domain_irq(pp->irq_domain,
> > -                                               (i * MAX_MSI_IRQS_PER_CTRL) +
> > -                                               pos);
> > -                     pos++;
> > -             }
> > -     }
> > +     for (i = 0; i < num_ctrls; i++)
> > +             ret |= dw_handle_single_msi_group(pp, i);
> >
> >       return ret;
> >  }
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 98a57249ecaf..2b2de517301a 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -52,34 +52,42 @@  static struct msi_domain_info dw_pcie_msi_domain_info = {
 	.chip	= &dw_pcie_msi_irq_chip,
 };
 
+static inline irqreturn_t dw_handle_single_msi_group(struct pcie_port *pp, int i)
+{
+	int pos;
+	unsigned long val;
+	u32 status;
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS +
+				   (i * MSI_REG_CTRL_BLOCK_SIZE));
+	if (!status)
+		return IRQ_NONE;
+
+	val = status;
+	pos = 0;
+	while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
+				    pos)) != MAX_MSI_IRQS_PER_CTRL) {
+		generic_handle_domain_irq(pp->irq_domain,
+					  (i * MAX_MSI_IRQS_PER_CTRL) +
+					  pos);
+		pos++;
+	}
+
+	return IRQ_HANDLED;
+}
+
 /* MSI int handler */
 irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 {
-	int i, pos;
-	unsigned long val;
-	u32 status, num_ctrls;
+	int i;
+	u32 num_ctrls;
 	irqreturn_t ret = IRQ_NONE;
-	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 
 	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
 
-	for (i = 0; i < num_ctrls; i++) {
-		status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS +
-					   (i * MSI_REG_CTRL_BLOCK_SIZE));
-		if (!status)
-			continue;
-
-		ret = IRQ_HANDLED;
-		val = status;
-		pos = 0;
-		while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
-					    pos)) != MAX_MSI_IRQS_PER_CTRL) {
-			generic_handle_domain_irq(pp->irq_domain,
-						  (i * MAX_MSI_IRQS_PER_CTRL) +
-						  pos);
-			pos++;
-		}
-	}
+	for (i = 0; i < num_ctrls; i++)
+		ret |= dw_handle_single_msi_group(pp, i);
 
 	return ret;
 }
@@ -98,6 +106,38 @@  static void dw_chained_msi_isr(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static void dw_split_msi_isr(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int irq = irq_desc_get_irq(desc);
+	struct pcie_port *pp;
+	int i;
+	u32 num_ctrls;
+	struct dw_pcie *pci;
+
+	chained_irq_enter(chip, desc);
+
+	pp = irq_desc_get_handler_data(desc);
+	pci = to_dw_pcie_from_pp(pp);
+
+	/*
+	 * Unlike generic dw_handle_msi_irq(), we can determine which group of
+	 * MSIs triggered the IRQ, so process just that group.
+	 */
+	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
+
+	for (i = 0; i < num_ctrls; i++) {
+		if (pp->msi_irq[i] == irq) {
+			dw_handle_single_msi_group(pp, i);
+			break;
+		}
+	}
+
+	WARN_ON_ONCE(i == num_ctrls);
+
+	chained_irq_exit(chip, desc);
+}
+
 static void dw_pci_setup_msi_msg(struct irq_data *d, struct msi_msg *msg)
 {
 	struct pcie_port *pp = irq_data_get_irq_chip_data(d);
@@ -336,6 +376,7 @@  static int dw_pcie_msi_host_init(struct pcie_port *pp)
 	struct platform_device *pdev = to_platform_device(dev);
 	int ret;
 	u32 ctrl, num_ctrls;
+	bool has_split_msi_irq = false;
 
 	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++)
 		pp->irq_mask[ctrl] = ~0;
@@ -344,6 +385,8 @@  static int dw_pcie_msi_host_init(struct pcie_port *pp)
 		ret = dw_pcie_parse_split_msi_irq(pp);
 		if (ret < 0 && ret != -ENXIO)
 			return ret;
+		else if (!ret)
+			has_split_msi_irq = true;
 	}
 
 	if (!pp->num_vectors)
@@ -372,6 +415,7 @@  static int dw_pcie_msi_host_init(struct pcie_port *pp)
 	for (ctrl = 0; ctrl < num_ctrls; ctrl++)
 		if (pp->msi_irq[ctrl] > 0)
 			irq_set_chained_handler_and_data(pp->msi_irq[ctrl],
+							 has_split_msi_irq ? dw_split_msi_isr :
 							 dw_chained_msi_isr,
 							 pp);