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

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

[Alex Williamson]

On Wed,  9 Oct 2024 10:35:07 +0200
Philipp Stanner <pstanner@redhat.com> 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@redhat.com>
> ---
>  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);

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

[Philipp Stanner]

On Thu, 2024-10-10 at 11:43 -0600, Alex Williamson wrote:
> On Wed,  9 Oct 2024 10:35:07 +0200
> Philipp Stanner <pstanner@redhat.com> 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@redhat.com>
> > ---
> >  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);
> 

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

[Philipp Stanner]

On Thu, 2024-10-10 at 17:40 +0300, Andy Shevchenko wrote:
> On Wed, Oct 09, 2024 at 10:35:07AM +0200, Philipp Stanner 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.
> 
> To avoid an additional churn we can make just completely new APIs,
> namely:
> pcim_int_x()
> pci_int_x()
> 
> You won't need all dirty dances with double underscored function
> naming and
> renaming.

Ähm.. I can't follow. The new version doesn't use double underscores
anymore. __pcim_intx() is being removed, effectively.
After this series, we'd end up with a clean:

	pci_intx() <-> pcim_intx()

just as in the other PCI APIs.


> 
> 
> ...
> 
> > +	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)
> 
> I would use positive conditionals as easy to read (yes, a couple of
> lines
> longer, but also a win is the indentation and avoiding an additional
> churn in
> the future in case we need to add something in this branch.

I can't follow. You mean:

if (new == pci_command)
    return;

?

That's exactly the same level of indentation. Plus, I just copied the
code.

> 
> > +		pci_write_config_word(pdev, PCI_COMMAND, new);
> 
> ...
> 
> Otherwise I'm for the idea in general.

\o/

> 

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

[Andy Shevchenko]

On Fri, Oct 11, 2024 at 02:16:06PM +0200, Philipp Stanner wrote:
> On Thu, 2024-10-10 at 17:40 +0300, Andy Shevchenko wrote:
> > On Wed, Oct 09, 2024 at 10:35:07AM +0200, Philipp Stanner 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.

It seems I got confused by these two paragraphs. Why the double underscored
function is even mentioned here?

> > To avoid an additional churn we can make just completely new APIs,
> > namely:
> > pcim_int_x()
> > pci_int_x()
> > 
> > You won't need all dirty dances with double underscored function
> > naming and
> > renaming.
> 
> Ähm.. I can't follow. The new version doesn't use double underscores
> anymore. __pcim_intx() is being removed, effectively.
> After this series, we'd end up with a clean:
> 
> 	pci_intx() <-> pcim_intx()
> 
> just as in the other PCI APIs.

...

> > > +	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)
> > 
> > I would use positive conditionals as easy to read (yes, a couple of
> > lines
> > longer, but also a win is the indentation and avoiding an additional
> > churn in
> > the future in case we need to add something in this branch.
> 
> I can't follow. You mean:
> 
> if (new == pci_command)
>     return;
> 
> ?
> 
> That's exactly the same level of indentation.

No, the body gets one level off.

> Plus, I just copied the code.
> 
> > > +		pci_write_config_word(pdev, PCI_COMMAND, new);

	if (new == pci_command)
		return;

	pci_write_config_word(pdev, PCI_COMMAND, new);

See the difference?
Also, imaging adding a new code in your case:

	if (new != pci_command)
		pci_write_config_word(pdev, PCI_COMMAND, new);

==>

	if (new != pci_command) {
		...foo...
		pci_write_config_word(pdev, PCI_COMMAND, new);
		...bar...
	}

And in mine:

	if (new == pci_command)
		return;

	...foo...
	pci_write_config_word(pdev, PCI_COMMAND, new);
	...bar...

I hope it's clear now what I meant.

-- 
With Best Regards,
Andy Shevchenko

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

[Philipp Stanner]

On Fri, 2024-10-11 at 16:50 +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2024 at 02:16:06PM +0200, Philipp Stanner wrote:
> > On Thu, 2024-10-10 at 17:40 +0300, Andy Shevchenko wrote:
> > > On Wed, Oct 09, 2024 at 10:35:07AM +0200, Philipp Stanner 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.
> 
> It seems I got confused by these two paragraphs. Why the double
> underscored
> function is even mentioned here?

It's mentioned because it's being moved.

> 
> > > To avoid an additional churn we can make just completely new
> > > APIs,
> > > namely:
> > > pcim_int_x()
> > > pci_int_x()
> > > 
> > > You won't need all dirty dances with double underscored function
> > > naming and
> > > renaming.
> > 
> > Ähm.. I can't follow. The new version doesn't use double
> > underscores
> > anymore. __pcim_intx() is being removed, effectively.
> > After this series, we'd end up with a clean:
> > 
> > 	pci_intx() <-> pcim_intx()
> > 
> > just as in the other PCI APIs.
> 
> ...
> 
> > > > +	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)
> > > 
> > > I would use positive conditionals as easy to read (yes, a couple
> > > of
> > > lines
> > > longer, but also a win is the indentation and avoiding an
> > > additional
> > > churn in
> > > the future in case we need to add something in this branch.
> > 
> > I can't follow. You mean:
> > 
> > if (new == pci_command)
> >     return;
> > 
> > ?
> > 
> > That's exactly the same level of indentation.
> 
> No, the body gets one level off.
> 
> > Plus, I just copied the code.
> > 
> > > > +		pci_write_config_word(pdev, PCI_COMMAND, new);
> 
> 	if (new == pci_command)
> 		return;
> 
> 	pci_write_config_word(pdev, PCI_COMMAND, new);
> 
> See the difference?
> Also, imaging adding a new code in your case:
> 
> 	if (new != pci_command)
> 		pci_write_config_word(pdev, PCI_COMMAND, new);
> 
> ==>
> 
> 	if (new != pci_command) {
> 		...foo...
> 		pci_write_config_word(pdev, PCI_COMMAND, new);
> 		...bar...
> 	}
> 
> And in mine:
> 
> 	if (new == pci_command)
> 		return;
> 
> 	...foo...
> 	pci_write_config_word(pdev, PCI_COMMAND, new);
> 	...bar...
> 
> I hope it's clear now what I meant.

It is clear.. I'm not necessarily convinced that it's better to review
than just copying the pre-existing code, but if you really want it we
can do it I guess.

P.

Re: [RFC PATCH 13/13] Remove devres from pci_intx()

[Alex Williamson]

On Thu, 10 Oct 2024 11:11:36 +0200
Philipp Stanner <pstanner@redhat.com> wrote:

> On Thu, 2024-10-10 at 11:50 +0300, Dan Carpenter wrote:
> > On Wed, Oct 09, 2024 at 10:35:19AM +0200, Philipp Stanner wrote:  
> > > pci_intx() is a hybrid function which can sometimes be managed
> > > through
> > > devres. This hybrid nature is undesirable.
> > > 
> > > Since all users of pci_intx() have by now been ported either to
> > > always-managed pcim_intx() or never-managed pci_intx_unmanaged(),
> > > the
> > > devres functionality can be removed from pci_intx().
> > > 
> > > Consequently, pci_intx_unmanaged() is now redundant, because
> > > pci_intx()
> > > itself is now unmanaged.
> > > 
> > > Remove the devres functionality from pci_intx(). Remove
> > > pci_intx_unmanaged().
> > > Have all users of pci_intx_unmanaged() call pci_intx().
> > > 
> > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>  
> > 
> > I don't like when we change a function like this but it still
> > compiles fine.
> > If someone is working on a driver and hasn't pushed it yet, then it's
> > probably
> > supposed to be using the new pcim_intx() but they won't discover that
> > until they
> > detect the leaks at runtime.  
> 
> There wouldn't be any *leaks*, it's just that the INTx state would not
> automatically be restored. BTW the official documentation in its
> current state does not hint at pci_intx() doing anything automatically,
> but rather actively marks it as deprecated.
> 
> But you are right that a hypothetical new driver and OOT drivers could
> experience bugs through this change.
> 
> > 
> > Why not leave the pci_intx_unmanaged() name.  It's ugly and that will
> > discorage
> > people from introducing new uses.  
> 
> I'd be OK with that. Then we'd have to remove pci_intx() as it has new
> users anymore.
> 
> Either way should be fine and keep the behavior for existing drivers
> identical.
> 
> I think Bjorn should express a preference

FWIW, I think pcim_intx() and pci_intx() align better to our naming
convention for devres interfaces.  Would it be sufficient if pci_intx()
triggered a WARN_ON if called for a pci_is_managed() device?  Thanks,

Alex

Re: [RFC PATCH 13/13] Remove devres from pci_intx()

[Dan Carpenter]

On Thu, Oct 10, 2024 at 11:43:14AM -0600, Alex Williamson wrote:
> FWIW, I think pcim_intx() and pci_intx() align better to our naming
> convention for devres interfaces.  Would it be sufficient if pci_intx()
> triggered a WARN_ON if called for a pci_is_managed() device?  Thanks,
> 

To be honest, I also don't mind if you also just merge the patchset as-is.  I
was mostly just throwing out ideas.

regards,
dan carpenter

Re: [RFC PATCH 13/13] Remove devres from pci_intx()

[Philipp Stanner]

On Thu, 2024-10-10 at 11:43 -0600, Alex Williamson wrote:
> On Thu, 10 Oct 2024 11:11:36 +0200
> Philipp Stanner <pstanner@redhat.com> wrote:
> 
> > On Thu, 2024-10-10 at 11:50 +0300, Dan Carpenter wrote:
> > > On Wed, Oct 09, 2024 at 10:35:19AM +0200, Philipp Stanner wrote: 
> > > > pci_intx() is a hybrid function which can sometimes be managed
> > > > through
> > > > devres. This hybrid nature is undesirable.
> > > > 
> > > > Since all users of pci_intx() have by now been ported either to
> > > > always-managed pcim_intx() or never-managed
> > > > pci_intx_unmanaged(),
> > > > the
> > > > devres functionality can be removed from pci_intx().
> > > > 
> > > > Consequently, pci_intx_unmanaged() is now redundant, because
> > > > pci_intx()
> > > > itself is now unmanaged.
> > > > 
> > > > Remove the devres functionality from pci_intx(). Remove
> > > > pci_intx_unmanaged().
> > > > Have all users of pci_intx_unmanaged() call pci_intx().
> > > > 
> > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com>  
> > > 
> > > I don't like when we change a function like this but it still
> > > compiles fine.
> > > If someone is working on a driver and hasn't pushed it yet, then
> > > it's
> > > probably
> > > supposed to be using the new pcim_intx() but they won't discover
> > > that
> > > until they
> > > detect the leaks at runtime.  
> > 
> > There wouldn't be any *leaks*, it's just that the INTx state would
> > not
> > automatically be restored. BTW the official documentation in its
> > current state does not hint at pci_intx() doing anything
> > automatically,
> > but rather actively marks it as deprecated.
> > 
> > But you are right that a hypothetical new driver and OOT drivers
> > could
> > experience bugs through this change.
> > 
> > > 
> > > Why not leave the pci_intx_unmanaged() name.  It's ugly and that
> > > will
> > > discorage
> > > people from introducing new uses.  
> > 
> > I'd be OK with that. Then we'd have to remove pci_intx() as it has
> > new
> > users anymore.
> > 
> > Either way should be fine and keep the behavior for existing
> > drivers
> > identical.
> > 
> > I think Bjorn should express a preference
> 
> FWIW, I think pcim_intx() and pci_intx() align better to our naming
> convention for devres interfaces.

Yup, also my personal preference. But we can mark those functions as
deprecated via docstring-comment. That should fullfill Damien's goal.

>   Would it be sufficient if pci_intx()
> triggered a WARN_ON if called for a pci_is_managed() device?

No, I don't think that's a good idea; reason being that
pci_is_managed() just checks that global boolean which we inherited
from the old implementation and which should not be necessary with
proper devres.
The boolean is used for making functions such as pci_intx() and
__pci_request_region() hybrid. So with our non-hybrid version we never
need it.

P.

>   Thanks,
> 
> Alex
> 

Re: [RFC PATCH 02/13] ALSA: hda: hda_intel: Use always-managed version of pcim_intx()

[Philipp Stanner]

On Thu, 2024-10-10 at 17:46 +0300, Andy Shevchenko wrote:
> On Wed, Oct 09, 2024 at 10:35:08AM +0200, 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.
> > 
> > hda_intel enables its PCI-Device with pcim_enable_device(). Thus,
> > it needs
> > the always-managed version.
> > 
> > Replace pci_intx() with pcim_intx().
> 
> ...
> 
> >  	bus->irq = chip->pci->irq;
> >  	chip->card->sync_irq = bus->irq;
> > -	pci_intx(chip->pci, !chip->msi);
> > +	pcim_intx(chip->pci, !chip->msi);
> >  	return 0;
> 
> I believe each driver needs an individual approach. Looking at the
> above
> I would first to understand why this one is being used and why we
> can't
> switch to pci{m}_alloc_irq_vectors(). (Yeah, managed
> pci_alloc_irq_vectors()
> is probably still missing, I don't remember if you introduced it or
> not.
> 

Alright alright – we touched it in the other mail briefly, but let me
point out another specific problem:

pci_alloc_irq_vectors() *uses* pci_intx(). And pci_intx() can be
managed sometimes.

See the problem? :(

So it's not just that I couldn't port the driver Alex is concerned
about, it's also that MSI itself is a user of pci_intx().

So a pcim_alloc_irq_vectors() might end up doing double-devres or God
knows what else. Only once pci_intx() is clean one can start thinking
about the code in pci/msi/

It's the biggest reason why I want to clean it up as suggested here,
and also why the only patch I'm really nervous about is number 8.


P.