| Message ID | 20241015185124.64726-1-pstanner@redhat.com |
|---|---|
| Headers | show |
| Series | Remove implicit devres from pci_intx() | expand |
On Tue, 15 Oct 2024 20:51:23 +0200 Philipp Stanner <pstanner@redhat.com> wrote: > pci_intx() and its managed counterpart pcim_intx() only exist for older > drivers which have not been ported yet for various reasons. Future > drivers should preferably use pci_alloc_irq_vectors(). > > Mark pci_intx() and pcim_intx() as deprecated and encourage usage of > pci_alloc_irq_vectors() in its place. I don't really understand this. As we've discussed previously pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI IRQ vectors while pci_intx() is for manipulating the INTx disable bit on PCI devices. The latter is a generic mechanism for preventing PCI devices from generating INTx, regardless of whether there's a vector allocated for it. How does the former replace the latter and why do we feel the need to deprecate the latter? It feels like this fits some narrow narrative and makes all users of these now deprecated functions second class citizens. Why? At it's root these are simply providing mask and set or mask and clear register bit operations. Thanks, Alex > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > --- > drivers/pci/devres.c | 5 ++++- > drivers/pci/pci.c | 5 ++++- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c > index 6f8f712fe34e..4c76fc063104 100644 > --- a/drivers/pci/devres.c > +++ b/drivers/pci/devres.c > @@ -435,7 +435,7 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev) > } > > /** > - * pcim_intx - managed pci_intx() > + * pcim_intx - managed pci_intx() (DEPRECATED) > * @pdev: the PCI device to operate on > * @enable: boolean: whether to enable or disable PCI INTx > * > @@ -443,6 +443,9 @@ static struct pcim_intx_devres *get_or_create_intx_devres(struct device *dev) > * > * Enable/disable PCI INTx for device @pdev. > * Restore the original state on driver detach. > + * > + * This function is DEPRECATED. Do not use it in new code. > + * Use pci_alloc_irq_vectors() instead (there is no managed version, currently). > */ > int pcim_intx(struct pci_dev *pdev, int enable) > { > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 7ce1d0e3a1d5..dc69e23b8982 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev *dev) > } > > /** > - * pci_intx - enables/disables PCI INTx for device dev > + * pci_intx - enables/disables PCI INTx for device dev (DEPRECATED) > * @pdev: the PCI device to operate on > * @enable: boolean: whether to enable or disable PCI INTx > * > * Enables/disables PCI INTx for device @pdev > + * > + * This function is DEPRECATED. Do not use it in new code. > + * Use pci_alloc_irq_vectors() instead. > */ > void pci_intx(struct pci_dev *pdev, int enable) > {
On Tue, Oct 15, 2024 at 08:51:23PM +0200, Philipp Stanner wrote: > pci_intx() and its managed counterpart pcim_intx() only exist for older > drivers which have not been ported yet for various reasons. Future > drivers should preferably use pci_alloc_irq_vectors(). > > Mark pci_intx() and pcim_intx() as deprecated and encourage usage of > pci_alloc_irq_vectors() in its place. No one is going to notice these comments, so please, if this really does need to be removed, just remove it from all callers and delete the function from the tree. thanks, greg k-h
On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote: > On Tue, 15 Oct 2024 20:51:23 +0200 > Philipp Stanner <pstanner@redhat.com> wrote: > > > pci_intx() and its managed counterpart pcim_intx() only exist for > > older > > drivers which have not been ported yet for various reasons. Future > > drivers should preferably use pci_alloc_irq_vectors(). > > > > Mark pci_intx() and pcim_intx() as deprecated and encourage usage > > of > > pci_alloc_irq_vectors() in its place. > > I don't really understand this. As we've discussed previously > pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI IRQ > vectors while pci_intx() is for manipulating the INTx disable bit on > PCI devices. The latter is a generic mechanism for preventing PCI > devices from generating INTx, regardless of whether there's a vector > allocated for it. How does the former replace the latter and why do > we > feel the need to deprecate the latter? > > It feels like this fits some narrow narrative and makes all users of > these now deprecated functions second class citizens. Why? At it's > root these are simply providing mask and set or mask and clear > register > bit operations. Thanks, I got the feeling from the RFC discussion that that was basically the consensus: people should use pci_alloc_irq_vectors(). Or did I misunderstand Andy and Heiner? I'm perfectly happy with dropping this patch and continue offering pci{m}_intx() to users, since after removing that hybrid hazzard I don't see any harm in them anymore. P. > > Alex > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > --- > > drivers/pci/devres.c | 5 ++++- > > drivers/pci/pci.c | 5 ++++- > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c > > index 6f8f712fe34e..4c76fc063104 100644 > > --- a/drivers/pci/devres.c > > +++ b/drivers/pci/devres.c > > @@ -435,7 +435,7 @@ static struct pcim_intx_devres > > *get_or_create_intx_devres(struct device *dev) > > } > > > > /** > > - * pcim_intx - managed pci_intx() > > + * pcim_intx - managed pci_intx() (DEPRECATED) > > * @pdev: the PCI device to operate on > > * @enable: boolean: whether to enable or disable PCI INTx > > * > > @@ -443,6 +443,9 @@ static struct pcim_intx_devres > > *get_or_create_intx_devres(struct device *dev) > > * > > * Enable/disable PCI INTx for device @pdev. > > * Restore the original state on driver detach. > > + * > > + * This function is DEPRECATED. Do not use it in new code. > > + * Use pci_alloc_irq_vectors() instead (there is no managed > > version, currently). > > */ > > int pcim_intx(struct pci_dev *pdev, int enable) > > { > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 7ce1d0e3a1d5..dc69e23b8982 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev > > *dev) > > } > > > > /** > > - * pci_intx - enables/disables PCI INTx for device dev > > + * pci_intx - enables/disables PCI INTx for device dev > > (DEPRECATED) > > * @pdev: the PCI device to operate on > > * @enable: boolean: whether to enable or disable PCI INTx > > * > > * Enables/disables PCI INTx for device @pdev > > + * > > + * This function is DEPRECATED. Do not use it in new code. > > + * Use pci_alloc_irq_vectors() instead. > > */ > > void pci_intx(struct pci_dev *pdev, int enable) > > { >
On 16.10.2024 08:57, Philipp Stanner wrote: > On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote: >> On Tue, 15 Oct 2024 20:51:23 +0200 >> Philipp Stanner <pstanner@redhat.com> wrote: >> >>> pci_intx() and its managed counterpart pcim_intx() only exist for >>> older >>> drivers which have not been ported yet for various reasons. Future >>> drivers should preferably use pci_alloc_irq_vectors(). >>> >>> Mark pci_intx() and pcim_intx() as deprecated and encourage usage >>> of >>> pci_alloc_irq_vectors() in its place. >> >> I don't really understand this. As we've discussed previously >> pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI IRQ >> vectors while pci_intx() is for manipulating the INTx disable bit on >> PCI devices. The latter is a generic mechanism for preventing PCI >> devices from generating INTx, regardless of whether there's a vector >> allocated for it. How does the former replace the latter and why do >> we >> feel the need to deprecate the latter? >> >> It feels like this fits some narrow narrative and makes all users of >> these now deprecated functions second class citizens. Why? At it's >> root these are simply providing mask and set or mask and clear >> register >> bit operations. Thanks, > > I got the feeling from the RFC discussion that that was basically the > consensus: people should use pci_alloc_irq_vectors(). Or did I > misunderstand Andy and Heiner? > I think there are two different use cases for pci_intx(). At first there are several drivers where the direct usage of pci_intx() can be eliminated by switching to the pci_alloc_irq_vectors() API. And then there's usage of pci_intx() in drivers/vfio/pci/vfio_pci_intrs.c drivers/xen/xen-pciback/conf_space_header.c There we have to keep the (AFAICS unmanaged) pci_intx() calls. > I'm perfectly happy with dropping this patch and continue offering > pci{m}_intx() to users, since after removing that hybrid hazzard I > don't see any harm in them anymore. > > > P. > >> >> Alex >> >>> Signed-off-by: Philipp Stanner <pstanner@redhat.com> >>> --- >>> drivers/pci/devres.c | 5 ++++- >>> drivers/pci/pci.c | 5 ++++- >>> 2 files changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c >>> index 6f8f712fe34e..4c76fc063104 100644 >>> --- a/drivers/pci/devres.c >>> +++ b/drivers/pci/devres.c >>> @@ -435,7 +435,7 @@ static struct pcim_intx_devres >>> *get_or_create_intx_devres(struct device *dev) >>> } >>> >>> /** >>> - * pcim_intx - managed pci_intx() >>> + * pcim_intx - managed pci_intx() (DEPRECATED) >>> * @pdev: the PCI device to operate on >>> * @enable: boolean: whether to enable or disable PCI INTx >>> * >>> @@ -443,6 +443,9 @@ static struct pcim_intx_devres >>> *get_or_create_intx_devres(struct device *dev) >>> * >>> * Enable/disable PCI INTx for device @pdev. >>> * Restore the original state on driver detach. >>> + * >>> + * This function is DEPRECATED. Do not use it in new code. >>> + * Use pci_alloc_irq_vectors() instead (there is no managed >>> version, currently). >>> */ >>> int pcim_intx(struct pci_dev *pdev, int enable) >>> { >>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>> index 7ce1d0e3a1d5..dc69e23b8982 100644 >>> --- a/drivers/pci/pci.c >>> +++ b/drivers/pci/pci.c >>> @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev >>> *dev) >>> } >>> >>> /** >>> - * pci_intx - enables/disables PCI INTx for device dev >>> + * pci_intx - enables/disables PCI INTx for device dev >>> (DEPRECATED) >>> * @pdev: the PCI device to operate on >>> * @enable: boolean: whether to enable or disable PCI INTx >>> * >>> * Enables/disables PCI INTx for device @pdev >>> + * >>> + * This function is DEPRECATED. Do not use it in new code. >>> + * Use pci_alloc_irq_vectors() instead. >>> */ >>> void pci_intx(struct pci_dev *pdev, int enable) >>> { >> > >
On Wed, 2024-10-16 at 10:43 +0200, Heiner Kallweit wrote: > On 16.10.2024 08:57, Philipp Stanner wrote: > > On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote: > > > On Tue, 15 Oct 2024 20:51:23 +0200 > > > Philipp Stanner <pstanner@redhat.com> wrote: > > > > > > > pci_intx() and its managed counterpart pcim_intx() only exist > > > > for > > > > older > > > > drivers which have not been ported yet for various reasons. > > > > Future > > > > drivers should preferably use pci_alloc_irq_vectors(). > > > > > > > > Mark pci_intx() and pcim_intx() as deprecated and encourage > > > > usage > > > > of > > > > pci_alloc_irq_vectors() in its place. > > > > > > I don't really understand this. As we've discussed previously > > > pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI > > > IRQ > > > vectors while pci_intx() is for manipulating the INTx disable bit > > > on > > > PCI devices. The latter is a generic mechanism for preventing > > > PCI > > > devices from generating INTx, regardless of whether there's a > > > vector > > > allocated for it. How does the former replace the latter and why > > > do > > > we > > > feel the need to deprecate the latter? > > > > > > It feels like this fits some narrow narrative and makes all users > > > of > > > these now deprecated functions second class citizens. Why? At > > > it's > > > root these are simply providing mask and set or mask and clear > > > register > > > bit operations. Thanks, > > > > I got the feeling from the RFC discussion that that was basically > > the > > consensus: people should use pci_alloc_irq_vectors(). Or did I > > misunderstand Andy and Heiner? > > > I think there are two different use cases for pci_intx(). > At first there are several drivers where the direct usage of > pci_intx() > can be eliminated by switching to the pci_alloc_irq_vectors() API. > > And then there's usage of pci_intx() in > drivers/vfio/pci/vfio_pci_intrs.c > drivers/xen/xen-pciback/conf_space_header.c > There we have to keep the (AFAICS unmanaged) pci_intx() calls. There is also the usage within PCI itself, in MSI. Patch №8 touches that. It's why I think this series should land before anyone should port direct pci_intx() users to the irq vectors function, because the latter also uses pci_intx() and its own devres, which sounds explosive to me. P. > > > I'm perfectly happy with dropping this patch and continue offering > > pci{m}_intx() to users, since after removing that hybrid hazzard I > > don't see any harm in them anymore. > > > > > > P. > > > > > > > > Alex > > > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > > > --- > > > > drivers/pci/devres.c | 5 ++++- > > > > drivers/pci/pci.c | 5 ++++- > > > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c > > > > index 6f8f712fe34e..4c76fc063104 100644 > > > > --- a/drivers/pci/devres.c > > > > +++ b/drivers/pci/devres.c > > > > @@ -435,7 +435,7 @@ static struct pcim_intx_devres > > > > *get_or_create_intx_devres(struct device *dev) > > > > } > > > > > > > > /** > > > > - * pcim_intx - managed pci_intx() > > > > + * pcim_intx - managed pci_intx() (DEPRECATED) > > > > * @pdev: the PCI device to operate on > > > > * @enable: boolean: whether to enable or disable PCI INTx > > > > * > > > > @@ -443,6 +443,9 @@ static struct pcim_intx_devres > > > > *get_or_create_intx_devres(struct device *dev) > > > > * > > > > * Enable/disable PCI INTx for device @pdev. > > > > * Restore the original state on driver detach. > > > > + * > > > > + * This function is DEPRECATED. Do not use it in new code. > > > > + * Use pci_alloc_irq_vectors() instead (there is no managed > > > > version, currently). > > > > */ > > > > int pcim_intx(struct pci_dev *pdev, int enable) > > > > { > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > index 7ce1d0e3a1d5..dc69e23b8982 100644 > > > > --- a/drivers/pci/pci.c > > > > +++ b/drivers/pci/pci.c > > > > @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev > > > > *dev) > > > > } > > > > > > > > /** > > > > - * pci_intx - enables/disables PCI INTx for device dev > > > > + * pci_intx - enables/disables PCI INTx for device dev > > > > (DEPRECATED) > > > > * @pdev: the PCI device to operate on > > > > * @enable: boolean: whether to enable or disable PCI INTx > > > > * > > > > * Enables/disables PCI INTx for device @pdev > > > > + * > > > > + * This function is DEPRECATED. Do not use it in new code. > > > > + * Use pci_alloc_irq_vectors() instead. > > > > */ > > > > void pci_intx(struct pci_dev *pdev, int enable) > > > > { > > > > > > > >
On 10/15/24 9:51 PM, Philipp Stanner wrote: > pci_intx() is a hybrid function which can sometimes be managed through > devres. To remove this hybrid nature from pci_intx(), it is necessary to > port users to either an always-managed or a never-managed version. > > All users in ata enable their PCI-Device with pcim_enable_device(). Thus, > they need the always-managed version. > > Replace pci_intx() with pcim_intx(). > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
On Wed, Oct 16, 2024 at 10:53:16AM +0200, Philipp Stanner wrote: > On Wed, 2024-10-16 at 10:43 +0200, Heiner Kallweit wrote: > > On 16.10.2024 08:57, Philipp Stanner wrote: > > > On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote: > > > > On Tue, 15 Oct 2024 20:51:23 +0200 > > > > Philipp Stanner <pstanner@redhat.com> wrote: > > > > > > > > > pci_intx() and its managed counterpart pcim_intx() only exist > > > > > for > > > > > older > > > > > drivers which have not been ported yet for various reasons. > > > > > Future > > > > > drivers should preferably use pci_alloc_irq_vectors(). > > > > > > > > > > Mark pci_intx() and pcim_intx() as deprecated and encourage > > > > > usage > > > > > of > > > > > pci_alloc_irq_vectors() in its place. > > > > > > > > I don't really understand this. As we've discussed previously > > > > pci_alloc_irq_vectors() is, unsurprisingly, for allocating PCI > > > > IRQ > > > > vectors while pci_intx() is for manipulating the INTx disable bit > > > > on > > > > PCI devices. The latter is a generic mechanism for preventing > > > > PCI > > > > devices from generating INTx, regardless of whether there's a > > > > vector > > > > allocated for it. How does the former replace the latter and why > > > > do > > > > we > > > > feel the need to deprecate the latter? > > > > > > > > It feels like this fits some narrow narrative and makes all users > > > > of > > > > these now deprecated functions second class citizens. Why? At > > > > it's > > > > root these are simply providing mask and set or mask and clear > > > > register > > > > bit operations. Thanks, > > > > > > I got the feeling from the RFC discussion that that was basically > > > the > > > consensus: people should use pci_alloc_irq_vectors(). Or did I > > > misunderstand Andy and Heiner? > > > > > I think there are two different use cases for pci_intx(). > > At first there are several drivers where the direct usage of > > pci_intx() > > can be eliminated by switching to the pci_alloc_irq_vectors() API. > > > > And then there's usage of pci_intx() in > > drivers/vfio/pci/vfio_pci_intrs.c > > drivers/xen/xen-pciback/conf_space_header.c > > There we have to keep the (AFAICS unmanaged) pci_intx() calls. > > There is also the usage within PCI itself, in MSI. Patch №8 touches > that. > > It's why I think this series should land before anyone should port > direct pci_intx() users to the irq vectors function, because the latter > also uses pci_intx() and its own devres, which sounds explosive to me. > > > > I'm perfectly happy with dropping this patch and continue offering > > > pci{m}_intx() to users, since after removing that hybrid hazzard I > > > don't see any harm in them anymore. So is the bottom line that we should drop *this* patch and apply the rest of the series? > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > > > > --- > > > > > drivers/pci/devres.c | 5 ++++- > > > > > drivers/pci/pci.c | 5 ++++- > > > > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c > > > > > index 6f8f712fe34e..4c76fc063104 100644 > > > > > --- a/drivers/pci/devres.c > > > > > +++ b/drivers/pci/devres.c > > > > > @@ -435,7 +435,7 @@ static struct pcim_intx_devres > > > > > *get_or_create_intx_devres(struct device *dev) > > > > > } > > > > > > > > > > /** > > > > > - * pcim_intx - managed pci_intx() > > > > > + * pcim_intx - managed pci_intx() (DEPRECATED) > > > > > * @pdev: the PCI device to operate on > > > > > * @enable: boolean: whether to enable or disable PCI INTx > > > > > * > > > > > @@ -443,6 +443,9 @@ static struct pcim_intx_devres > > > > > *get_or_create_intx_devres(struct device *dev) > > > > > * > > > > > * Enable/disable PCI INTx for device @pdev. > > > > > * Restore the original state on driver detach. > > > > > + * > > > > > + * This function is DEPRECATED. Do not use it in new code. > > > > > + * Use pci_alloc_irq_vectors() instead (there is no managed > > > > > version, currently). > > > > > */ > > > > > int pcim_intx(struct pci_dev *pdev, int enable) > > > > > { > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > index 7ce1d0e3a1d5..dc69e23b8982 100644 > > > > > --- a/drivers/pci/pci.c > > > > > +++ b/drivers/pci/pci.c > > > > > @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct pci_dev > > > > > *dev) > > > > > } > > > > > > > > > > /** > > > > > - * pci_intx - enables/disables PCI INTx for device dev > > > > > + * pci_intx - enables/disables PCI INTx for device dev > > > > > (DEPRECATED) > > > > > * @pdev: the PCI device to operate on > > > > > * @enable: boolean: whether to enable or disable PCI INTx > > > > > * > > > > > * Enables/disables PCI INTx for device @pdev > > > > > + * > > > > > + * This function is DEPRECATED. Do not use it in new code. > > > > > + * Use pci_alloc_irq_vectors() instead. > > > > > */ > > > > > void pci_intx(struct pci_dev *pdev, int enable) > > > > > { > > > > > > > > > > > > >
On Fri, 2024-10-18 at 18:45 -0500, Bjorn Helgaas wrote: > On Wed, Oct 16, 2024 at 10:53:16AM +0200, Philipp Stanner wrote: > > On Wed, 2024-10-16 at 10:43 +0200, Heiner Kallweit wrote: > > > On 16.10.2024 08:57, Philipp Stanner wrote: > > > > On Tue, 2024-10-15 at 13:53 -0600, Alex Williamson wrote: > > > > > On Tue, 15 Oct 2024 20:51:23 +0200 > > > > > Philipp Stanner <pstanner@redhat.com> wrote: > > > > > > > > > > > pci_intx() and its managed counterpart pcim_intx() only > > > > > > exist > > > > > > for > > > > > > older > > > > > > drivers which have not been ported yet for various reasons. > > > > > > Future > > > > > > drivers should preferably use pci_alloc_irq_vectors(). > > > > > > > > > > > > Mark pci_intx() and pcim_intx() as deprecated and encourage > > > > > > usage > > > > > > of > > > > > > pci_alloc_irq_vectors() in its place. > > > > > > > > > > I don't really understand this. As we've discussed > > > > > previously > > > > > pci_alloc_irq_vectors() is, unsurprisingly, for allocating > > > > > PCI > > > > > IRQ > > > > > vectors while pci_intx() is for manipulating the INTx disable > > > > > bit > > > > > on > > > > > PCI devices. The latter is a generic mechanism for > > > > > preventing > > > > > PCI > > > > > devices from generating INTx, regardless of whether there's a > > > > > vector > > > > > allocated for it. How does the former replace the latter and > > > > > why > > > > > do > > > > > we > > > > > feel the need to deprecate the latter? > > > > > > > > > > It feels like this fits some narrow narrative and makes all > > > > > users > > > > > of > > > > > these now deprecated functions second class citizens. Why? > > > > > At > > > > > it's > > > > > root these are simply providing mask and set or mask and > > > > > clear > > > > > register > > > > > bit operations. Thanks, > > > > > > > > I got the feeling from the RFC discussion that that was > > > > basically > > > > the > > > > consensus: people should use pci_alloc_irq_vectors(). Or did I > > > > misunderstand Andy and Heiner? > > > > > > > I think there are two different use cases for pci_intx(). > > > At first there are several drivers where the direct usage of > > > pci_intx() > > > can be eliminated by switching to the pci_alloc_irq_vectors() > > > API. > > > > > > And then there's usage of pci_intx() in > > > drivers/vfio/pci/vfio_pci_intrs.c > > > drivers/xen/xen-pciback/conf_space_header.c > > > There we have to keep the (AFAICS unmanaged) pci_intx() calls. > > > > There is also the usage within PCI itself, in MSI. Patch №8 touches > > that. > > > > It's why I think this series should land before anyone should port > > direct pci_intx() users to the irq vectors function, because the > > latter > > also uses pci_intx() and its own devres, which sounds explosive to > > me. > > > > > > I'm perfectly happy with dropping this patch and continue > > > > offering > > > > pci{m}_intx() to users, since after removing that hybrid > > > > hazzard I > > > > don't see any harm in them anymore. > > So is the bottom line that we should drop *this* patch and apply the > rest of the series? Yes Sir, that's the idea Regards, P. > > > > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > > > > > --- > > > > > > drivers/pci/devres.c | 5 ++++- > > > > > > drivers/pci/pci.c | 5 ++++- > > > > > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c > > > > > > index 6f8f712fe34e..4c76fc063104 100644 > > > > > > --- a/drivers/pci/devres.c > > > > > > +++ b/drivers/pci/devres.c > > > > > > @@ -435,7 +435,7 @@ static struct pcim_intx_devres > > > > > > *get_or_create_intx_devres(struct device *dev) > > > > > > } > > > > > > > > > > > > /** > > > > > > - * pcim_intx - managed pci_intx() > > > > > > + * pcim_intx - managed pci_intx() (DEPRECATED) > > > > > > * @pdev: the PCI device to operate on > > > > > > * @enable: boolean: whether to enable or disable PCI INTx > > > > > > * > > > > > > @@ -443,6 +443,9 @@ static struct pcim_intx_devres > > > > > > *get_or_create_intx_devres(struct device *dev) > > > > > > * > > > > > > * Enable/disable PCI INTx for device @pdev. > > > > > > * Restore the original state on driver detach. > > > > > > + * > > > > > > + * This function is DEPRECATED. Do not use it in new code. > > > > > > + * Use pci_alloc_irq_vectors() instead (there is no > > > > > > managed > > > > > > version, currently). > > > > > > */ > > > > > > int pcim_intx(struct pci_dev *pdev, int enable) > > > > > > { > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > > > > > index 7ce1d0e3a1d5..dc69e23b8982 100644 > > > > > > --- a/drivers/pci/pci.c > > > > > > +++ b/drivers/pci/pci.c > > > > > > @@ -4477,11 +4477,14 @@ void pci_disable_parity(struct > > > > > > pci_dev > > > > > > *dev) > > > > > > } > > > > > > > > > > > > /** > > > > > > - * pci_intx - enables/disables PCI INTx for device dev > > > > > > + * pci_intx - enables/disables PCI INTx for device dev > > > > > > (DEPRECATED) > > > > > > * @pdev: the PCI device to operate on > > > > > > * @enable: boolean: whether to enable or disable PCI INTx > > > > > > * > > > > > > * Enables/disables PCI INTx for device @pdev > > > > > > + * > > > > > > + * This function is DEPRECATED. Do not use it in new code. > > > > > > + * Use pci_alloc_irq_vectors() instead. > > > > > > */ > > > > > > void pci_intx(struct pci_dev *pdev, int enable) > > > > > > { > > > > > > > > > > > > > > > > > > >
On Tue, Oct 15, 2024 at 08:51:10PM +0200, Philipp Stanner wrote: > @Driver-Maintainers: Your driver might be touched by patch "Remove > devres from pci_intx()". You might want to take a look. > > Changes since the RFC [1]: > - Add a patch deprecating pci{m}_intx(). (Heiner, Andy, Me) > - Add Acked-by's already given. > - Export pcim_intx() as a GPL function. (Alex) > - Drop patch for rts5280, since this driver will be removed quite > soon. (Philipp Hortmann, Greg) > - Use early-return in pci_intx_unmanaged() and pci_intx(). (Andy) > > Hi all, > > this series removes a problematic feature from pci_intx(). That function > sometimes implicitly uses devres for automatic cleanup. We should get > rid of this implicit behavior. > > To do so, a pci_intx() version that is always-managed, and one that is > never-managed are provided. Then, all pci_intx() users are ported to the > version they need. Afterwards, pci_intx() can be cleaned up and the > users of the never-managed version be ported back to pci_intx(). > > This way we'd get this PCI API consistent again. > > Patch "Remove devres from pci_intx()" obviously reverts the previous > patches that made drivers use pci_intx_unmanaged(). But this way it's > easier to review and approve. It also makes sure that each checked out > commit should provide correct behavior, not just the entire series as a > whole. > > Merge plan for this is to enter through the PCI tree. > > [1] https://lore.kernel.org/all/20241009083519.10088-1-pstanner@redhat.com/ I *think* this series depends on resolution of Takashi's "Restore the original INTX_DISABLE bit by pcim_intx()" patch [2], right? For now I'm postponing this series, but let me know if that's not the right thing. [2] https://lore.kernel.org/r/20241024155539.19416-1-tiwai@suse.de > Philipp Stanner (13): > PCI: Prepare removing devres from pci_intx() > ALSA: hda_intel: Use always-managed version of pcim_intx() > drivers/xen: Use never-managed version of pci_intx() > net/ethernet: Use never-managed version of pci_intx() > net/ntb: Use never-managed version of pci_intx() > misc: Use never-managed version of pci_intx() > vfio/pci: Use never-managed version of pci_intx() > PCI: MSI: Use never-managed version of pci_intx() > ata: Use always-managed version of pci_intx() > wifi: qtnfmac: use always-managed version of pcim_intx() > HID: amd_sfh: Use always-managed version of pcim_intx() > Remove devres from pci_intx() > PCI: Deprecate pci_intx(), pcim_intx() > > drivers/ata/ahci.c | 2 +- > drivers/ata/ata_piix.c | 2 +- > drivers/ata/pata_rdc.c | 2 +- > drivers/ata/sata_sil24.c | 2 +- > drivers/ata/sata_sis.c | 2 +- > drivers/ata/sata_uli.c | 2 +- > drivers/ata/sata_vsc.c | 2 +- > drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 +-- > drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 2 +- > .../wireless/quantenna/qtnfmac/pcie/pcie.c | 2 +- > drivers/pci/devres.c | 29 +++++-------------- > drivers/pci/pci.c | 19 ++++-------- > include/linux/pci.h | 1 + > sound/pci/hda/hda_intel.c | 2 +- > 14 files changed, 26 insertions(+), 47 deletions(-) > > -- > 2.47.0 >
On Wed, 30 Oct 2024 23:17:37 +0100, Bjorn Helgaas wrote: > > On Tue, Oct 15, 2024 at 08:51:10PM +0200, Philipp Stanner wrote: > > @Driver-Maintainers: Your driver might be touched by patch "Remove > > devres from pci_intx()". You might want to take a look. > > > > Changes since the RFC [1]: > > - Add a patch deprecating pci{m}_intx(). (Heiner, Andy, Me) > > - Add Acked-by's already given. > > - Export pcim_intx() as a GPL function. (Alex) > > - Drop patch for rts5280, since this driver will be removed quite > > soon. (Philipp Hortmann, Greg) > > - Use early-return in pci_intx_unmanaged() and pci_intx(). (Andy) > > > > Hi all, > > > > this series removes a problematic feature from pci_intx(). That function > > sometimes implicitly uses devres for automatic cleanup. We should get > > rid of this implicit behavior. > > > > To do so, a pci_intx() version that is always-managed, and one that is > > never-managed are provided. Then, all pci_intx() users are ported to the > > version they need. Afterwards, pci_intx() can be cleaned up and the > > users of the never-managed version be ported back to pci_intx(). > > > > This way we'd get this PCI API consistent again. > > > > Patch "Remove devres from pci_intx()" obviously reverts the previous > > patches that made drivers use pci_intx_unmanaged(). But this way it's > > easier to review and approve. It also makes sure that each checked out > > commit should provide correct behavior, not just the entire series as a > > whole. > > > > Merge plan for this is to enter through the PCI tree. > > > > [1] https://lore.kernel.org/all/20241009083519.10088-1-pstanner@redhat.com/ > > I *think* this series depends on resolution of Takashi's "Restore the > original INTX_DISABLE bit by pcim_intx()" patch [2], right? IIUC, it's not really dependent, as pcim_intx() has been used in pci_intx() internally when the PCI device is already managed. My patch is to correct the already existing behavior. So I guess you can take this series, and I'll post the revised patch later (sorry, I was too busy for other tasks). thanks, Takashi > > For now I'm postponing this series, but let me know if that's not the > right thing. > > [2] https://lore.kernel.org/r/20241024155539.19416-1-tiwai@suse.de > > > Philipp Stanner (13): > > PCI: Prepare removing devres from pci_intx() > > ALSA: hda_intel: Use always-managed version of pcim_intx() > > drivers/xen: Use never-managed version of pci_intx() > > net/ethernet: Use never-managed version of pci_intx() > > net/ntb: Use never-managed version of pci_intx() > > misc: Use never-managed version of pci_intx() > > vfio/pci: Use never-managed version of pci_intx() > > PCI: MSI: Use never-managed version of pci_intx() > > ata: Use always-managed version of pci_intx() > > wifi: qtnfmac: use always-managed version of pcim_intx() > > HID: amd_sfh: Use always-managed version of pcim_intx() > > Remove devres from pci_intx() > > PCI: Deprecate pci_intx(), pcim_intx() > > > > drivers/ata/ahci.c | 2 +- > > drivers/ata/ata_piix.c | 2 +- > > drivers/ata/pata_rdc.c | 2 +- > > drivers/ata/sata_sil24.c | 2 +- > > drivers/ata/sata_sis.c | 2 +- > > drivers/ata/sata_uli.c | 2 +- > > drivers/ata/sata_vsc.c | 2 +- > > drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 +-- > > drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 2 +- > > .../wireless/quantenna/qtnfmac/pcie/pcie.c | 2 +- > > drivers/pci/devres.c | 29 +++++-------------- > > drivers/pci/pci.c | 19 ++++-------- > > include/linux/pci.h | 1 + > > sound/pci/hda/hda_intel.c | 2 +- > > 14 files changed, 26 insertions(+), 47 deletions(-) > > > > -- > > 2.47.0 > >
On Thu, 2024-10-31 at 10:19 +0100, Takashi Iwai wrote: > On Wed, 30 Oct 2024 23:17:37 +0100, > Bjorn Helgaas wrote: > > > > On Tue, Oct 15, 2024 at 08:51:10PM +0200, Philipp Stanner wrote: > > > @Driver-Maintainers: Your driver might be touched by patch > > > "Remove > > > devres from pci_intx()". You might want to take a look. > > > > > > Changes since the RFC [1]: > > > - Add a patch deprecating pci{m}_intx(). (Heiner, Andy, Me) > > > - Add Acked-by's already given. > > > - Export pcim_intx() as a GPL function. (Alex) > > > - Drop patch for rts5280, since this driver will be removed > > > quite > > > soon. (Philipp Hortmann, Greg) > > > - Use early-return in pci_intx_unmanaged() and pci_intx(). > > > (Andy) > > > > > > Hi all, > > > > > > this series removes a problematic feature from pci_intx(). That > > > function > > > sometimes implicitly uses devres for automatic cleanup. We should > > > get > > > rid of this implicit behavior. > > > > > > To do so, a pci_intx() version that is always-managed, and one > > > that is > > > never-managed are provided. Then, all pci_intx() users are ported > > > to the > > > version they need. Afterwards, pci_intx() can be cleaned up and > > > the > > > users of the never-managed version be ported back to pci_intx(). > > > > > > This way we'd get this PCI API consistent again. > > > > > > Patch "Remove devres from pci_intx()" obviously reverts the > > > previous > > > patches that made drivers use pci_intx_unmanaged(). But this way > > > it's > > > easier to review and approve. It also makes sure that each > > > checked out > > > commit should provide correct behavior, not just the entire > > > series as a > > > whole. > > > > > > Merge plan for this is to enter through the PCI tree. > > > > > > [1] > > > https://lore.kernel.org/all/20241009083519.10088-1-pstanner@redhat.com/ > > > > I *think* this series depends on resolution of Takashi's "Restore > > the > > original INTX_DISABLE bit by pcim_intx()" patch [2], right? > > IIUC, it's not really dependent, as pcim_intx() has been used in > pci_intx() internally when the PCI device is already managed. > My patch is to correct the already existing behavior. IOW, pcim_intx() does not behave correctly, independently from removing the call to it in pci_intx(). > So I guess you > can take this series, and I'll post the revised patch later (sorry, I > was too busy for other tasks). > > > thanks, > > Takashi > > > > > For now I'm postponing this series, but let me know if that's not > > the > > right thing. There are still several reviews / acks missing from the respective driver maintainers, so there's no hurry with this series regarding your side ;) Regards P. > > > > [2] https://lore.kernel.org/r/20241024155539.19416-1-tiwai@suse.de > > > > > Philipp Stanner (13): > > > PCI: Prepare removing devres from pci_intx() > > > ALSA: hda_intel: Use always-managed version of pcim_intx() > > > drivers/xen: Use never-managed version of pci_intx() > > > net/ethernet: Use never-managed version of pci_intx() > > > net/ntb: Use never-managed version of pci_intx() > > > misc: Use never-managed version of pci_intx() > > > vfio/pci: Use never-managed version of pci_intx() > > > PCI: MSI: Use never-managed version of pci_intx() > > > ata: Use always-managed version of pci_intx() > > > wifi: qtnfmac: use always-managed version of pcim_intx() > > > HID: amd_sfh: Use always-managed version of pcim_intx() > > > Remove devres from pci_intx() > > > PCI: Deprecate pci_intx(), pcim_intx() > > > > > > drivers/ata/ahci.c | 2 +- > > > drivers/ata/ata_piix.c | 2 +- > > > drivers/ata/pata_rdc.c | 2 +- > > > drivers/ata/sata_sil24.c | 2 +- > > > drivers/ata/sata_sis.c | 2 +- > > > drivers/ata/sata_uli.c | 2 +- > > > drivers/ata/sata_vsc.c | 2 +- > > > drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 +-- > > > drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 2 +- > > > .../wireless/quantenna/qtnfmac/pcie/pcie.c | 2 +- > > > drivers/pci/devres.c | 29 +++++-------- > > > ------ > > > drivers/pci/pci.c | 19 ++++-------- > > > include/linux/pci.h | 1 + > > > sound/pci/hda/hda_intel.c | 2 +- > > > 14 files changed, 26 insertions(+), 47 deletions(-) > > > > > > -- > > > 2.47.0 > > > >
@Driver-Maintainers: Your driver might be touched by patch "Remove devres from pci_intx()". You might want to take a look. Changes since the RFC [1]: - Add a patch deprecating pci{m}_intx(). (Heiner, Andy, Me) - Add Acked-by's already given. - Export pcim_intx() as a GPL function. (Alex) - Drop patch for rts5280, since this driver will be removed quite soon. (Philipp Hortmann, Greg) - Use early-return in pci_intx_unmanaged() and pci_intx(). (Andy) Hi all, this series removes a problematic feature from pci_intx(). That function sometimes implicitly uses devres for automatic cleanup. We should get rid of this implicit behavior. To do so, a pci_intx() version that is always-managed, and one that is never-managed are provided. Then, all pci_intx() users are ported to the version they need. Afterwards, pci_intx() can be cleaned up and the users of the never-managed version be ported back to pci_intx(). This way we'd get this PCI API consistent again. Patch "Remove devres from pci_intx()" obviously reverts the previous patches that made drivers use pci_intx_unmanaged(). But this way it's easier to review and approve. It also makes sure that each checked out commit should provide correct behavior, not just the entire series as a whole. Merge plan for this is to enter through the PCI tree. [1] https://lore.kernel.org/all/20241009083519.10088-1-pstanner@redhat.com/ Regards, P. Philipp Stanner (13): PCI: Prepare removing devres from pci_intx() ALSA: hda_intel: Use always-managed version of pcim_intx() drivers/xen: Use never-managed version of pci_intx() net/ethernet: Use never-managed version of pci_intx() net/ntb: Use never-managed version of pci_intx() misc: Use never-managed version of pci_intx() vfio/pci: Use never-managed version of pci_intx() PCI: MSI: Use never-managed version of pci_intx() ata: Use always-managed version of pci_intx() wifi: qtnfmac: use always-managed version of pcim_intx() HID: amd_sfh: Use always-managed version of pcim_intx() Remove devres from pci_intx() PCI: Deprecate pci_intx(), pcim_intx() drivers/ata/ahci.c | 2 +- drivers/ata/ata_piix.c | 2 +- drivers/ata/pata_rdc.c | 2 +- drivers/ata/sata_sil24.c | 2 +- drivers/ata/sata_sis.c | 2 +- drivers/ata/sata_uli.c | 2 +- drivers/ata/sata_vsc.c | 2 +- drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 +-- drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 2 +- .../wireless/quantenna/qtnfmac/pcie/pcie.c | 2 +- drivers/pci/devres.c | 29 +++++-------------- drivers/pci/pci.c | 19 ++++-------- include/linux/pci.h | 1 + sound/pci/hda/hda_intel.c | 2 +- 14 files changed, 26 insertions(+), 47 deletions(-)