[PATCH 0/2] iommu/riscv: Add platform msi support

[PATCH 0/2] iommu/riscv: Add platform msi support

[Andrew Jones]

The first patch is fix for an issue found while preparing the second.
The second patch adds MSI support for a platform IOMMU. The patches
may be tested with QEMU when including [1].

Based on linux-next commit 5b913f5d7d7f ("Add linux-next specific files for 20241106")

[1] https://lore.kernel.org/all/20241106133407.604587-1-dbarboza@ventanamicro.com/


Andrew Jones (2):
  iommu/riscv: Free irq vectors on pci remove
  iommu/riscv: Add support for platform msi

 drivers/iommu/riscv/iommu-pci.c      |   1 +
 drivers/iommu/riscv/iommu-platform.c | 102 ++++++++++++++++++++++-----
 2 files changed, 85 insertions(+), 18 deletions(-)

-- 
2.47.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/2] iommu/riscv: Free irq vectors on pci remove

[Andrew Jones]

riscv_iommu_pci_probe() calls pci_alloc_irq_vectors() which
states pci_free_irq_vectors() must be called on cleanup.

Fixes: 68682e9578fb ("iommu/riscv: Add RISC-V IOMMU PCIe device driver")
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 drivers/iommu/riscv/iommu-pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/riscv/iommu-pci.c b/drivers/iommu/riscv/iommu-pci.c
index c7a89143014c..25a27e627a0e 100644
--- a/drivers/iommu/riscv/iommu-pci.c
+++ b/drivers/iommu/riscv/iommu-pci.c
@@ -99,6 +99,7 @@ static void riscv_iommu_pci_remove(struct pci_dev *pdev)
 	struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev);
 
 	riscv_iommu_remove(iommu);
+	pci_free_irq_vectors(pdev);
 }
 
 static const struct pci_device_id riscv_iommu_pci_tbl[] = {
-- 
2.47.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] iommu/riscv: Free irq vectors on pci remove

[Tomasz Jeznach]

On Wed, Nov 6, 2024 at 9:51 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> riscv_iommu_pci_probe() calls pci_alloc_irq_vectors() which
> states pci_free_irq_vectors() must be called on cleanup.
>
> Fixes: 68682e9578fb ("iommu/riscv: Add RISC-V IOMMU PCIe device driver")
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  drivers/iommu/riscv/iommu-pci.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iommu/riscv/iommu-pci.c b/drivers/iommu/riscv/iommu-pci.c
> index c7a89143014c..25a27e627a0e 100644
> --- a/drivers/iommu/riscv/iommu-pci.c
> +++ b/drivers/iommu/riscv/iommu-pci.c
> @@ -99,6 +99,7 @@ static void riscv_iommu_pci_remove(struct pci_dev *pdev)
>         struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev);
>
>         riscv_iommu_remove(iommu);
> +       pci_free_irq_vectors(pdev);
>  }
>
>  static const struct pci_device_id riscv_iommu_pci_tbl[] = {
> --
> 2.47.0
>

Andrew, interrupt release call pci_free_irq_vectors() should already
be called by device
managed resources framework, with unwind action callback pcim_msi_release().

Callback is registered during vectors allocation:
 pci_alloc_irq_vectors_affinity()
  __pci_enable_msix_range()
   pci_setup_msi_context()
    pcim_setup_msi_release()

This driver enables device resources management with initial call to
pcim_enable_device().

Are there any conditions / testing sequence the release function is not called?

Thanks,
- Tomasz

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] iommu/riscv: Free irq vectors on pci remove

[Andrew Jones]

On Wed, Nov 06, 2024 at 11:00:55AM -0800, Tomasz Jeznach wrote:
> On Wed, Nov 6, 2024 at 9:51 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > riscv_iommu_pci_probe() calls pci_alloc_irq_vectors() which
> > states pci_free_irq_vectors() must be called on cleanup.
> >
> > Fixes: 68682e9578fb ("iommu/riscv: Add RISC-V IOMMU PCIe device driver")
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  drivers/iommu/riscv/iommu-pci.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/iommu/riscv/iommu-pci.c b/drivers/iommu/riscv/iommu-pci.c
> > index c7a89143014c..25a27e627a0e 100644
> > --- a/drivers/iommu/riscv/iommu-pci.c
> > +++ b/drivers/iommu/riscv/iommu-pci.c
> > @@ -99,6 +99,7 @@ static void riscv_iommu_pci_remove(struct pci_dev *pdev)
> >         struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev);
> >
> >         riscv_iommu_remove(iommu);
> > +       pci_free_irq_vectors(pdev);
> >  }
> >
> >  static const struct pci_device_id riscv_iommu_pci_tbl[] = {
> > --
> > 2.47.0
> >
> 
> Andrew, interrupt release call pci_free_irq_vectors() should already
> be called by device
> managed resources framework, with unwind action callback pcim_msi_release().
> 
> Callback is registered during vectors allocation:
>  pci_alloc_irq_vectors_affinity()
>   __pci_enable_msix_range()
>    pci_setup_msi_context()
>     pcim_setup_msi_release()
> 
> This driver enables device resources management with initial call to
> pcim_enable_device().

Ah, thank you for the education and inspiration to look closer at how this
works. It was easy to confirm with a WARN in pci_free_irq_vectors() and an
echo 1 to the IOMMU device's remove sysfs node.

> 
> Are there any conditions / testing sequence the release function is not called?

No, not that I know of, this was just my knee jerk decision to mimic the
call pairing I was doing on the platform side to the pci side. Sorry for
the noise, this patch can be dropped.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/2] iommu/riscv: Add support for platform msi

[Andrew Jones]

Apply platform_device_msi_init_and_alloc_irqs() to add support for
MSIs when the IOMMU is a platform device.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 drivers/iommu/riscv/iommu-platform.c | 102 ++++++++++++++++++++++-----
 1 file changed, 84 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/riscv/iommu-platform.c b/drivers/iommu/riscv/iommu-platform.c
index da336863f152..89aa622bcbde 100644
--- a/drivers/iommu/riscv/iommu-platform.c
+++ b/drivers/iommu/riscv/iommu-platform.c
@@ -11,18 +11,41 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/msi.h>
 
 #include "iommu-bits.h"
 #include "iommu.h"
 
+static void riscv_iommu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
+{
+	struct device *dev = msi_desc_to_dev(desc);
+	struct riscv_iommu_device *iommu = dev_get_drvdata(dev);
+	u16 idx = desc->msi_index;
+	u64 addr;
+
+	addr = ((u64)msg->address_hi << 32) | msg->address_lo;
+
+	if (addr != (addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR))
+		pr_warn_once("uh oh, the IOMMU can't send MSIs to 0x%llx, sending to 0x%llx instead\n",
+			     addr, addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR);
+
+	addr &= RISCV_IOMMU_MSI_CFG_TBL_ADDR;
+
+	riscv_iommu_writeq(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_ADDR(idx), addr);
+	riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_DATA(idx), msg->data);
+	riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_CTRL(idx), 0);
+}
+
 static int riscv_iommu_platform_probe(struct platform_device *pdev)
 {
+	enum riscv_iommu_igs_settings igs;
 	struct device *dev = &pdev->dev;
 	struct riscv_iommu_device *iommu = NULL;
 	struct resource *res = NULL;
-	int vec;
+	int vec, ret;
 
 	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
 	if (!iommu)
@@ -40,16 +63,6 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
 	iommu->caps = riscv_iommu_readq(iommu, RISCV_IOMMU_REG_CAPABILITIES);
 	iommu->fctl = riscv_iommu_readl(iommu, RISCV_IOMMU_REG_FCTL);
 
-	/* For now we only support WSI */
-	switch (FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps)) {
-	case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
-	case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
-		break;
-	default:
-		return dev_err_probe(dev, -ENODEV,
-				     "unable to use wire-signaled interrupts\n");
-	}
-
 	iommu->irqs_count = platform_irq_count(pdev);
 	if (iommu->irqs_count <= 0)
 		return dev_err_probe(dev, -ENODEV,
@@ -57,13 +70,60 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
 	if (iommu->irqs_count > RISCV_IOMMU_INTR_COUNT)
 		iommu->irqs_count = RISCV_IOMMU_INTR_COUNT;
 
-	for (vec = 0; vec < iommu->irqs_count; vec++)
-		iommu->irqs[vec] = platform_get_irq(pdev, vec);
+	igs = FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps);
+	switch (igs) {
+	case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
+	case RISCV_IOMMU_CAPABILITIES_IGS_MSI:
+		if (is_of_node(dev->fwnode))
+			of_msi_configure(dev, to_of_node(dev->fwnode));
+
+		if (!dev_get_msi_domain(dev)) {
+			dev_warn(dev, "failed to find an MSI domain");
+			goto msi_fail;
+		}
+
+		ret = platform_device_msi_init_and_alloc_irqs(dev, iommu->irqs_count,
+							      riscv_iommu_write_msi_msg);
+		if (ret) {
+			dev_warn(dev, "failed to allocate MSIs");
+			goto msi_fail;
+		}
+
+		for (vec = 0; vec < iommu->irqs_count; vec++)
+			iommu->irqs[vec] = msi_get_virq(dev, vec);
+
+		/* Enable message-signaled interrupts, fctl.WSI */
+		if (iommu->fctl & RISCV_IOMMU_FCTL_WSI) {
+			iommu->fctl ^= RISCV_IOMMU_FCTL_WSI;
+			riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
+		}
+
+		dev_info(dev, "using MSIs\n");
+		break;
+
+msi_fail:
+		if (igs != RISCV_IOMMU_CAPABILITIES_IGS_BOTH) {
+			dev_warn(dev, "\n");
+			return dev_err_probe(dev, -ENODEV,
+					     "unable to use wire-signaled interrupts\n");
+		}
+
+		dev_warn(dev, " - falling back to wired irqs\n");
+		fallthrough;
 
-	/* Enable wire-signaled interrupts, fctl.WSI */
-	if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
-		iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
-		riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
+	case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
+		for (vec = 0; vec < iommu->irqs_count; vec++)
+			iommu->irqs[vec] = platform_get_irq(pdev, vec);
+
+		/* Enable wire-signaled interrupts, fctl.WSI */
+		if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
+			iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
+			riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
+		}
+		dev_info(dev, "using wire-signaled interrupts\n");
+		break;
+	default:
+		return dev_err_probe(dev, -ENODEV, "invalid IGS\n");
 	}
 
 	return riscv_iommu_init(iommu);
@@ -71,7 +131,13 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
 
 static void riscv_iommu_platform_remove(struct platform_device *pdev)
 {
-	riscv_iommu_remove(dev_get_drvdata(&pdev->dev));
+	struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev);
+	bool msi = !(iommu->fctl & RISCV_IOMMU_FCTL_WSI);
+
+	riscv_iommu_remove(iommu);
+
+	if (msi)
+		platform_device_msi_free_irqs_all(&pdev->dev);
 };
 
 static const struct of_device_id riscv_iommu_of_match[] = {
-- 
2.47.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] iommu/riscv: Add support for platform msi

[Samuel Holland]

Hi Drew,

On 2024-11-06 11:51 AM, Andrew Jones wrote:
> Apply platform_device_msi_init_and_alloc_irqs() to add support for
> MSIs when the IOMMU is a platform device.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  drivers/iommu/riscv/iommu-platform.c | 102 ++++++++++++++++++++++-----
>  1 file changed, 84 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iommu/riscv/iommu-platform.c b/drivers/iommu/riscv/iommu-platform.c
> index da336863f152..89aa622bcbde 100644
> --- a/drivers/iommu/riscv/iommu-platform.c
> +++ b/drivers/iommu/riscv/iommu-platform.c
> @@ -11,18 +11,41 @@
>   */
>  
>  #include <linux/kernel.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/msi.h>

If you respin, please keep these sorted.

>  
>  #include "iommu-bits.h"
>  #include "iommu.h"
>  
> +static void riscv_iommu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct device *dev = msi_desc_to_dev(desc);
> +	struct riscv_iommu_device *iommu = dev_get_drvdata(dev);
> +	u16 idx = desc->msi_index;
> +	u64 addr;
> +
> +	addr = ((u64)msg->address_hi << 32) | msg->address_lo;
> +
> +	if (addr != (addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR))
> +		pr_warn_once("uh oh, the IOMMU can't send MSIs to 0x%llx, sending to 0x%llx instead\n",
> +			     addr, addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR);

Can this use dev_warn_once()? And should it really be only a warning?
Configuring the IOMMU to write to some other address seems dangerous. I guess
there's no clean way to handle this error, since this function cannot fail, and
irq_chip_compose_msi_msg() isn't supposed to fail either.

> +
> +	addr &= RISCV_IOMMU_MSI_CFG_TBL_ADDR;
> +
> +	riscv_iommu_writeq(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_ADDR(idx), addr);
> +	riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_DATA(idx), msg->data);
> +	riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_CTRL(idx), 0);
> +}
> +
>  static int riscv_iommu_platform_probe(struct platform_device *pdev)
>  {
> +	enum riscv_iommu_igs_settings igs;
>  	struct device *dev = &pdev->dev;
>  	struct riscv_iommu_device *iommu = NULL;
>  	struct resource *res = NULL;
> -	int vec;
> +	int vec, ret;
>  
>  	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
>  	if (!iommu)
> @@ -40,16 +63,6 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
>  	iommu->caps = riscv_iommu_readq(iommu, RISCV_IOMMU_REG_CAPABILITIES);
>  	iommu->fctl = riscv_iommu_readl(iommu, RISCV_IOMMU_REG_FCTL);
>  
> -	/* For now we only support WSI */
> -	switch (FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps)) {
> -	case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
> -	case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
> -		break;
> -	default:
> -		return dev_err_probe(dev, -ENODEV,
> -				     "unable to use wire-signaled interrupts\n");
> -	}
> -
>  	iommu->irqs_count = platform_irq_count(pdev);
>  	if (iommu->irqs_count <= 0)
>  		return dev_err_probe(dev, -ENODEV,
> @@ -57,13 +70,60 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
>  	if (iommu->irqs_count > RISCV_IOMMU_INTR_COUNT)
>  		iommu->irqs_count = RISCV_IOMMU_INTR_COUNT;
>  
> -	for (vec = 0; vec < iommu->irqs_count; vec++)
> -		iommu->irqs[vec] = platform_get_irq(pdev, vec);
> +	igs = FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps);
> +	switch (igs) {
> +	case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
> +	case RISCV_IOMMU_CAPABILITIES_IGS_MSI:
> +		if (is_of_node(dev->fwnode))
> +			of_msi_configure(dev, to_of_node(dev->fwnode));
> +
> +		if (!dev_get_msi_domain(dev)) {
> +			dev_warn(dev, "failed to find an MSI domain");
> +			goto msi_fail;
> +		}
> +
> +		ret = platform_device_msi_init_and_alloc_irqs(dev, iommu->irqs_count,
> +							      riscv_iommu_write_msi_msg);
> +		if (ret) {
> +			dev_warn(dev, "failed to allocate MSIs");
> +			goto msi_fail;
> +		}
> +
> +		for (vec = 0; vec < iommu->irqs_count; vec++)
> +			iommu->irqs[vec] = msi_get_virq(dev, vec);
> +
> +		/* Enable message-signaled interrupts, fctl.WSI */
> +		if (iommu->fctl & RISCV_IOMMU_FCTL_WSI) {
> +			iommu->fctl ^= RISCV_IOMMU_FCTL_WSI;
> +			riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> +		}
> +
> +		dev_info(dev, "using MSIs\n");
> +		break;
> +
> +msi_fail:
> +		if (igs != RISCV_IOMMU_CAPABILITIES_IGS_BOTH) {
> +			dev_warn(dev, "\n");
> +			return dev_err_probe(dev, -ENODEV,
> +					     "unable to use wire-signaled interrupts\n");

Is the dev_warn() just to call attention to the following error? There's no
guarantee that these two messages will be printed adjacently.

> +		}
> +
> +		dev_warn(dev, " - falling back to wired irqs\n");

No need for the extra hyphen here.

Regards,
Samuel

> +		fallthrough;
>  
> -	/* Enable wire-signaled interrupts, fctl.WSI */
> -	if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
> -		iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
> -		riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> +	case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
> +		for (vec = 0; vec < iommu->irqs_count; vec++)
> +			iommu->irqs[vec] = platform_get_irq(pdev, vec);
> +
> +		/* Enable wire-signaled interrupts, fctl.WSI */
> +		if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
> +			iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
> +			riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> +		}
> +		dev_info(dev, "using wire-signaled interrupts\n");
> +		break;
> +	default:
> +		return dev_err_probe(dev, -ENODEV, "invalid IGS\n");
>  	}
>  
>  	return riscv_iommu_init(iommu);
> @@ -71,7 +131,13 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
>  
>  static void riscv_iommu_platform_remove(struct platform_device *pdev)
>  {
> -	riscv_iommu_remove(dev_get_drvdata(&pdev->dev));
> +	struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev);
> +	bool msi = !(iommu->fctl & RISCV_IOMMU_FCTL_WSI);
> +
> +	riscv_iommu_remove(iommu);
> +
> +	if (msi)
> +		platform_device_msi_free_irqs_all(&pdev->dev);
>  };
>  
>  static const struct of_device_id riscv_iommu_of_match[] = {


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 2/2] iommu/riscv: Add support for platform msi

[Andrew Jones]

On Wed, Nov 06, 2024 at 01:45:23PM -0600, Samuel Holland wrote:
> Hi Drew,
> 
> On 2024-11-06 11:51 AM, Andrew Jones wrote:
> > Apply platform_device_msi_init_and_alloc_irqs() to add support for
> > MSIs when the IOMMU is a platform device.
> > 
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  drivers/iommu/riscv/iommu-platform.c | 102 ++++++++++++++++++++++-----
> >  1 file changed, 84 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/iommu/riscv/iommu-platform.c b/drivers/iommu/riscv/iommu-platform.c
> > index da336863f152..89aa622bcbde 100644
> > --- a/drivers/iommu/riscv/iommu-platform.c
> > +++ b/drivers/iommu/riscv/iommu-platform.c
> > @@ -11,18 +11,41 @@
> >   */
> >  
> >  #include <linux/kernel.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/msi.h>
> 
> If you respin, please keep these sorted.

Sure

> 
> >  
> >  #include "iommu-bits.h"
> >  #include "iommu.h"
> >  
> > +static void riscv_iommu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > +{
> > +	struct device *dev = msi_desc_to_dev(desc);
> > +	struct riscv_iommu_device *iommu = dev_get_drvdata(dev);
> > +	u16 idx = desc->msi_index;
> > +	u64 addr;
> > +
> > +	addr = ((u64)msg->address_hi << 32) | msg->address_lo;
> > +
> > +	if (addr != (addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR))
> > +		pr_warn_once("uh oh, the IOMMU can't send MSIs to 0x%llx, sending to 0x%llx instead\n",
> > +			     addr, addr & RISCV_IOMMU_MSI_CFG_TBL_ADDR);
> 
> Can this use dev_warn_once()? And should it really be only a warning?
> Configuring the IOMMU to write to some other address seems dangerous. I guess
> there's no clean way to handle this error, since this function cannot fail, and
> irq_chip_compose_msi_msg() isn't supposed to fail either.

I'll change it to a dev_err_once().

> 
> > +
> > +	addr &= RISCV_IOMMU_MSI_CFG_TBL_ADDR;
> > +
> > +	riscv_iommu_writeq(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_ADDR(idx), addr);
> > +	riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_DATA(idx), msg->data);
> > +	riscv_iommu_writel(iommu, RISCV_IOMMU_REG_MSI_CFG_TBL_CTRL(idx), 0);
> > +}
> > +
> >  static int riscv_iommu_platform_probe(struct platform_device *pdev)
> >  {
> > +	enum riscv_iommu_igs_settings igs;
> >  	struct device *dev = &pdev->dev;
> >  	struct riscv_iommu_device *iommu = NULL;
> >  	struct resource *res = NULL;
> > -	int vec;
> > +	int vec, ret;
> >  
> >  	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
> >  	if (!iommu)
> > @@ -40,16 +63,6 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
> >  	iommu->caps = riscv_iommu_readq(iommu, RISCV_IOMMU_REG_CAPABILITIES);
> >  	iommu->fctl = riscv_iommu_readl(iommu, RISCV_IOMMU_REG_FCTL);
> >  
> > -	/* For now we only support WSI */
> > -	switch (FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps)) {
> > -	case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
> > -	case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
> > -		break;
> > -	default:
> > -		return dev_err_probe(dev, -ENODEV,
> > -				     "unable to use wire-signaled interrupts\n");
> > -	}
> > -
> >  	iommu->irqs_count = platform_irq_count(pdev);
> >  	if (iommu->irqs_count <= 0)
> >  		return dev_err_probe(dev, -ENODEV,
> > @@ -57,13 +70,60 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
> >  	if (iommu->irqs_count > RISCV_IOMMU_INTR_COUNT)
> >  		iommu->irqs_count = RISCV_IOMMU_INTR_COUNT;
> >  
> > -	for (vec = 0; vec < iommu->irqs_count; vec++)
> > -		iommu->irqs[vec] = platform_get_irq(pdev, vec);
> > +	igs = FIELD_GET(RISCV_IOMMU_CAPABILITIES_IGS, iommu->caps);
> > +	switch (igs) {
> > +	case RISCV_IOMMU_CAPABILITIES_IGS_BOTH:
> > +	case RISCV_IOMMU_CAPABILITIES_IGS_MSI:
> > +		if (is_of_node(dev->fwnode))
> > +			of_msi_configure(dev, to_of_node(dev->fwnode));
> > +
> > +		if (!dev_get_msi_domain(dev)) {
> > +			dev_warn(dev, "failed to find an MSI domain");
> > +			goto msi_fail;
> > +		}
> > +
> > +		ret = platform_device_msi_init_and_alloc_irqs(dev, iommu->irqs_count,
> > +							      riscv_iommu_write_msi_msg);
> > +		if (ret) {
> > +			dev_warn(dev, "failed to allocate MSIs");
> > +			goto msi_fail;
> > +		}
> > +
> > +		for (vec = 0; vec < iommu->irqs_count; vec++)
> > +			iommu->irqs[vec] = msi_get_virq(dev, vec);
> > +
> > +		/* Enable message-signaled interrupts, fctl.WSI */
> > +		if (iommu->fctl & RISCV_IOMMU_FCTL_WSI) {
> > +			iommu->fctl ^= RISCV_IOMMU_FCTL_WSI;
> > +			riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> > +		}
> > +
> > +		dev_info(dev, "using MSIs\n");
> > +		break;
> > +
> > +msi_fail:
> > +		if (igs != RISCV_IOMMU_CAPABILITIES_IGS_BOTH) {
> > +			dev_warn(dev, "\n");
> > +			return dev_err_probe(dev, -ENODEV,
> > +					     "unable to use wire-signaled interrupts\n");
> 
> Is the dev_warn() just to call attention to the following error? There's no
> guarantee that these two messages will be printed adjacently.

I'll drop the dev_warn since it was supposed to tie in with the dev_warn
above which was missing its endline on purpose, but your statement about
avoiding split logs means I should probably just do a complete log above
and complete logs here and...

> 
> > +		}
> > +
> > +		dev_warn(dev, " - falling back to wired irqs\n");
> 
> No need for the extra hyphen here.

...here, which also means the hyphen should be dropped.
> 
> Regards,
> Samuel

Thanks,
drew

> 
> > +		fallthrough;
> >  
> > -	/* Enable wire-signaled interrupts, fctl.WSI */
> > -	if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
> > -		iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
> > -		riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> > +	case RISCV_IOMMU_CAPABILITIES_IGS_WSI:
> > +		for (vec = 0; vec < iommu->irqs_count; vec++)
> > +			iommu->irqs[vec] = platform_get_irq(pdev, vec);
> > +
> > +		/* Enable wire-signaled interrupts, fctl.WSI */
> > +		if (!(iommu->fctl & RISCV_IOMMU_FCTL_WSI)) {
> > +			iommu->fctl |= RISCV_IOMMU_FCTL_WSI;
> > +			riscv_iommu_writel(iommu, RISCV_IOMMU_REG_FCTL, iommu->fctl);
> > +		}
> > +		dev_info(dev, "using wire-signaled interrupts\n");
> > +		break;
> > +	default:
> > +		return dev_err_probe(dev, -ENODEV, "invalid IGS\n");
> >  	}
> >  
> >  	return riscv_iommu_init(iommu);
> > @@ -71,7 +131,13 @@ static int riscv_iommu_platform_probe(struct platform_device *pdev)
> >  
> >  static void riscv_iommu_platform_remove(struct platform_device *pdev)
> >  {
> > -	riscv_iommu_remove(dev_get_drvdata(&pdev->dev));
> > +	struct riscv_iommu_device *iommu = dev_get_drvdata(&pdev->dev);
> > +	bool msi = !(iommu->fctl & RISCV_IOMMU_FCTL_WSI);
> > +
> > +	riscv_iommu_remove(iommu);
> > +
> > +	if (msi)
> > +		platform_device_msi_free_irqs_all(&pdev->dev);
> >  };
> >  
> >  static const struct of_device_id riscv_iommu_of_match[] = {
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv