Skip to contentSkip to conversation

remove pci_enable_msix()

PATCH In review Christoph Hellwig 5 participants 21 messages Mar 27, 2017 – Mar 30, 2017

Subsystems (6)
Networking Network drivers PCI Atlx Ethernet drivers +2 more subsystems
lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

Hi all,

this series removes the remaining callers of the pci_enable_msix() function and then the function itself. All remaining users in in network drivers, so if Bjorn is fine with it I'd like to handle it entirely through the netdev tree.

author
#1 lore raw

PATCH 1/5 net: thunderx: switch to pci_alloc_irq_vectors

Remove the deprecated pci_enable_msix API in favour of it's successor.

Signed-off-by Christoph Hellwig <[email protected]>
---
 drivers/net/ethernet/cavium/thunder/nic_main.c | 75 ++++++--------------------
 1 file changed, 15 insertions(+), 60 deletions(-)
drivers/net/ethernet/cavium/thunder/nic_main.c +15 -60
index 767234e2e8f9..205569db9093 100644
@@ -65,10 +65,7 @@ struct nicpf {
 	bool			mbx_lock[MAX_NUM_VFS_SUPPORTED];
 
 	/* MSI-X */
-	bool			msix_enabled;
 	u8			num_vec;
-	struct msix_entry	*msix_entries;
-	bool			irq_allocated[NIC_PF_MSIX_VECTORS];
 	char			irq_name[NIC_PF_MSIX_VECTORS][20];
 };
 

@@ -1088,7 +1085,7 @@ static irqreturn_t nic_mbx_intr_handler(int irq, void *nic_irq)
 	u64 intr;
 	u8  vf, vf_per_mbx_reg = 64;
 
-	if (irq == nic->msix_entries[NIC_PF_INTR_ID_MBOX0].vector)
+	if (irq == pci_irq_vector(nic->pdev, NIC_PF_INTR_ID_MBOX0))
 		mbx = 0;
 	else
 		mbx = 1;

@@ -1107,76 +1104,30 @@ static irqreturn_t nic_mbx_intr_handler(int irq, void *nic_irq)
 	return IRQ_HANDLED;
 }
 
-static int nic_enable_msix(struct nicpf *nic)
+static int nic_register_interrupts(struct nicpf *nic)
 {
 	int i, ret;
 
-	nic->num_vec = pci_msix_vec_count(nic->pdev);
-
-	nic->msix_entries = kmalloc_array(nic->num_vec,
-					  sizeof(struct msix_entry),
-					  GFP_KERNEL);
-	if (!nic->msix_entries)
-		return -ENOMEM;
-
-	for (i = 0; i < nic->num_vec; i++)
-		nic->msix_entries[i].entry = i;
-
-	ret = pci_enable_msix(nic->pdev, nic->msix_entries, nic->num_vec);
-	if (ret) {
+	/* Enable MSI-X */
+	ret = pci_alloc_irq_vectors(nic->pdev, nic->num_vec, nic->num_vec,
+			PCI_IRQ_MSIX);
+	if (ret < 0) {
 		dev_err(&nic->pdev->dev,
 			"Request for #%d msix vectors failed, returned %d\n",
 			   nic->num_vec, ret);
-		kfree(nic->msix_entries);
 		return ret;
 	}
 
-	nic->msix_enabled = 1;
-	return 0;
-}
-
-static void nic_disable_msix(struct nicpf *nic)
-{
-	if (nic->msix_enabled) {
-		pci_disable_msix(nic->pdev);
-		kfree(nic->msix_entries);
-		nic->msix_enabled = 0;
-		nic->num_vec = 0;
-	}
-}
-
-static void nic_free_all_interrupts(struct nicpf *nic)
-{
-	int irq;
-
-	for (irq = 0; irq < nic->num_vec; irq++) {
-		if (nic->irq_allocated[irq])
-			free_irq(nic->msix_entries[irq].vector, nic);
-		nic->irq_allocated[irq] = false;
-	}
-}
-
-static int nic_register_interrupts(struct nicpf *nic)
-{
-	int i, ret;
-
-	/* Enable MSI-X */
-	ret = nic_enable_msix(nic);
-	if (ret)
-		return ret;
-
 	/* Register mailbox interrupt handler */
 	for (i = NIC_PF_INTR_ID_MBOX0; i < nic->num_vec; i++) {
 		sprintf(nic->irq_name[i],
 			"NICPF Mbox%d", (i - NIC_PF_INTR_ID_MBOX0));
 
-		ret = request_irq(nic->msix_entries[i].vector,
+		ret = request_irq(pci_irq_vector(nic->pdev, i),
 				  nic_mbx_intr_handler, 0,
 				  nic->irq_name[i], nic);
 		if (ret)
 			goto fail;
-
-		nic->irq_allocated[i] = true;
 	}
 
 	/* Enable mailbox interrupt */

@@ -1185,15 +1136,19 @@ static int nic_register_interrupts(struct nicpf *nic)
 
 fail:
 	dev_err(&nic->pdev->dev, "Request irq failed\n");
-	nic_free_all_interrupts(nic);
-	nic_disable_msix(nic);
+	while (--i >= 0)
+		free_irq(pci_irq_vector(nic->pdev, i), nic);
+	pci_free_irq_vectors(nic->pdev);
 	return ret;
 }
 
 static void nic_unregister_interrupts(struct nicpf *nic)
 {
-	nic_free_all_interrupts(nic);
-	nic_disable_msix(nic);
+	int i;
+
+	for (i = 0; i < nic->num_vec; i++)
+		free_irq(pci_irq_vector(nic->pdev, i), nic);
+	pci_free_irq_vectors(nic->pdev);
 }
 
 static int nic_num_sqs_en(struct nicpf *nic, int vf_en)
Signature · 1 line
2.11.0
#2 lore raw

Re: [PATCH 1/5] net: thunderx: switch to pci_alloc_irq_vectors

drivers/net/ethernet/cavium/thunder/nic_main.c static irqreturn_t nic_mbx_intr_handler(int irq, void *nic_irq) · line 1113

Quoted 13 lines · Christoph Hellwig

Remove the deprecated pci_enable_msix API in favour of it's successor.

Signed-off-by Christoph Hellwig <[email protected]>
---
 drivers/net/ethernet/cavium/thunder/nic_main.c | 75 ++++++--------------------
 1 file changed, 15 insertions(+), 60 deletions(-)
drivers/net/ethernet/cavium/thunder/nic_main.c +3 -4
-       nic->num_vec = pci_msix_vec_count(nic->pdev);
-
-       ret = pci_enable_msix(nic->pdev, nic->msix_entries, nic->num_vec);
-       if (ret) {
+       /* Enable MSI-X */
+       ret = pci_alloc_irq_vectors(nic->pdev, nic->num_vec, nic->num_vec,
+                       PCI_IRQ_MSIX);

nic->num_vec will always be zero here.

drivers/net/ethernet/cavium/thunder/nic_main.c static int nic_register_interrupts(struct nicpf *nic) · line 1142

Quoted 4 lines
drivers/net/ethernet/cavium/thunder/nic_main.c +3 -0
+       while (--i >= 0)
+               free_irq(pci_irq_vector(nic->pdev, i), nic);
+       pci_free_irq_vectors(nic->pdev);
        return ret;

IRQ handlers are not registered for all vectors to free them unconditionally

drivers/net/ethernet/cavium/thunder/nic_main.c static int nic_register_interrupts(struct nicpf *nic) · line 1152

Quoted 6 lines
drivers/net/ethernet/cavium/thunder/nic_main.c +5 -0
+       int i;
+
+       for (i = 0; i < nic->num_vec; i++)
+               free_irq(pci_irq_vector(nic->pdev, i), nic);
+       pci_free_irq_vectors(nic->pdev);
 }

Same as here.

How urgent is this ?
Can we comeup with a proper patch to fix this in couple of weeks, instead of going with these untested patches ?

author
#3 lore raw

Re: [PATCH 1/5] net: thunderx: switch to pci_alloc_irq_vectors

Quoted 1 line · Sunil Kovvuri

How urgent is this ?

I would like to get this into 4.12 to stop people from adding new users of pci_enable_msix.

Quoted 2 lines

Can we comeup with a proper patch to fix this in couple of weeks, instead of going with these untested patches ?

Sure.

author
#4 lore raw

PATCH 2/5 net: thunderxvf: switch to pci_alloc_irq_vectors

In reply to Christoph Hellwig

Remove the deprecated pci_enable_msix API in favour of it's successor.

Signed-off-by Christoph Hellwig <[email protected]>
---
 drivers/net/ethernet/cavium/thunder/nic.h        |  2 -
 drivers/net/ethernet/cavium/thunder/nicvf_main.c | 70 ++++++++----------------
 2 files changed, 24 insertions(+), 48 deletions(-)
drivers/net/ethernet/cavium/thunder/nic.h +0 -2
index 2269ff562d95..6fb44218bf55 100644
@@ -319,9 +319,7 @@ struct nicvf {
 	struct bgx_stats	bgx_stats;
 
 	/* MSI-X  */
-	bool			msix_enabled;
 	u8			num_vec;
-	struct msix_entry	msix_entries[NIC_VF_MSIX_VECTORS];
 	char			irq_name[NIC_VF_MSIX_VECTORS][IFNAMSIZ + 15];
 	bool			irq_allocated[NIC_VF_MSIX_VECTORS];
 	cpumask_var_t		affinity_mask[NIC_VF_MSIX_VECTORS];
drivers/net/ethernet/cavium/thunder/nicvf_main.c +24 -46

155 lines not on this page · raw message

Signature · 1 line
2.11.0
author
#5 lore raw

PATCH 3/5 net: alx: switch to pci_alloc_irq_vectors

In reply to Christoph Hellwig

Remove the deprecated pci_enable_msix API in favour of it's successor, and make sure to handle errors during IRQ setup properly.

Signed-off-by Christoph Hellwig <[email protected]>
---
 drivers/net/ethernet/atheros/alx/alx.h  |   6 --
 drivers/net/ethernet/atheros/alx/main.c | 128 ++++++++++++++------------------
 2 files changed, 54 insertions(+), 80 deletions(-)
drivers/net/ethernet/atheros/alx/alx.h +0 -6
index d4a409139ea2..78c5de467426 100644
@@ -102,9 +102,6 @@ struct alx_napi {
 
 #define ALX_MAX_NAPIS 8
 
-#define ALX_FLAG_USING_MSIX	BIT(0)
-#define ALX_FLAG_USING_MSI	BIT(1)
-
 struct alx_priv {
 	struct net_device *dev;
 

@@ -112,7 +109,6 @@ struct alx_priv {
 
 	/* msi-x vectors */
 	int num_vec;
-	struct msix_entry *msix_entries;
 
 	/* all descriptor memory */
 	struct {

@@ -139,8 +135,6 @@ struct alx_priv {
 
 	u16 msg_enable;
 
-	int flags;
-
 	/* protects hw.stats */
 	spinlock_t stats_lock;
 };
drivers/net/ethernet/atheros/alx/main.c +54 -74

273 lines not on this page · raw message

Signature · 1 line
2.11.0
author
#6 lore raw

PATCH 4/5 net/ena: switch to pci_alloc_irq_vectors

In reply to Christoph Hellwig

Remove the deprecated pci_enable_msix API in favour of it's successor.

Signed-off-by Christoph Hellwig <[email protected]>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 55 ++++++----------------------
 drivers/net/ethernet/amazon/ena/ena_netdev.h |  2 -
 2 files changed, 12 insertions(+), 45 deletions(-)
drivers/net/ethernet/amazon/ena/ena_netdev.c +12 -43
index 35f19430c84a..7c1214d78855 100644
@@ -133,7 +133,7 @@ static int ena_init_rx_cpu_rmap(struct ena_adapter *adapter)
 		int irq_idx = ENA_IO_IRQ_IDX(i);
 
 		rc = irq_cpu_rmap_add(adapter->netdev->rx_cpu_rmap,
-				      adapter->msix_entries[irq_idx].vector);
+				      pci_irq_vector(adapter->pdev, irq_idx));
 		if (rc) {
 			free_irq_cpu_rmap(adapter->netdev->rx_cpu_rmap);
 			adapter->netdev->rx_cpu_rmap = NULL;

@@ -1208,13 +1208,7 @@ static irqreturn_t ena_intr_msix_io(int irq, void *data)
 
 static int ena_enable_msix(struct ena_adapter *adapter, int num_queues)
 {
-	int i, msix_vecs, rc;
-
-	if (test_bit(ENA_FLAG_MSIX_ENABLED, &adapter->flags)) {
-		netif_err(adapter, probe, adapter->netdev,
-			  "Error, MSI-X is already enabled\n");
-		return -EPERM;
-	}
+	int msix_vecs, rc;
 
 	/* Reserved the max msix vectors we might need */
 	msix_vecs = ENA_MAX_MSIX_VEC(num_queues);

@@ -1222,16 +1216,9 @@ static int ena_enable_msix(struct ena_adapter *adapter, int num_queues)
 	netif_dbg(adapter, probe, adapter->netdev,
 		  "trying to enable MSI-X, vectors %d\n", msix_vecs);
 
-	adapter->msix_entries = vzalloc(msix_vecs * sizeof(struct msix_entry));
-
-	if (!adapter->msix_entries)
-		return -ENOMEM;
-
-	for (i = 0; i < msix_vecs; i++)
-		adapter->msix_entries[i].entry = i;
-
-	rc = pci_enable_msix(adapter->pdev, adapter->msix_entries, msix_vecs);
-	if (rc != 0) {
+	rc = pci_alloc_irq_vectors(adapter->pdev, msix_vecs, msix_vecs,
+			PCI_IRQ_MSIX);
+	if (rc < 0) {
 		netif_err(adapter, probe, adapter->netdev,
 			  "Failed to enable MSI-X, vectors %d rc %d\n",
 			  msix_vecs, rc);

@@ -1248,7 +1235,6 @@ static int ena_enable_msix(struct ena_adapter *adapter, int num_queues)
 	}
 
 	adapter->msix_vecs = msix_vecs;
-	set_bit(ENA_FLAG_MSIX_ENABLED, &adapter->flags);
 
 	return 0;
 }

@@ -1264,7 +1250,7 @@ static void ena_setup_mgmnt_intr(struct ena_adapter *adapter)
 		ena_intr_msix_mgmnt;
 	adapter->irq_tbl[ENA_MGMNT_IRQ_IDX].data = adapter;
 	adapter->irq_tbl[ENA_MGMNT_IRQ_IDX].vector =
-		adapter->msix_entries[ENA_MGMNT_IRQ_IDX].vector;
+		pci_irq_vector(adapter->pdev, ENA_MGMNT_IRQ_IDX);
 	cpu = cpumask_first(cpu_online_mask);
 	adapter->irq_tbl[ENA_MGMNT_IRQ_IDX].cpu = cpu;
 	cpumask_set_cpu(cpu,

@@ -1287,7 +1273,7 @@ static void ena_setup_io_intr(struct ena_adapter *adapter)
 		adapter->irq_tbl[irq_idx].handler = ena_intr_msix_io;
 		adapter->irq_tbl[irq_idx].data = &adapter->ena_napi[i];
 		adapter->irq_tbl[irq_idx].vector =
-			adapter->msix_entries[irq_idx].vector;
+			pci_irq_vector(adapter->pdev, irq_idx);
 		adapter->irq_tbl[irq_idx].cpu = cpu;
 
 		cpumask_set_cpu(cpu,

@@ -1325,12 +1311,6 @@ static int ena_request_io_irq(struct ena_adapter *adapter)
 	struct ena_irq *irq;
 	int rc = 0, i, k;
 
-	if (!test_bit(ENA_FLAG_MSIX_ENABLED, &adapter->flags)) {
-		netif_err(adapter, ifup, adapter->netdev,
-			  "Failed to request I/O IRQ: MSI-X is not enabled\n");
-		return -EINVAL;
-	}
-
 	for (i = ENA_IO_IRQ_FIRST_IDX; i < adapter->msix_vecs; i++) {
 		irq = &adapter->irq_tbl[i];
 		rc = request_irq(irq->vector, irq->handler, flags, irq->name,

@@ -1389,16 +1369,6 @@ static void ena_free_io_irq(struct ena_adapter *adapter)
 	}
 }
 
-static void ena_disable_msix(struct ena_adapter *adapter)
-{
-	if (test_and_clear_bit(ENA_FLAG_MSIX_ENABLED, &adapter->flags))
-		pci_disable_msix(adapter->pdev);
-
-	if (adapter->msix_entries)
-		vfree(adapter->msix_entries);
-	adapter->msix_entries = NULL;
-}
-
 static void ena_disable_io_intr_sync(struct ena_adapter *adapter)
 {
 	int i;

@@ -2479,8 +2449,7 @@ static int ena_enable_msix_and_set_admin_interrupts(struct ena_adapter *adapter,
 	return 0;
 
 err_disable_msix:
-	ena_disable_msix(adapter);
-
+	pci_free_irq_vectors(adapter->pdev);
 	return rc;
 }
 

@@ -2518,7 +2487,7 @@ static void ena_fw_reset_device(struct work_struct *work)
 
 	ena_free_mgmnt_irq(adapter);
 
-	ena_disable_msix(adapter);
+	pci_free_irq_vectors(adapter->pdev);
 
 	ena_com_abort_admin_commands(ena_dev);
 

@@ -2569,7 +2538,7 @@ static void ena_fw_reset_device(struct work_struct *work)
 	return;
 err_disable_msix:
 	ena_free_mgmnt_irq(adapter);
-	ena_disable_msix(adapter);
+	pci_free_irq_vectors(adapter->pdev);
 err_device_destroy:
 	ena_com_admin_destroy(ena_dev);
 err:

@@ -3103,7 +3072,7 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 err_free_msix:
 	ena_com_dev_reset(ena_dev);
 	ena_free_mgmnt_irq(adapter);
-	ena_disable_msix(adapter);
+	pci_free_irq_vectors(adapter->pdev);
 err_worker_destroy:
 	ena_com_destroy_interrupt_moderation(ena_dev);
 	del_timer(&adapter->timer_service);

@@ -3188,7 +3157,7 @@ static void ena_remove(struct pci_dev *pdev)
 
 	ena_free_mgmnt_irq(adapter);
 
-	ena_disable_msix(adapter);
+	pci_free_irq_vectors(adapter->pdev);
 
 	free_netdev(netdev);
drivers/net/ethernet/amazon/ena/ena_netdev.h +0 -2
index ed62d8e231a1..0e22bce6239d 100644
@@ -248,7 +248,6 @@ enum ena_flags_t {
 	ENA_FLAG_DEVICE_RUNNING,
 	ENA_FLAG_DEV_UP,
 	ENA_FLAG_LINK_UP,
-	ENA_FLAG_MSIX_ENABLED,
 	ENA_FLAG_TRIGGER_RESET
 };
 

@@ -267,7 +266,6 @@ struct ena_adapter {
 
 	int num_queues;
 
-	struct msix_entry *msix_entries;
 	int msix_vecs;
 
 	u32 tx_usecs, rx_usecs; /* interrupt moderation */
Signature · 1 line
2.11.0
author
#7 lore raw

PATCH 5/5 PCI: remove pci_enable_msix

In reply to Christoph Hellwig

Unused now that all callers switched to pci_alloc_irq_vectors.

Signed-off-by Christoph Hellwig <[email protected]>
---
 drivers/pci/msi.c   | 21 ---------------------
 include/linux/pci.h |  4 ----
 2 files changed, 25 deletions(-)
drivers/pci/msi.c +0 -21
index d571bc330686..0042c365b29b 100644
@@ -973,27 +973,6 @@ static int __pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
 	return msix_capability_init(dev, entries, nvec, affd);
 }
 
-/**
- * pci_enable_msix - configure device's MSI-X capability structure
- * @dev: pointer to the pci_dev data structure of MSI-X device function
- * @entries: pointer to an array of MSI-X entries (optional)
- * @nvec: number of MSI-X irqs requested for allocation by device driver
- *
- * Setup the MSI-X capability structure of device function with the number
- * of requested irqs upon its software driver call to request for
- * MSI-X mode enabled on its hardware device function. A return of zero
- * indicates the successful configuration of MSI-X capability structure
- * with new allocated MSI-X irqs. A return of < 0 indicates a failure.
- * Or a return of > 0 indicates that driver request is exceeding the number
- * of irqs or MSI-X vectors available. Driver should use the returned value to
- * re-send its request.
- **/
-int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
-{
-	return __pci_enable_msix(dev, entries, nvec, NULL);
-}
-EXPORT_SYMBOL(pci_enable_msix);
-
 void pci_msix_shutdown(struct pci_dev *dev)
 {
 	struct msi_desc *entry;
include/linux/pci.h +0 -4
index eb3da1a04e6c..82dec36845e6 100644
@@ -1300,7 +1300,6 @@ int pci_msi_vec_count(struct pci_dev *dev);
 void pci_msi_shutdown(struct pci_dev *dev);
 void pci_disable_msi(struct pci_dev *dev);
 int pci_msix_vec_count(struct pci_dev *dev);
-int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec);
 void pci_msix_shutdown(struct pci_dev *dev);
 void pci_disable_msix(struct pci_dev *dev);
 void pci_restore_msi_state(struct pci_dev *dev);

@@ -1330,9 +1329,6 @@ static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; }
 static inline void pci_msi_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msi(struct pci_dev *dev) { }
 static inline int pci_msix_vec_count(struct pci_dev *dev) { return -ENOSYS; }
-static inline int pci_enable_msix(struct pci_dev *dev,
-				  struct msix_entry *entries, int nvec)
-{ return -ENOSYS; }
 static inline void pci_msix_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msix(struct pci_dev *dev) { }
 static inline void pci_restore_msi_state(struct pci_dev *dev) { }
Signature · 1 line
2.11.0
#8 lore raw

RE: [PATCH 5/5] PCI: remove pci_enable_msix

From: Christoph Hellwig

Quoted 3 lines

Sent: 27 March 2017 09:30

Unused now that all callers switched to pci_alloc_irq_vectors.

Apart from all the 'out of tree' drivers that will need fixing and a conditional compile added.

At least give us a couple of kernel versions to get it sorted.

David

author
#9 lore raw

Re: [PATCH 5/5] PCI: remove pci_enable_msix

Quoted 2 lines · David Laight

Apart from all the 'out of tree' drivers that will need fixing and a conditional compile added.

That has never been a reason for stopping linux kernel changes.

Never mind that your out of tree drivers should probably never have used this function anyway, but pci_enable_msix_{exact,range} which would have been the better choice even before pci_alloc_irq_vectors.

#10 lore raw

RE: [PATCH 5/5] PCI: remove pci_enable_msix

From: 'Christoph Hellwig'

Quoted 6 lines

Sent: 27 March 2017 15:52

Quoted 2 lines · David Laight

Apart from all the 'out of tree' drivers that will need fixing and a conditional compile added.

That has never been a reason for stopping linux kernel changes.

I'm aware of that, but sometimes you just make life hard.

Quoted 3 lines

Never mind that your out of tree drivers should probably never have used this function anyway, but pci_enable_msix_{exact,range} which would have been the better choice even before pci_alloc_irq_vectors.

Indeed, but pci_enable_msix_range() only appeared in 3.14. We have to support a wide range of kernels.
I think we've finally managed to get most of our customers off 2.6.18.

David

author
#11 lore raw

Re: [PATCH 5/5] PCI: remove pci_enable_msix

Quoted 3 lines · David Laight

Indeed, but pci_enable_msix_range() only appeared in 3.14. We have to support a wide range of kernels.
I think we've finally managed to get most of our customers off 2.6.18.

That's your problem, not that of upstream kernel developers.

#12 lore raw

Re: [PATCH 5/5] PCI: remove pci_enable_msix

In reply to Christoph Hellwig
Quoted 2 lines · Christoph Hellwig

Unused now that all callers switched to pci_alloc_irq_vectors.

And you are aware that the ThunderX GPIO driver that I am attempting to merge uses this interface.

If this patch gets merged, should I ask to revert it when the GPIO driver goes in?

You offer no solution for drivers that would benefit from using a sparse sub set of the available MSI-X vectors.

David Daney

Quoted 61 lines
Signed-off-by Christoph Hellwig <[email protected]>
---
 drivers/pci/msi.c   | 21 ---------------------
 include/linux/pci.h |  4 ----
 2 files changed, 25 deletions(-)
drivers/pci/msi.c +0 -21
index d571bc330686..0042c365b29b 100644
@@ -973,27 +973,6 @@ static int __pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
 	return msix_capability_init(dev, entries, nvec, affd);
 }
 
-/**
- * pci_enable_msix - configure device's MSI-X capability structure
- * @dev: pointer to the pci_dev data structure of MSI-X device function
- * @entries: pointer to an array of MSI-X entries (optional)
- * @nvec: number of MSI-X irqs requested for allocation by device driver
- *
- * Setup the MSI-X capability structure of device function with the number
- * of requested irqs upon its software driver call to request for
- * MSI-X mode enabled on its hardware device function. A return of zero
- * indicates the successful configuration of MSI-X capability structure
- * with new allocated MSI-X irqs. A return of < 0 indicates a failure.
- * Or a return of > 0 indicates that driver request is exceeding the number
- * of irqs or MSI-X vectors available. Driver should use the returned value to
- * re-send its request.
- **/
-int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
-{
-	return __pci_enable_msix(dev, entries, nvec, NULL);
-}
-EXPORT_SYMBOL(pci_enable_msix);
-
 void pci_msix_shutdown(struct pci_dev *dev)
 {
 	struct msi_desc *entry;
include/linux/pci.h +0 -4
index eb3da1a04e6c..82dec36845e6 100644
@@ -1300,7 +1300,6 @@ int pci_msi_vec_count(struct pci_dev *dev);
 void pci_msi_shutdown(struct pci_dev *dev);
 void pci_disable_msi(struct pci_dev *dev);
 int pci_msix_vec_count(struct pci_dev *dev);
-int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec);
 void pci_msix_shutdown(struct pci_dev *dev);
 void pci_disable_msix(struct pci_dev *dev);
 void pci_restore_msi_state(struct pci_dev *dev);

@@ -1330,9 +1329,6 @@ static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; }
 static inline void pci_msi_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msi(struct pci_dev *dev) { }
 static inline int pci_msix_vec_count(struct pci_dev *dev) { return -ENOSYS; }
-static inline int pci_enable_msix(struct pci_dev *dev,
-				  struct msix_entry *entries, int nvec)
-{ return -ENOSYS; }
 static inline void pci_msix_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msix(struct pci_dev *dev) { }
 static inline void pci_restore_msi_state(struct pci_dev *dev) { }
author
#13 lore raw

Re: [PATCH 5/5] PCI: remove pci_enable_msix

Quoted 9 lines · David Daney
Quoted 2 lines · Christoph Hellwig

Unused now that all callers switched to pci_alloc_irq_vectors.

And you are aware that the ThunderX GPIO driver that I am attempting to merge uses this interface.

If this patch gets merged, should I ask to revert it when the GPIO driver goes in?

No. You should not use pci_enable_msix in your new driver as I told you before.

Quoted 2 lines

You offer no solution for drivers that would benefit from using a sparse sub set of the available MSI-X vectors.

Use pci_enable_msix_{exact,range} for now, as I told you before.

#14 lore raw

Re: [PATCH 5/5] PCI: remove pci_enable_msix

Quoted 11 lines · Christoph Hellwig
Quoted 9 lines · David Daney
Quoted 2 lines · Christoph Hellwig

Unused now that all callers switched to pci_alloc_irq_vectors.

And you are aware that the ThunderX GPIO driver that I am attempting to merge uses this interface.

If this patch gets merged, should I ask to revert it when the GPIO driver goes in?

Quoted 8 lines

No. You should not use pci_enable_msix in your new driver as I told you before.

Quoted 2 lines

You offer no solution for drivers that would benefit from using a sparse sub set of the available MSI-X vectors.

Use pci_enable_msix_{exact,range} for now, as I told you before.

That still results in twice as many MSI-X being provisioned than are needed.

For drivers that use a contiguous range of MSI-X, your suggestion is usable, but for others you are forcing resources to be wasted. For what end?

David Daney

author
#15 lore raw

Re: [PATCH 5/5] PCI: remove pci_enable_msix

Quoted 4 lines · David Daney
Quoted 2 lines

Use pci_enable_msix_{exact,range} for now, as I told you before.

That still results in twice as many MSI-X being provisioned than are needed.

How so? Except for the return value, a pci_enable_msix_exact call with the same arguments as your previous pci_enable_msix will work exactly the same.

#16 lore raw

Re: [PATCH 5/5] PCI: remove pci_enable_msix

Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · David Daney
Quoted 2 lines

Use pci_enable_msix_{exact,range} for now, as I told you before.

That still results in twice as many MSI-X being provisioned than are needed.

How so? Except for the return value, a pci_enable_msix_exact call with the same arguments as your previous pci_enable_msix will work exactly the same.

Sorry, I think it was my misunderstanding. I didn't realize that we had essentially renamed the function, but left the functionality mostly unchanged.

#17 lore raw

Re: [PATCH 5/5] PCI: remove pci_enable_msix

Quoted 7 lines · David Daney
Quoted 6 lines · Christoph Hellwig
Quoted 4 lines · David Daney
Quoted 2 lines

Use pci_enable_msix_{exact,range} for now, as I told you before.

That still results in twice as many MSI-X being provisioned than are needed.

Quoted 8 lines
Quoted 4 lines

How so? Except for the return value, a pci_enable_msix_exact call with the same arguments as your previous pci_enable_msix will work exactly the same.

Sorry, I think it was my misunderstanding. I didn't realize that we had essentially renamed the function, but left the functionality mostly unchanged.

Does this mean you're OK with this patch? I know it may require some work on out-of-tree drivers and so on, but if that work is possible and you don't actually lose functionality, I'm OK with this patch.

Bjorn

#18 lore raw

Re: [PATCH 5/5] PCI: remove pci_enable_msix

Quoted 13 lines · Bjorn Helgaas
Quoted 12 lines · David Daney
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · David Daney
Quoted 2 lines

Use pci_enable_msix_{exact,range} for now, as I told you before.

That still results in twice as many MSI-X being provisioned than are needed.

How so? Except for the return value, a pci_enable_msix_exact call with the same arguments as your previous pci_enable_msix will work exactly the same.

Quoted 5 lines
Quoted 3 lines

Sorry, I think it was my misunderstanding. I didn't realize that we had essentially renamed the function, but left the functionality mostly unchanged.

Does this mean you're OK with this patch?

Yes. I have re-written my GPIO driver to use the newer functions, so I withdraw my objections to the patch.

Thanks,
David Daney

Quoted 6 lines

I know it may require some
work on out-of-tree drivers and so on, but if that work is possible and you don't actually lose functionality, I'm OK with this patch.

Bjorn

#19 lore raw

RE: [PATCH 5/5] PCI: remove pci_enable_msix

In reply to David Daney

From: David Daney

Quoted 19 lines

Sent: 27 March 2017 18:31

Quoted 17 lines · Christoph Hellwig
Quoted 9 lines · David Daney
Quoted 2 lines · Christoph Hellwig

Unused now that all callers switched to pci_alloc_irq_vectors.

And you are aware that the ThunderX GPIO driver that I am attempting to merge uses this interface.

If this patch gets merged, should I ask to revert it when the GPIO driver goes in?

No. You should not use pci_enable_msix in your new driver as I told you before.

Quoted 2 lines

You offer no solution for drivers that would benefit from using a sparse sub set of the available MSI-X vectors.

Quoted 8 lines
Quoted 2 lines

Use pci_enable_msix_{exact,range} for now, as I told you before.

That still results in twice as many MSI-X being provisioned than are needed.

For drivers that use a contiguous range of MSI-X, your suggestion is usable, but for others you are forcing resources to be wasted. For what end?

There are also drivers that only need some interrupts after a specific device is opened, and could free them when closed.

This could even be network devices with lots of queues, or a driver realising that the workload is high and per-cpu interrupts make sense.

So any real change to the interface should allow drivers to allocate and free individual MSI-X vectors.

I remember a lot of discussions when pci_enable_msix_range() was added, but almost nothing about this change.

David

Acked-by on v1 5/5
#20 lore raw

Re: [PATCH 5/5] PCI: remove pci_enable_msix

In reply to Christoph Hellwig
Quoted 3 lines · Christoph Hellwig

Unused now that all callers switched to pci_alloc_irq_vectors.

Signed-off-by Christoph Hellwig <[email protected]>
Acked-by Bjorn Helgaas <[email protected]>

I assume this will be merged with the rest via the netdev tree.

Quoted 62 lines
---
 drivers/pci/msi.c   | 21 ---------------------
 include/linux/pci.h |  4 ----
 2 files changed, 25 deletions(-)
drivers/pci/msi.c +0 -21
index d571bc330686..0042c365b29b 100644
@@ -973,27 +973,6 @@ static int __pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
 	return msix_capability_init(dev, entries, nvec, affd);
 }
 
-/**
- * pci_enable_msix - configure device's MSI-X capability structure
- * @dev: pointer to the pci_dev data structure of MSI-X device function
- * @entries: pointer to an array of MSI-X entries (optional)
- * @nvec: number of MSI-X irqs requested for allocation by device driver
- *
- * Setup the MSI-X capability structure of device function with the number
- * of requested irqs upon its software driver call to request for
- * MSI-X mode enabled on its hardware device function. A return of zero
- * indicates the successful configuration of MSI-X capability structure
- * with new allocated MSI-X irqs. A return of < 0 indicates a failure.
- * Or a return of > 0 indicates that driver request is exceeding the number
- * of irqs or MSI-X vectors available. Driver should use the returned value to
- * re-send its request.
- **/
-int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
-{
-	return __pci_enable_msix(dev, entries, nvec, NULL);
-}
-EXPORT_SYMBOL(pci_enable_msix);
-
 void pci_msix_shutdown(struct pci_dev *dev)
 {
 	struct msi_desc *entry;
include/linux/pci.h +0 -4
index eb3da1a04e6c..82dec36845e6 100644
@@ -1300,7 +1300,6 @@ int pci_msi_vec_count(struct pci_dev *dev);
 void pci_msi_shutdown(struct pci_dev *dev);
 void pci_disable_msi(struct pci_dev *dev);
 int pci_msix_vec_count(struct pci_dev *dev);
-int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec);
 void pci_msix_shutdown(struct pci_dev *dev);
 void pci_disable_msix(struct pci_dev *dev);
 void pci_restore_msi_state(struct pci_dev *dev);

@@ -1330,9 +1329,6 @@ static inline int pci_msi_vec_count(struct pci_dev *dev) { return -ENOSYS; }
 static inline void pci_msi_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msi(struct pci_dev *dev) { }
 static inline int pci_msix_vec_count(struct pci_dev *dev) { return -ENOSYS; }
-static inline int pci_enable_msix(struct pci_dev *dev,
-				  struct msix_entry *entries, int nvec)
-{ return -ENOSYS; }
 static inline void pci_msix_shutdown(struct pci_dev *dev) { }
 static inline void pci_disable_msix(struct pci_dev *dev) { }
 static inline void pci_restore_msi_state(struct pci_dev *dev) { }
Signature · 1 line
2.11.0