[PATCH v4 3/4] PCI: cadence: Check whether MSI is masked before sending it

[PATCH v4 3/4] PCI: cadence: Check whether MSI is masked before sending it

[Alan Douglas]

The EP driver did not check the mask bit for each MSI before
sending it in raise_irq.  This is now checked, and -EINVAL is
returned if masked.

Fixes: 37dddf14f1ae ("PCI: cadence: Add EndPoint Controller driver for Cadence PCIe controller")
Signed-off-by: Alan Douglas <adouglas@cadence.com>
---
 drivers/pci/controller/pcie-cadence-ep.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/pcie-cadence-ep.c b/drivers/pci/controller/pcie-cadence-ep.c
index c3a0889..b762214 100644
--- a/drivers/pci/controller/pcie-cadence-ep.c
+++ b/drivers/pci/controller/pcie-cadence-ep.c
@@ -332,6 +332,11 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn,
 	if (!interrupt_num || interrupt_num > msi_count)
 		return -EINVAL;
 
+	/* Check whether MSI is masked */
+	data = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_MASK_64);
+	if (data & (1 << (interrupt_num - 1)))
+		return -EINVAL;
+
 	/* Compute the data value to be written. */
 	data_mask = msi_count - 1;
 	data = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_DATA_64);
-- 
1.9.0

Re: [PATCH v4 3/4] PCI: cadence: Check whether MSI is masked before sending it

[Lorenzo Pieralisi]

On Thu, Oct 11, 2018 at 05:16:11PM +0100, Alan Douglas wrote:
> The EP driver did not check the mask bit for each MSI before
> sending it in raise_irq.  This is now checked, and -EINVAL is
> returned if masked.
> 
> Fixes: 37dddf14f1ae ("PCI: cadence: Add EndPoint Controller driver for Cadence PCIe controller")
> Signed-off-by: Alan Douglas <adouglas@cadence.com>
> ---
>  drivers/pci/controller/pcie-cadence-ep.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-cadence-ep.c b/drivers/pci/controller/pcie-cadence-ep.c
> index c3a0889..b762214 100644
> --- a/drivers/pci/controller/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/pcie-cadence-ep.c
> @@ -332,6 +332,11 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn,
>  	if (!interrupt_num || interrupt_num > msi_count)
>  		return -EINVAL;
>  
> +	/* Check whether MSI is masked */
> +	data = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_MASK_64);
> +	if (data & (1 << (interrupt_num - 1)))
> +		return -EINVAL;
> +

By looking at this, I think this is wrong too. To the best of my
knowledge a masked MSI should be still delivered when the MSI is
unmasked, otherwise the condition that triggered the MSI is lost.

There must be code in the EP subsystem that records the "pending" MSI
and deliver it when the MSI is actually unmasked (I assumed this is
done in HW but I need your clarification on this).

Either that or the return value must signal a masked MSI so that the
code calling send_msi_irq() can reissue it (when though ?).

I will have to drop this patch too unless you convince me I am wrong; I
still want to understand how this code is *currently* working, I
understand that for the time being the only EP device we have is the
test one but this code path does not look very solid to me.

Thanks,
Lorenzo

>  	/* Compute the data value to be written. */
>  	data_mask = msi_count - 1;
>  	data = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_DATA_64);
> -- 
> 1.9.0
> 

RE: [PATCH v4 3/4] PCI: cadence: Check whether MSI is masked before sending it

[Alan Douglas]

On 16 October 2018 17:01, Lorenzo Pieralisi wrote:
> On Thu, Oct 11, 2018 at 05:16:11PM +0100, Alan Douglas wrote:
> > The EP driver did not check the mask bit for each MSI before
> > sending it in raise_irq.  This is now checked, and -EINVAL is
> > returned if masked.
> >
> > Fixes: 37dddf14f1ae ("PCI: cadence: Add EndPoint Controller driver for Cadence PCIe controller")
> > Signed-off-by: Alan Douglas <adouglas@cadence.com>
> > ---
> >  drivers/pci/controller/pcie-cadence-ep.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-cadence-ep.c b/drivers/pci/controller/pcie-cadence-ep.c
> > index c3a0889..b762214 100644
> > --- a/drivers/pci/controller/pcie-cadence-ep.c
> > +++ b/drivers/pci/controller/pcie-cadence-ep.c
> > @@ -332,6 +332,11 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn,
> >  	if (!interrupt_num || interrupt_num > msi_count)
> >  		return -EINVAL;
> >
> > +	/* Check whether MSI is masked */
> > +	data = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_MASK_64);
> > +	if (data & (1 << (interrupt_num - 1)))
> > +		return -EINVAL;
> > +
> 
> By looking at this, I think this is wrong too. To the best of my
> knowledge a masked MSI should be still delivered when the MSI is
> unmasked, otherwise the condition that triggered the MSI is lost.
> 
In fact, the driver clears the per-vector masking capable bit in the Message
Control register, so this check should not be needed.  Please drop this
patch .

I was returning -EINVAL so that the calling code would be aware that the
MSI was not delivered.

> There must be code in the EP subsystem that records the "pending" MSI
> and deliver it when the MSI is actually unmasked (I assumed this is
> done in HW but I need your clarification on this).
> 
If per-vector masking is enabled, the EP should set the pending bit when
wishing to send a currently masked MSI.  The HW will generate an
local interrupt on any change to the MSI mask register, allowing the
driver to send any pending MSI if they are unmasked.  I would need
to add driver support for this if enabling PVM.  The question then is
what to return from send_msi_irq(), and whether/how to let the
calling code know when/if the MSI was eventually delivered.

> Either that or the return value must signal a masked MSI so that the
> code calling send_msi_irq() can reissue it (when though ?).
>
The only way I see to do this would be call send_msi_irq() until it
succeeds, this would be effectively polling the MSI mask register.

> I will have to drop this patch too unless you convince me I am wrong; I
> still want to understand how this code is *currently* working, I
> understand that for the time being the only EP device we have is the
> test one but this code path does not look very solid to me.
> 
As above I don't want to convince you to take this patch.  However,
since PVM is currently disabled for the device, the original code
path looks OK to me.  Let me know if you have other concerns
about it.

Thanks,
Alan

> Thanks,
> Lorenzo
> 
> >  	/* Compute the data value to be written. */
> >  	data_mask = msi_count - 1;
> >  	data = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_MSI_DATA_64);
> > --
> > 1.9.0
> >