Re: [RFC PATCH 01/13] PCI: Prepare removing devres from pci_intx()

[Philipp Stanner <pstanner@xxxxxxxxxx>]

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