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,
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);
