[PATCH v2 0/2] PCI: uniphier: Fix INTx masking/unmasking

[PATCH v2 0/2] PCI: uniphier: Fix INTx masking/unmasking

[Kunihiko Hayashi]

This series includes some fixes to INTx masking/unmasking for UniPhier PCIe
host controller driver.

- Remove unnecessary bit clears to INTX mask field
- Remove unnecessary irq_ack() function because write access to status field
  doesn't work
- Add lock into callback functions to avoid race condition

Kunihiko Hayashi (2):
  PCI: uniphier: Fix INTx mask/unmask bit operation and remove ack
    function
  PCI: uniphier: Serialize INTx masking/unmasking

 drivers/pci/controller/dwc/pcie-uniphier.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] PCI: uniphier: Fix INTx mask/unmask bit operation and remove ack function

[Kunihiko Hayashi]

INTX mask and unmask fields in PCL_RCV_INTX register should only be
set/reset for each bit. Clearing by PCL_RCV_INTX_ALL_MASK should be
removed.

INTX status fields in PCL_RCV_INTX register only indicates each INTX
interrupt status, so the handler can't clear by writing 1 to the field.
The status is expected to be cleared by the interrupt origin.
The ack function has no meaning, so should remove it.

Fixes: 7e6d5cd88a6f ("PCI: uniphier: Add UniPhier PCIe host controller support")
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index ebe43e9..26f630c 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -181,19 +181,6 @@ static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
 	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
 }
 
-static void uniphier_pcie_irq_ack(struct irq_data *d)
-{
-	struct pcie_port *pp = irq_data_get_irq_chip_data(d);
-	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
-	u32 val;
-
-	val = readl(priv->base + PCL_RCV_INTX);
-	val &= ~PCL_RCV_INTX_ALL_STATUS;
-	val |= BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_STATUS_SHIFT);
-	writel(val, priv->base + PCL_RCV_INTX);
-}
-
 static void uniphier_pcie_irq_mask(struct irq_data *d)
 {
 	struct pcie_port *pp = irq_data_get_irq_chip_data(d);
@@ -202,7 +189,6 @@ static void uniphier_pcie_irq_mask(struct irq_data *d)
 	u32 val;
 
 	val = readl(priv->base + PCL_RCV_INTX);
-	val &= ~PCL_RCV_INTX_ALL_MASK;
 	val |= BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_MASK_SHIFT);
 	writel(val, priv->base + PCL_RCV_INTX);
 }
@@ -215,14 +201,12 @@ static void uniphier_pcie_irq_unmask(struct irq_data *d)
 	u32 val;
 
 	val = readl(priv->base + PCL_RCV_INTX);
-	val &= ~PCL_RCV_INTX_ALL_MASK;
 	val &= ~BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_MASK_SHIFT);
 	writel(val, priv->base + PCL_RCV_INTX);
 }
 
 static struct irq_chip uniphier_pcie_irq_chip = {
 	.name = "PCI",
-	.irq_ack = uniphier_pcie_irq_ack,
 	.irq_mask = uniphier_pcie_irq_mask,
 	.irq_unmask = uniphier_pcie_irq_unmask,
 };
-- 
2.7.4

Re: [PATCH v2 1/2] PCI: uniphier: Fix INTx mask/unmask bit operation and remove ack function

[Pali Rohár]

On Monday 30 August 2021 11:22:37 Kunihiko Hayashi wrote:
> INTX mask and unmask fields in PCL_RCV_INTX register should only be
> set/reset for each bit. Clearing by PCL_RCV_INTX_ALL_MASK should be
> removed.
> 
> INTX status fields in PCL_RCV_INTX register only indicates each INTX
> interrupt status, so the handler can't clear by writing 1 to the field.
> The status is expected to be cleared by the interrupt origin.
> The ack function has no meaning, so should remove it.
> 
> Fixes: 7e6d5cd88a6f ("PCI: uniphier: Add UniPhier PCIe host controller support")
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

Acked-by: Pali Rohár <pali@kernel.org>

> ---
>  drivers/pci/controller/dwc/pcie-uniphier.c | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> index ebe43e9..26f630c 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -181,19 +181,6 @@ static void uniphier_pcie_irq_enable(struct uniphier_pcie_priv *priv)
>  	writel(PCL_RCV_INTX_ALL_ENABLE, priv->base + PCL_RCV_INTX);
>  }
>  
> -static void uniphier_pcie_irq_ack(struct irq_data *d)
> -{
> -	struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> -	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> -	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
> -	u32 val;
> -
> -	val = readl(priv->base + PCL_RCV_INTX);
> -	val &= ~PCL_RCV_INTX_ALL_STATUS;
> -	val |= BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_STATUS_SHIFT);
> -	writel(val, priv->base + PCL_RCV_INTX);
> -}
> -
>  static void uniphier_pcie_irq_mask(struct irq_data *d)
>  {
>  	struct pcie_port *pp = irq_data_get_irq_chip_data(d);
> @@ -202,7 +189,6 @@ static void uniphier_pcie_irq_mask(struct irq_data *d)
>  	u32 val;
>  
>  	val = readl(priv->base + PCL_RCV_INTX);
> -	val &= ~PCL_RCV_INTX_ALL_MASK;
>  	val |= BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_MASK_SHIFT);
>  	writel(val, priv->base + PCL_RCV_INTX);
>  }
> @@ -215,14 +201,12 @@ static void uniphier_pcie_irq_unmask(struct irq_data *d)
>  	u32 val;
>  
>  	val = readl(priv->base + PCL_RCV_INTX);
> -	val &= ~PCL_RCV_INTX_ALL_MASK;
>  	val &= ~BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_MASK_SHIFT);
>  	writel(val, priv->base + PCL_RCV_INTX);
>  }
>  
>  static struct irq_chip uniphier_pcie_irq_chip = {
>  	.name = "PCI",
> -	.irq_ack = uniphier_pcie_irq_ack,
>  	.irq_mask = uniphier_pcie_irq_mask,
>  	.irq_unmask = uniphier_pcie_irq_unmask,
>  };
> -- 
> 2.7.4
> 

[PATCH v2 2/2] PCI: uniphier: Serialize INTx masking/unmasking

[Kunihiko Hayashi]

The condition register PCI_RCV_INTX is used in irq_mask() and irq_unmask()
callbacks. Accesses to register can occur at the same time without a lock.
Add a lock into each callback to prevent the issue.

Fixes: 7e6d5cd88a6f ("PCI: uniphier: Add UniPhier PCIe host controller support")
Suggested-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Acked-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index 26f630c..0ddeec0 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -186,11 +186,16 @@ static void uniphier_pcie_irq_mask(struct irq_data *d)
 	struct pcie_port *pp = irq_data_get_irq_chip_data(d);
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
+	unsigned long flags;
 	u32 val;
 
+	raw_spin_lock_irqsave(&pp->lock, flags);
+
 	val = readl(priv->base + PCL_RCV_INTX);
 	val |= BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_MASK_SHIFT);
 	writel(val, priv->base + PCL_RCV_INTX);
+
+	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
 
 static void uniphier_pcie_irq_unmask(struct irq_data *d)
@@ -198,11 +203,16 @@ static void uniphier_pcie_irq_unmask(struct irq_data *d)
 	struct pcie_port *pp = irq_data_get_irq_chip_data(d);
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct uniphier_pcie_priv *priv = to_uniphier_pcie(pci);
+	unsigned long flags;
 	u32 val;
 
+	raw_spin_lock_irqsave(&pp->lock, flags);
+
 	val = readl(priv->base + PCL_RCV_INTX);
 	val &= ~BIT(irqd_to_hwirq(d) + PCL_RCV_INTX_MASK_SHIFT);
 	writel(val, priv->base + PCL_RCV_INTX);
+
+	raw_spin_unlock_irqrestore(&pp->lock, flags);
 }
 
 static struct irq_chip uniphier_pcie_irq_chip = {
-- 
2.7.4

Re: [PATCH v2 0/2] PCI: uniphier: Fix INTx masking/unmasking

[Kunihiko Hayashi]

Gentle ping, are there any comments about this series?

Thank you,

On 2021/08/30 11:22, Kunihiko Hayashi wrote:
> This series includes some fixes to INTx masking/unmasking for UniPhier PCIe
> host controller driver.
> 
> - Remove unnecessary bit clears to INTX mask field
> - Remove unnecessary irq_ack() function because write access to status field
>    doesn't work
> - Add lock into callback functions to avoid race condition
> 
> Kunihiko Hayashi (2):
>    PCI: uniphier: Fix INTx mask/unmask bit operation and remove ack
>      function
>    PCI: uniphier: Serialize INTx masking/unmasking
> 
>   drivers/pci/controller/dwc/pcie-uniphier.c | 26 ++++++++++----------------
>   1 file changed, 10 insertions(+), 16 deletions(-)
> 

---
Best Regards
Kunihiko Hayashi

Re: [PATCH v2 0/2] PCI: uniphier: Fix INTx masking/unmasking

[Marc Zyngier]

On Thu, 16 Sep 2021 12:30:52 +0100,
Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote:
> 
> Gentle ping, are there any comments about this series?
> 
> Thank you,
> 
> On 2021/08/30 11:22, Kunihiko Hayashi wrote:
> > This series includes some fixes to INTx masking/unmasking for UniPhier PCIe
> > host controller driver.
> > 
> > - Remove unnecessary bit clears to INTX mask field
> > - Remove unnecessary irq_ack() function because write access to status field
> >    doesn't work
> > - Add lock into callback functions to avoid race condition
> > 
> > Kunihiko Hayashi (2):
> >    PCI: uniphier: Fix INTx mask/unmask bit operation and remove ack
> >      function
> >    PCI: uniphier: Serialize INTx masking/unmasking
> > 
> >   drivers/pci/controller/dwc/pcie-uniphier.c | 26 ++++++++++----------------
> >   1 file changed, 10 insertions(+), 16 deletions(-)
> > 

Patches look OK, although I would personally squash them into a single
one (INTx masking never really worked before that). FWIW:

Acked-by: Marc Zyngier <maz@kernel.org>

	N,

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 0/2] PCI: uniphier: Fix INTx masking/unmasking

[Kunihiko Hayashi]

Hi Marc,

Thank you for your comment.

On 2021/09/17 22:54, Marc Zyngier wrote:
> On Thu, 16 Sep 2021 12:30:52 +0100,
> Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote:
>>
>> Gentle ping, are there any comments about this series?
>>
>> Thank you,
>>
>> On 2021/08/30 11:22, Kunihiko Hayashi wrote:
>>> This series includes some fixes to INTx masking/unmasking for UniPhier PCIe
>>> host controller driver.
>>>
>>> - Remove unnecessary bit clears to INTX mask field
>>> - Remove unnecessary irq_ack() function because write access to status field
>>>     doesn't work
>>> - Add lock into callback functions to avoid race condition
>>>
>>> Kunihiko Hayashi (2):
>>>     PCI: uniphier: Fix INTx mask/unmask bit operation and remove ack
>>>       function
>>>     PCI: uniphier: Serialize INTx masking/unmasking
>>>
>>>    drivers/pci/controller/dwc/pcie-uniphier.c | 26 ++++++++++----------------
>>>    1 file changed, 10 insertions(+), 16 deletions(-)
>>>
> 
> Patches look OK, although I would personally squash them into a single
> one (INTx masking never really worked before that). FWIW:

Surely applying only the first patch leaves the issue with the second one.
I'll squash them in v3.

Thank you,

> 
> Acked-by: Marc Zyngier <maz@kernel.org>
> 
> 	N,
> 

---
Best Regards
Kunihiko Hayashi