public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device
@ 2024-11-20  7:04 Shijith Thotton
  2024-11-20  7:04 ` [PATCH 2/4] vdpa/octeon_ep: handle device config change events Shijith Thotton
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Shijith Thotton @ 2024-11-20  7:04 UTC (permalink / raw)
  To: virtualization, mst, jasowang
  Cc: Shijith Thotton, schalla, vattunuru, ndabilpuram, jerinj,
	Xuan Zhuo, Eugenio Pérez, Dan Carpenter, Satha Rao,
	open list

Updated the driver to utilize all the MSI-X interrupt vectors supported
by each OCTEON endpoint VF, instead of relying on a single vector.
Enabling more interrupts allows packets from multiple rings to be
distributed across multiple cores, improving parallelism and
performance.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 drivers/vdpa/octeon_ep/octep_vdpa.h      | 10 +--
 drivers/vdpa/octeon_ep/octep_vdpa_hw.c   |  2 -
 drivers/vdpa/octeon_ep/octep_vdpa_main.c | 88 ++++++++++++++++--------
 3 files changed, 63 insertions(+), 37 deletions(-)

diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h b/drivers/vdpa/octeon_ep/octep_vdpa.h
index 046710ec4d42..2d4bb07f91b3 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa.h
+++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
@@ -29,12 +29,12 @@
 #define OCTEP_EPF_RINFO(x) (0x000209f0 | ((x) << 25))
 #define OCTEP_VF_MBOX_DATA(x) (0x00010210 | ((x) << 17))
 #define OCTEP_PF_MBOX_DATA(x) (0x00022000 | ((x) << 4))
-
-#define OCTEP_EPF_RINFO_RPVF(val) (((val) >> 32) & 0xF)
-#define OCTEP_EPF_RINFO_NVFS(val) (((val) >> 48) & 0x7F)
+#define OCTEP_VF_IN_CTRL(x)        (0x00010000 | ((x) << 17))
+#define OCTEP_VF_IN_CTRL_RPVF(val) (((val) >> 48) & 0xF)
 
 #define OCTEP_FW_READY_SIGNATURE0  0xFEEDFEED
 #define OCTEP_FW_READY_SIGNATURE1  0x3355ffaa
+#define OCTEP_MAX_CB_INTR          8
 
 enum octep_vdpa_dev_status {
 	OCTEP_VDPA_DEV_STATUS_INVALID,
@@ -50,7 +50,6 @@ struct octep_vring_info {
 	void __iomem *notify_addr;
 	u32 __iomem *cb_notify_addr;
 	phys_addr_t notify_pa;
-	char msix_name[256];
 };
 
 struct octep_hw {
@@ -68,7 +67,8 @@ struct octep_hw {
 	u64 features;
 	u16 nr_vring;
 	u32 config_size;
-	int irq;
+	int nb_irqs;
+	int *irqs;
 };
 
 u8 octep_hw_get_status(struct octep_hw *oct_hw);
diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_hw.c b/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
index 1d4767b33315..d5a599f87e18 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
@@ -495,8 +495,6 @@ int octep_hw_caps_read(struct octep_hw *oct_hw, struct pci_dev *pdev)
 	if (!oct_hw->vqs)
 		return -ENOMEM;
 
-	oct_hw->irq = -1;
-
 	dev_info(&pdev->dev, "Device features : %llx\n", oct_hw->features);
 	dev_info(&pdev->dev, "Maximum queues : %u\n", oct_hw->nr_vring);
 
diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
index cd55b1aac151..482c178a5221 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
@@ -47,13 +47,30 @@ static struct octep_hw *vdpa_to_octep_hw(struct vdpa_device *vdpa_dev)
 static irqreturn_t octep_vdpa_intr_handler(int irq, void *data)
 {
 	struct octep_hw *oct_hw = data;
-	int i;
+	int i, ring_start, ring_stride;
+
+	/* Each device has multiple interrupts (nb_irqs) shared among receive
+	 * rings (nr_vring). Device interrupts are mapped to specific receive
+	 * rings in a round-robin fashion. Only rings handling receive
+	 * operations require interrupts, and these are at even indices.
+	 *
+	 * For example, if nb_irqs = 8 and nr_vring = 64:
+	 * 0 -> 0, 16, 32, 48;
+	 * 1 -> 2, 18, 34, 50;
+	 * ...
+	 * 7 -> 14, 30, 46, 62;
+	 */
+	ring_start = (irq - oct_hw->irqs[0]) * 2;
+	ring_stride = oct_hw->nb_irqs * 2;
 
-	for (i = 0; i < oct_hw->nr_vring; i++) {
-		if (oct_hw->vqs[i].cb.callback && ioread32(oct_hw->vqs[i].cb_notify_addr)) {
-			/* Acknowledge the per queue notification to the device */
+	for (i = ring_start; i < oct_hw->nr_vring; i += ring_stride) {
+		if (ioread32(oct_hw->vqs[i].cb_notify_addr)) {
+			/* Acknowledge the per ring notification to the device */
 			iowrite32(0, oct_hw->vqs[i].cb_notify_addr);
-			oct_hw->vqs[i].cb.callback(oct_hw->vqs[i].cb.private);
+
+			if (likely(oct_hw->vqs[i].cb.callback))
+				oct_hw->vqs[i].cb.callback(oct_hw->vqs[i].cb.private);
+			break;
 		}
 	}
 
@@ -63,44 +80,53 @@ static irqreturn_t octep_vdpa_intr_handler(int irq, void *data)
 static void octep_free_irqs(struct octep_hw *oct_hw)
 {
 	struct pci_dev *pdev = oct_hw->pdev;
+	int irq;
+
+	for (irq = 0; irq < oct_hw->nb_irqs && oct_hw->irqs; irq++) {
+		if (oct_hw->irqs[irq] < 0)
+			continue;
 
-	if (oct_hw->irq != -1) {
-		devm_free_irq(&pdev->dev, oct_hw->irq, oct_hw);
-		oct_hw->irq = -1;
+		devm_free_irq(&pdev->dev, oct_hw->irqs[irq], oct_hw);
 	}
+
 	pci_free_irq_vectors(pdev);
+	kfree(oct_hw->irqs);
 }
 
 static int octep_request_irqs(struct octep_hw *oct_hw)
 {
 	struct pci_dev *pdev = oct_hw->pdev;
-	int ret, irq;
+	int ret, irq, idx;
 
-	/* Currently HW device provisions one IRQ per VF, hence
-	 * allocate one IRQ for all virtqueues call interface.
-	 */
-	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
+	ret = pci_alloc_irq_vectors(pdev, 1, oct_hw->nb_irqs, PCI_IRQ_MSIX);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to alloc msix vector");
 		return ret;
 	}
 
-	snprintf(oct_hw->vqs->msix_name, sizeof(oct_hw->vqs->msix_name),
-		 OCTEP_VDPA_DRIVER_NAME "-vf-%d", pci_iov_vf_id(pdev));
+	oct_hw->irqs = kcalloc(oct_hw->nb_irqs, sizeof(int), GFP_KERNEL);
+	if (!oct_hw->irqs) {
+		ret = -ENOMEM;
+		goto free_irqs;
+	}
 
-	irq = pci_irq_vector(pdev, 0);
-	ret = devm_request_irq(&pdev->dev, irq, octep_vdpa_intr_handler, 0,
-			       oct_hw->vqs->msix_name, oct_hw);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to register interrupt handler\n");
-		goto free_irq_vec;
+	memset(oct_hw->irqs, -1, sizeof(oct_hw->irqs));
+
+	for (idx = 0; idx < oct_hw->nb_irqs; idx++) {
+		irq = pci_irq_vector(pdev, idx);
+		ret = devm_request_irq(&pdev->dev, irq, octep_vdpa_intr_handler, 0,
+				       dev_name(&pdev->dev), oct_hw);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to register interrupt handler\n");
+			goto free_irqs;
+		}
+		oct_hw->irqs[idx] = irq;
 	}
-	oct_hw->irq = irq;
 
 	return 0;
 
-free_irq_vec:
-	pci_free_irq_vectors(pdev);
+free_irqs:
+	octep_free_irqs(oct_hw);
 	return ret;
 }
 
@@ -559,6 +585,7 @@ static void octep_vdpa_setup_task(struct work_struct *work)
 	struct device *dev = &pdev->dev;
 	struct octep_hw *oct_hw;
 	unsigned long timeout;
+	u64 val;
 	int ret;
 
 	oct_hw = &mgmt_dev->oct_hw;
@@ -590,6 +617,13 @@ static void octep_vdpa_setup_task(struct work_struct *work)
 	if (ret)
 		return;
 
+	val = readq(oct_hw->base[OCTEP_HW_MBOX_BAR] + OCTEP_VF_IN_CTRL(0));
+	oct_hw->nb_irqs = OCTEP_VF_IN_CTRL_RPVF(val);
+	if (!oct_hw->nb_irqs || oct_hw->nb_irqs > OCTEP_MAX_CB_INTR) {
+		dev_err(dev, "Invalid number of interrupts %d\n", oct_hw->nb_irqs);
+		goto unmap_region;
+	}
+
 	ret = octep_hw_caps_read(oct_hw, pdev);
 	if (ret < 0)
 		goto unmap_region;
@@ -768,12 +802,6 @@ static int octep_vdpa_pf_setup(struct octep_pf *octpf)
 		return -EINVAL;
 	}
 
-	if (OCTEP_EPF_RINFO_RPVF(val) != BIT_ULL(0)) {
-		val &= ~GENMASK_ULL(35, 32);
-		val |= BIT_ULL(32);
-		writeq(val, addr + OCTEP_EPF_RINFO(0));
-	}
-
 	len = pci_resource_len(pdev, OCTEP_HW_CAPS_BAR);
 
 	octpf->vf_stride = len / totalvfs;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/4] vdpa/octeon_ep: handle device config change events
  2024-11-20  7:04 [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device Shijith Thotton
@ 2024-11-20  7:04 ` Shijith Thotton
  2024-11-20  7:26   ` Dan Carpenter
  2024-11-20  7:04 ` [PATCH 3/4] vdpa/octeon_ep: read vendor-specific PCI capability Shijith Thotton
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Shijith Thotton @ 2024-11-20  7:04 UTC (permalink / raw)
  To: virtualization, mst, jasowang
  Cc: Satha Rao, schalla, vattunuru, ndabilpuram, jerinj,
	Shijith Thotton, Xuan Zhuo, Eugenio Pérez, open list

From: Satha Rao <skoteshwar@marvell.com>

The first interrupt of the device is used to notify the host about
device configuration changes, such as link status updates. The ISR
configuration area is updated to indicate a config change event when
triggered.

Signed-off-by: Satha Rao <skoteshwar@marvell.com>
Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 drivers/vdpa/octeon_ep/octep_vdpa_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
index 482c178a5221..6db951c6cb92 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
@@ -74,6 +74,14 @@ static irqreturn_t octep_vdpa_intr_handler(int irq, void *data)
 		}
 	}
 
+	/* Check for config interrupt. Config uses the first interrupt */
+	if (unlikely(ring_start == 0 && ioread8(oct_hw->isr))) {
+		iowrite8(0, oct_hw->isr);
+
+		if (likely(oct_hw->config_cb.callback))
+			oct_hw->config_cb.callback(oct_hw->config_cb.private);
+	}
+
 	return IRQ_HANDLED;
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 3/4] vdpa/octeon_ep: read vendor-specific PCI capability
  2024-11-20  7:04 [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device Shijith Thotton
  2024-11-20  7:04 ` [PATCH 2/4] vdpa/octeon_ep: handle device config change events Shijith Thotton
@ 2024-11-20  7:04 ` Shijith Thotton
  2024-11-20  7:04 ` [PATCH 4/4] vdpa/octeon_ep: add interrupt handler for virtio crypto device Shijith Thotton
  2024-11-20  8:05 ` [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device Dan Carpenter
  3 siblings, 0 replies; 6+ messages in thread
From: Shijith Thotton @ 2024-11-20  7:04 UTC (permalink / raw)
  To: virtualization, mst, jasowang
  Cc: Shijith Thotton, schalla, vattunuru, ndabilpuram, jerinj,
	Xuan Zhuo, Eugenio Pérez, Dan Carpenter, Satha Rao,
	open list

Added support to read the vendor-specific PCI capability to identify the
type of device being emulated.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 drivers/vdpa/octeon_ep/octep_vdpa.h      | 24 +++++++++++++++++
 drivers/vdpa/octeon_ep/octep_vdpa_hw.c   | 34 +++++++++++++++++++++++-
 drivers/vdpa/octeon_ep/octep_vdpa_main.c |  4 ++-
 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/vdpa/octeon_ep/octep_vdpa.h b/drivers/vdpa/octeon_ep/octep_vdpa.h
index 2d4bb07f91b3..0f83a1eca408 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa.h
+++ b/drivers/vdpa/octeon_ep/octep_vdpa.h
@@ -8,6 +8,7 @@
 #include <linux/pci_regs.h>
 #include <linux/vdpa.h>
 #include <linux/virtio_pci_modern.h>
+#include <uapi/linux/virtio_crypto.h>
 #include <uapi/linux/virtio_net.h>
 #include <uapi/linux/virtio_blk.h>
 #include <uapi/linux/virtio_config.h>
@@ -52,6 +53,28 @@ struct octep_vring_info {
 	phys_addr_t notify_pa;
 };
 
+enum octep_pci_vndr_cfg_type {
+	OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID,
+	OCTEP_PCI_VNDR_CFG_TYPE_MAX,
+};
+
+struct octep_pci_vndr_data {
+	u8 cap_vndr;
+	u8 cap_next;
+	u8 cap_len;
+	u8 cfg_type;
+	u16 vendor_id;
+	u8 id;
+	u8 bar;
+	union {
+		u64 data;
+		struct {
+			u32 offset;
+			u32 length;
+		};
+	};
+};
+
 struct octep_hw {
 	struct pci_dev *pdev;
 	u8 __iomem *base[PCI_STD_NUM_BARS];
@@ -69,6 +92,7 @@ struct octep_hw {
 	u32 config_size;
 	int nb_irqs;
 	int *irqs;
+	u8 dev_id;
 };
 
 u8 octep_hw_get_status(struct octep_hw *oct_hw);
diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_hw.c b/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
index d5a599f87e18..2f4849e90525 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_hw.c
@@ -358,7 +358,14 @@ u16 octep_get_vq_size(struct octep_hw *oct_hw)
 
 static u32 octep_get_config_size(struct octep_hw *oct_hw)
 {
-	return sizeof(struct virtio_net_config);
+	switch (oct_hw->dev_id) {
+	case VIRTIO_ID_NET:
+		return sizeof(struct virtio_net_config);
+	case VIRTIO_ID_CRYPTO:
+		return sizeof(struct virtio_crypto_config);
+	default:
+		return 0;
+	}
 }
 
 static void __iomem *octep_get_cap_addr(struct octep_hw *oct_hw, struct virtio_pci_cap *cap)
@@ -416,8 +423,24 @@ static int octep_pci_signature_verify(struct octep_hw *oct_hw)
 	return 0;
 }
 
+static void octep_vndr_data_process(struct octep_hw *oct_hw,
+				    struct octep_pci_vndr_data *vndr_data)
+{
+	switch (vndr_data->id) {
+	case OCTEP_PCI_VNDR_CFG_TYPE_VIRTIO_ID:
+		oct_hw->dev_id = (u8)vndr_data->data;
+		break;
+	default:
+		dev_err(&oct_hw->pdev->dev, "Invalid vendor data id %u\n",
+			vndr_data->id);
+		break;
+	}
+}
+
+#define VIRTIO_PCI_CAP_VENDOR_CFG	9
 int octep_hw_caps_read(struct octep_hw *oct_hw, struct pci_dev *pdev)
 {
+	struct octep_pci_vndr_data vndr_data;
 	struct octep_mbox __iomem *mbox;
 	struct device *dev = &pdev->dev;
 	struct virtio_pci_cap cap;
@@ -466,6 +489,15 @@ int octep_hw_caps_read(struct octep_hw *oct_hw, struct pci_dev *pdev)
 		case VIRTIO_PCI_CAP_ISR_CFG:
 			oct_hw->isr = octep_get_cap_addr(oct_hw, &cap);
 			break;
+		case VIRTIO_PCI_CAP_VENDOR_CFG:
+			octep_pci_caps_read(oct_hw, &vndr_data, sizeof(vndr_data), pos);
+			if (vndr_data.vendor_id != PCI_VENDOR_ID_CAVIUM) {
+				dev_err(dev, "Invalid vendor data\n");
+				return -EINVAL;
+			}
+
+			octep_vndr_data_process(oct_hw, &vndr_data);
+			break;
 		}
 
 		pos = cap.cap_next;
diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
index 6db951c6cb92..6ead42d76415 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
@@ -305,7 +305,9 @@ static u32 octep_vdpa_get_generation(struct vdpa_device *vdpa_dev)
 
 static u32 octep_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
 {
-	return VIRTIO_ID_NET;
+	struct octep_hw *oct_hw = vdpa_to_octep_hw(vdpa_dev);
+
+	return oct_hw->dev_id;
 }
 
 static u32 octep_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 4/4] vdpa/octeon_ep: add interrupt handler for virtio crypto device
  2024-11-20  7:04 [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device Shijith Thotton
  2024-11-20  7:04 ` [PATCH 2/4] vdpa/octeon_ep: handle device config change events Shijith Thotton
  2024-11-20  7:04 ` [PATCH 3/4] vdpa/octeon_ep: read vendor-specific PCI capability Shijith Thotton
@ 2024-11-20  7:04 ` Shijith Thotton
  2024-11-20  8:05 ` [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device Dan Carpenter
  3 siblings, 0 replies; 6+ messages in thread
From: Shijith Thotton @ 2024-11-20  7:04 UTC (permalink / raw)
  To: virtualization, mst, jasowang
  Cc: Shijith Thotton, schalla, vattunuru, ndabilpuram, jerinj,
	Xuan Zhuo, Eugenio Pérez, Satha Rao, open list

Introduced an interrupt handler for the virtio crypto device, as its
queue usage differs from that of network devices. While virtio network
device receives packets only on even-indexed queues, virtio crypto
device utilize all available queues for processing data.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 drivers/vdpa/octeon_ep/octep_vdpa_main.c | 51 +++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
index 6ead42d76415..1b65cb697fa5 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
@@ -44,7 +44,35 @@ static struct octep_hw *vdpa_to_octep_hw(struct vdpa_device *vdpa_dev)
 	return oct_vdpa->oct_hw;
 }
 
-static irqreturn_t octep_vdpa_intr_handler(int irq, void *data)
+static irqreturn_t octep_vdpa_crypto_irq_handler(int irq, void *data)
+{
+	struct octep_hw *oct_hw = data;
+	int i;
+
+	/* Each device interrupt (nb_irqs) maps to specific receive rings
+	 * (nr_vring) in a round-robin fashion.
+	 *
+	 * For example, if nb_irqs = 8 and nr_vring = 64:
+	 * 0 -> 0, 8, 16, 24, 32, 40, 48, 56;
+	 * 1 -> 1, 9, 17, 25, 33, 41, 49, 57;
+	 * ...
+	 * 7 -> 7, 15, 23, 31, 39, 47, 55, 63;
+	 */
+	for (i = irq - oct_hw->irqs[0]; i < oct_hw->nr_vring; i += oct_hw->nb_irqs) {
+		if (ioread32(oct_hw->vqs[i].cb_notify_addr)) {
+			/* Acknowledge the per ring notification to the device */
+			iowrite32(0, oct_hw->vqs[i].cb_notify_addr);
+
+			if (likely(oct_hw->vqs[i].cb.callback))
+				oct_hw->vqs[i].cb.callback(oct_hw->vqs[i].cb.private);
+			break;
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t octep_vdpa_net_irq_handler(int irq, void *data)
 {
 	struct octep_hw *oct_hw = data;
 	int i, ring_start, ring_stride;
@@ -85,6 +113,18 @@ static irqreturn_t octep_vdpa_intr_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t (*octep_vdpa_get_irq_handler(u32 dev_id))(int, void *)
+{
+	switch (dev_id) {
+	case VIRTIO_ID_NET:
+		return octep_vdpa_net_irq_handler;
+	case VIRTIO_ID_CRYPTO:
+		return octep_vdpa_crypto_irq_handler;
+	default:
+		return NULL;
+	}
+}
+
 static void octep_free_irqs(struct octep_hw *oct_hw)
 {
 	struct pci_dev *pdev = oct_hw->pdev;
@@ -103,6 +143,7 @@ static void octep_free_irqs(struct octep_hw *oct_hw)
 
 static int octep_request_irqs(struct octep_hw *oct_hw)
 {
+	irqreturn_t (*irq_handler)(int irq, void *data);
 	struct pci_dev *pdev = oct_hw->pdev;
 	int ret, irq, idx;
 
@@ -119,10 +160,16 @@ static int octep_request_irqs(struct octep_hw *oct_hw)
 	}
 
 	memset(oct_hw->irqs, -1, sizeof(oct_hw->irqs));
+	irq_handler = octep_vdpa_get_irq_handler(oct_hw->dev_id);
+	if (!irq_handler) {
+		dev_err(&pdev->dev, "Invalid device id %d\n", oct_hw->dev_id);
+		ret = -EINVAL;
+		goto free_irqs;
+	}
 
 	for (idx = 0; idx < oct_hw->nb_irqs; idx++) {
 		irq = pci_irq_vector(pdev, idx);
-		ret = devm_request_irq(&pdev->dev, irq, octep_vdpa_intr_handler, 0,
+		ret = devm_request_irq(&pdev->dev, irq, irq_handler, 0,
 				       dev_name(&pdev->dev), oct_hw);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to register interrupt handler\n");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/4] vdpa/octeon_ep: handle device config change events
  2024-11-20  7:04 ` [PATCH 2/4] vdpa/octeon_ep: handle device config change events Shijith Thotton
@ 2024-11-20  7:26   ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2024-11-20  7:26 UTC (permalink / raw)
  To: Shijith Thotton
  Cc: virtualization, mst, jasowang, Satha Rao, schalla, vattunuru,
	ndabilpuram, jerinj, Xuan Zhuo, Eugenio Pérez, open list

On Wed, Nov 20, 2024 at 12:34:51PM +0530, Shijith Thotton wrote:
> From: Satha Rao <skoteshwar@marvell.com>
> 
> The first interrupt of the device is used to notify the host about
> device configuration changes, such as link status updates. The ISR
> configuration area is updated to indicate a config change event when
> triggered.
> 
> Signed-off-by: Satha Rao <skoteshwar@marvell.com>
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---
>  drivers/vdpa/octeon_ep/octep_vdpa_main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> index 482c178a5221..6db951c6cb92 100644
> --- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> +++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
> @@ -74,6 +74,14 @@ static irqreturn_t octep_vdpa_intr_handler(int irq, void *data)
>  		}
>  	}
>  
> +	/* Check for config interrupt. Config uses the first interrupt */
> +	if (unlikely(ring_start == 0 && ioread8(oct_hw->isr))) {
> +		iowrite8(0, oct_hw->isr);
> +
> +		if (likely(oct_hw->config_cb.callback))

Adding unlikely() and likey() hurts readability.  It adds noise.  You should do
it if it makes a difference on a benchmark but that seems unlikely here.

regards,
dan carpenter

> +			oct_hw->config_cb.callback(oct_hw->config_cb.private);
> +	}
> +
>  	return IRQ_HANDLED;
>  }
>  
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device
  2024-11-20  7:04 [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device Shijith Thotton
                   ` (2 preceding siblings ...)
  2024-11-20  7:04 ` [PATCH 4/4] vdpa/octeon_ep: add interrupt handler for virtio crypto device Shijith Thotton
@ 2024-11-20  8:05 ` Dan Carpenter
  3 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2024-11-20  8:05 UTC (permalink / raw)
  To: Shijith Thotton
  Cc: virtualization, mst, jasowang, schalla, vattunuru, ndabilpuram,
	jerinj, Xuan Zhuo, Eugenio Pérez, Satha Rao, open list

On Wed, Nov 20, 2024 at 12:34:50PM +0530, Shijith Thotton wrote:
> @@ -63,44 +80,53 @@ static irqreturn_t octep_vdpa_intr_handler(int irq, void *data)
>  static void octep_free_irqs(struct octep_hw *oct_hw)
>  {
>  	struct pci_dev *pdev = oct_hw->pdev;
> +	int irq;
> +
> +	for (irq = 0; irq < oct_hw->nb_irqs && oct_hw->irqs; irq++) {
> +		if (oct_hw->irqs[irq] < 0)
> +			continue;
>  
> -	if (oct_hw->irq != -1) {
> -		devm_free_irq(&pdev->dev, oct_hw->irq, oct_hw);
> -		oct_hw->irq = -1;
> +		devm_free_irq(&pdev->dev, oct_hw->irqs[irq], oct_hw);
>  	}
> +
>  	pci_free_irq_vectors(pdev);
> +	kfree(oct_hw->irqs);

You should add:

	oct_hw->nb_irqs = 0;
	oct_hw->irqs = NULL;

Otherwise if reset is called twice in a row, before re-initializing the IRQs it
results in a use after free.

>  }
>  
>  static int octep_request_irqs(struct octep_hw *oct_hw)
>  {
>  	struct pci_dev *pdev = oct_hw->pdev;
> -	int ret, irq;
> +	int ret, irq, idx;
>  
> -	/* Currently HW device provisions one IRQ per VF, hence
> -	 * allocate one IRQ for all virtqueues call interface.
> -	 */
> -	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
> +	ret = pci_alloc_irq_vectors(pdev, 1, oct_hw->nb_irqs, PCI_IRQ_MSIX);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to alloc msix vector");
>  		return ret;
>  	}
>  
> -	snprintf(oct_hw->vqs->msix_name, sizeof(oct_hw->vqs->msix_name),
> -		 OCTEP_VDPA_DRIVER_NAME "-vf-%d", pci_iov_vf_id(pdev));
> +	oct_hw->irqs = kcalloc(oct_hw->nb_irqs, sizeof(int), GFP_KERNEL);

This isn't free on the ->release() path or whatever.  octep_free_irqs() is
called on reset() but we rely on devm_ to free the IRQs on ->release().  Use
devm_kcalloc() here as well, probably.

> +	if (!oct_hw->irqs) {
> +		ret = -ENOMEM;
> +		goto free_irqs;
> +	}
>  
> -	irq = pci_irq_vector(pdev, 0);
> -	ret = devm_request_irq(&pdev->dev, irq, octep_vdpa_intr_handler, 0,
> -			       oct_hw->vqs->msix_name, oct_hw);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to register interrupt handler\n");
> -		goto free_irq_vec;
> +	memset(oct_hw->irqs, -1, sizeof(oct_hw->irqs));

This works, but it would be more normal to just leave it zeroed and check for
zero instead of checking for negatives.  There is never a zero IRQ.  See my blog
for more details:
https://staticthinking.wordpress.com/2023/08/07/writing-a-check-for-zero-irq-error-codes/

regards,
dan carpenter

> +
> +	for (idx = 0; idx < oct_hw->nb_irqs; idx++) {
> +		irq = pci_irq_vector(pdev, idx);
> +		ret = devm_request_irq(&pdev->dev, irq, octep_vdpa_intr_handler, 0,
> +				       dev_name(&pdev->dev), oct_hw);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to register interrupt handler\n");
> +			goto free_irqs;
> +		}
> +		oct_hw->irqs[idx] = irq;
>  	}
> -	oct_hw->irq = irq;
>  
>  	return 0;
>  
> -free_irq_vec:
> -	pci_free_irq_vectors(pdev);
> +free_irqs:
> +	octep_free_irqs(oct_hw);
>  	return ret;
>  }
>  

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-11-20  8:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-20  7:04 [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device Shijith Thotton
2024-11-20  7:04 ` [PATCH 2/4] vdpa/octeon_ep: handle device config change events Shijith Thotton
2024-11-20  7:26   ` Dan Carpenter
2024-11-20  7:04 ` [PATCH 3/4] vdpa/octeon_ep: read vendor-specific PCI capability Shijith Thotton
2024-11-20  7:04 ` [PATCH 4/4] vdpa/octeon_ep: add interrupt handler for virtio crypto device Shijith Thotton
2024-11-20  8:05 ` [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device Dan Carpenter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox