On Thu, 2024-10-10 at 11:43 -0600, Alex Williamson wrote:
> On Wed, 9 Oct 2024 10:35:07 +0200
> Philipp Stanner <pstanner@xxxxxxxxxx> wrote:
>
> > pci_intx() is a hybrid function which sometimes performs devres
> > operations, depending on whether pcim_enable_device() has been used
> > to
> > enable the pci_dev. This sometimes-managed nature of the function
> > is
> > problematic. Notably, it causes the function to allocate under some
> > circumstances which makes it unusable from interrupt context.
> >
> > To, ultimately, remove the hybrid nature from pci_intx(), it is
> > first
> > necessary to provide an always-managed and a never-managed version
> > of that function. Then, all callers of pci_intx() can be ported to
> > the
> > version they need, depending whether they use pci_enable_device()
> > or
> > pcim_enable_device().
> >
> > An always-managed function exists, namely pcim_intx(), for which
> > __pcim_intx(), a never-managed version of pci_intx() had been
> > implemented.
> >
> > Make __pcim_intx() a public function under the name
> > pci_intx_unmanaged(). Make pcim_intx() a public function.
> >
> > Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> > ---
> > drivers/pci/devres.c | 24 +++---------------------
> > drivers/pci/pci.c | 26 ++++++++++++++++++++++++++
> > include/linux/pci.h | 2 ++
> > 3 files changed, 31 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> > index b133967faef8..475a3ae5c33f 100644
> > --- a/drivers/pci/devres.c
> > +++ b/drivers/pci/devres.c
> > @@ -411,31 +411,12 @@ static inline bool mask_contains_bar(int
> > mask, int bar)
> > return mask & BIT(bar);
> > }
> >
> > -/*
> > - * This is a copy of pci_intx() used to bypass the problem of
> > recursive
> > - * function calls due to the hybrid nature of pci_intx().
> > - */
> > -static void __pcim_intx(struct pci_dev *pdev, int enable)
> > -{
> > - u16 pci_command, new;
> > -
> > - pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
> > -
> > - if (enable)
> > - new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
> > - else
> > - new = pci_command | PCI_COMMAND_INTX_DISABLE;
> > -
> > - if (new != pci_command)
> > - pci_write_config_word(pdev, PCI_COMMAND, new);
> > -}
> > -
> > static void pcim_intx_restore(struct device *dev, void *data)
> > {
> > struct pci_dev *pdev = to_pci_dev(dev);
> > struct pcim_intx_devres *res = data;
> >
> > - __pcim_intx(pdev, res->orig_intx);
> > + pci_intx_unmanaged(pdev, res->orig_intx);
> > }
> >
> > static struct pcim_intx_devres *get_or_create_intx_devres(struct
> > device *dev)
> > @@ -472,10 +453,11 @@ int pcim_intx(struct pci_dev *pdev, int
> > enable)
> > return -ENOMEM;
> >
> > res->orig_intx = !enable;
> > - __pcim_intx(pdev, enable);
> > + pci_intx_unmanaged(pdev, enable);
> >
> > return 0;
> > }
> > +EXPORT_SYMBOL(pcim_intx);
>
> What precludes this from _GPL? Also note that this is now calling a
> GPL symbol, so by default I'd assume it should also be GPL. Thanks,
Ah right, I overlooked that pci_intx() also has the _GPL version.
Will make consistent.
Thx,
P.
>
> Alex
>
> >
> > static void pcim_disable_device(void *pdev_raw)
> > {
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 7d85c04fbba2..318cfb5b5e15 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -4476,6 +4476,32 @@ void pci_disable_parity(struct pci_dev *dev)
> > }
> > }
> >
> > +/**
> > + * pci_intx - enables/disables PCI INTx for device dev, unmanaged
> > version
> > + * @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 behavios identically to pci_intx(), but is never
> > managed with
> > + * devres.
> > + */
> > +void pci_intx_unmanaged(struct pci_dev *pdev, int enable)
> > +{
> > + u16 pci_command, new;
> > +
> > + pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
> > +
> > + if (enable)
> > + new = pci_command & ~PCI_COMMAND_INTX_DISABLE;
> > + else
> > + new = pci_command | PCI_COMMAND_INTX_DISABLE;
> > +
> > + if (new != pci_command)
> > + pci_write_config_word(pdev, PCI_COMMAND, new);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_intx_unmanaged);
> > +
> > /**
> > * pci_intx - enables/disables PCI INTx for device dev
> > * @pdev: the PCI device to operate on
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 573b4c4c2be6..6b8cde76d564 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1353,6 +1353,7 @@ int __must_check pcim_set_mwi(struct pci_dev
> > *dev);
> > int pci_try_set_mwi(struct pci_dev *dev);
> > void pci_clear_mwi(struct pci_dev *dev);
> > void pci_disable_parity(struct pci_dev *dev);
> > +void pci_intx_unmanaged(struct pci_dev *pdev, int enable);
> > void pci_intx(struct pci_dev *dev, int enable);
> > bool pci_check_and_mask_intx(struct pci_dev *dev);
> > bool pci_check_and_unmask_intx(struct pci_dev *dev);
> > @@ -2293,6 +2294,7 @@ static inline void pci_fixup_device(enum
> > pci_fixup_pass pass,
> > struct pci_dev *dev) { }
> > #endif
> >
> > +int pcim_intx(struct pci_dev *pdev, int enabled);
> > void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned
> > long maxlen);
> > void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar,
> > const char *name);
>
