| Message ID | 1539274571-545-1-git-send-email-adouglas@cadence.com |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Fixes for cadence EP driver | expand |
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 >
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 > >
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);
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(+)