| Message ID | 20170327082936.6830-6-hch@lst.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Christoph Hellwig > 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
On Mon, Mar 27, 2017 at 02:06:45PM +0000, David Laight wrote: > 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.
From: 'Christoph Hellwig' > Sent: 27 March 2017 15:52 > On Mon, Mar 27, 2017 at 02:06:45PM +0000, David Laight wrote: > > 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. > 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
On 03/27/2017 01:29 AM, Christoph Hellwig wrote: > 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 > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/pci/msi.c | 21 --------------------- > include/linux/pci.h | 4 ---- > 2 files changed, 25 deletions(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index d571bc330686..0042c365b29b 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -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; > diff --git a/include/linux/pci.h b/include/linux/pci.h > index eb3da1a04e6c..82dec36845e6 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -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) { } >
On Mon, Mar 27, 2017 at 03:03:30PM +0000, David Laight wrote: > 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.
On Mon, Mar 27, 2017 at 09:59:35AM -0700, David Daney wrote: > On 03/27/2017 01:29 AM, Christoph Hellwig wrote: >> 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. > 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.
On 03/27/2017 10:11 AM, Christoph Hellwig wrote: > On Mon, Mar 27, 2017 at 09:59:35AM -0700, David Daney wrote: >> On 03/27/2017 01:29 AM, Christoph Hellwig wrote: >>> 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. > >> 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
On Mon, Mar 27, 2017 at 10:30:46AM -0700, David Daney wrote: >> 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.
From: David Daney > Sent: 27 March 2017 18:31 > On 03/27/2017 10:11 AM, Christoph Hellwig wrote: > > On Mon, Mar 27, 2017 at 09:59:35AM -0700, David Daney wrote: > >> On 03/27/2017 01:29 AM, Christoph Hellwig wrote: > >>> 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. > > > >> 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? 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
On 03/27/2017 11:41 PM, Christoph Hellwig wrote: > On Mon, Mar 27, 2017 at 10:30:46AM -0700, David Daney wrote: >>> 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.
On Tue, Mar 28, 2017 at 09:24:15AM -0700, David Daney wrote: > On 03/27/2017 11:41 PM, Christoph Hellwig wrote: > >On Mon, Mar 27, 2017 at 10:30:46AM -0700, David Daney wrote: > >>>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. 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
On 03/30/2017 03:56 PM, Bjorn Helgaas wrote: > On Tue, Mar 28, 2017 at 09:24:15AM -0700, David Daney wrote: >> On 03/27/2017 11:41 PM, Christoph Hellwig wrote: >>> On Mon, Mar 27, 2017 at 10:30:46AM -0700, David Daney wrote: >>>>> 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. > > 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 > 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 >
On Mon, Mar 27, 2017 at 10:29:36AM +0200, Christoph Hellwig wrote: > Unused now that all callers switched to pci_alloc_irq_vectors. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Acked-by: Bjorn Helgaas <bhelgaas@google.com> I assume this will be merged with the rest via the netdev tree. > --- > drivers/pci/msi.c | 21 --------------------- > include/linux/pci.h | 4 ---- > 2 files changed, 25 deletions(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index d571bc330686..0042c365b29b 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -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; > diff --git a/include/linux/pci.h b/include/linux/pci.h > index eb3da1a04e6c..82dec36845e6 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -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) { } > -- > 2.11.0 >
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index d571bc330686..0042c365b29b 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -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; diff --git a/include/linux/pci.h b/include/linux/pci.h index eb3da1a04e6c..82dec36845e6 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -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) { }
Unused now that all callers switched to pci_alloc_irq_vectors. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/pci/msi.c | 21 --------------------- include/linux/pci.h | 4 ---- 2 files changed, 25 deletions(-)