[PATCHv3,02/11] pci: use weak functions for MSI arch-specific functions

Until now, the MSI architecture-specific functions could be overloaded
using a fairly complex set of #define and compile-time
conditionals. In order to prepare for the introduction of the msi_chip
infrastructure, it is desirable to switch all those functions to use
the 'weak' mechanism. This commit converts all the architecture that
were overidding those MSI functions to use the new strategy.

The new strategy consists of defining two functions for each hook:

 * One function is called default_<something>() and is not weak. It
   contains the default implementation of the hook.

 * One function is called arch_<something>() and is weak. The default
   implementation (in drivers/pci/msi.c) simply calls the
   corresponding default_<something>() function.

The idea is that architecture-specific code can re-implement its own,
non-weak, version of arch_<something>(). And to achieve this, it may
wish to call back into the default implementation provided by the PCI
core, which is possible thanks to the default_<something>() function.

This is needed because the x86 Xen implementation of
teardown_msi_irqs() needs to do some work, and then call the default
implementation of this hook.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/mips/include/asm/pci.h    |  5 ----
 arch/powerpc/include/asm/pci.h |  5 ----
 arch/s390/include/asm/pci.h    |  4 ---
 arch/x86/include/asm/pci.h     | 28 --------------------
 arch/x86/kernel/x86_init.c     | 21 +++++++++++++++
 drivers/pci/msi.c              | 58 +++++++++++++++++++++++++++---------------
 include/linux/msi.h            | 15 ++++++++++-
 7 files changed, 72 insertions(+), 64 deletions(-)

On Wed, Jun 19, 2013 at 06:56:10PM +0200, Thomas Petazzoni wrote:
[...]
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 2c10752..4bc0c8f 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -30,19 +30,35 @@ static int pci_msi_enable = 1;
>  
>  /* Arch hooks */
>  
> -#ifndef arch_msi_check_device
> -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +int default_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> +{
> +	return -EINVAL;
> +}
> +
> +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> +{
> +	return default_setup_msi_irq(dev, desc);
> +}
> +
> +void default_teardown_msi_irq(unsigned int irq)
> +{
> +}
> +
> +void __weak arch_teardown_msi_irq(unsigned int irq)
> +{
> +	return default_teardown_msi_irq(irq);
> +}
> +
> +int default_msi_check_device(struct pci_dev *dev, int nvec, int type)
>  {
>  	return 0;
>  }
> -#endif

I don't think keeping the default_*() for these three is necessary,
given that they don't do anything and therefore no architecture is
likely to call them when overriding.

Thierry

Dear Thierry Reding,

On Thu, 20 Jun 2013 20:57:08 +0200, Thierry Reding wrote:

> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 2c10752..4bc0c8f 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -30,19 +30,35 @@ static int pci_msi_enable = 1;
> >  
> >  /* Arch hooks */
> >  
> > -#ifndef arch_msi_check_device
> > -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> > +int default_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> > +{
> > +	return -EINVAL;
> > +}
> > +
> > +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> > +{
> > +	return default_setup_msi_irq(dev, desc);
> > +}
> > +
> > +void default_teardown_msi_irq(unsigned int irq)
> > +{
> > +}
> > +
> > +void __weak arch_teardown_msi_irq(unsigned int irq)
> > +{
> > +	return default_teardown_msi_irq(irq);
> > +}
> > +
> > +int default_msi_check_device(struct pci_dev *dev, int nvec, int type)
> >  {
> >  	return 0;
> >  }
> > -#endif
> 
> I don't think keeping the default_*() for these three is necessary,
> given that they don't do anything and therefore no architecture is
> likely to call them when overriding.

Ok. I was just keeping them for the sake of consistency with the other
calls that do have a default behavior, but if it's considered not
useful, I'll get rid of them in v4.

Thanks for your comments!

Thomas

On Thu, Jun 20, 2013 at 12:57 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Jun 19, 2013 at 06:56:10PM +0200, Thomas Petazzoni wrote:
> [...]
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index 2c10752..4bc0c8f 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -30,19 +30,35 @@ static int pci_msi_enable = 1;
>>
>>  /* Arch hooks */
>>
>> -#ifndef arch_msi_check_device
>> -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
>> +int default_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
>> +{
>> +     return -EINVAL;
>> +}
>> +
>> +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
>> +{
>> +     return default_setup_msi_irq(dev, desc);
>> +}
>> +
>> +void default_teardown_msi_irq(unsigned int irq)
>> +{
>> +}
>> +
>> +void __weak arch_teardown_msi_irq(unsigned int irq)
>> +{
>> +     return default_teardown_msi_irq(irq);
>> +}
>> +
>> +int default_msi_check_device(struct pci_dev *dev, int nvec, int type)
>>  {
>>       return 0;
>>  }
>> -#endif
>
> I don't think keeping the default_*() for these three is necessary,
> given that they don't do anything and therefore no architecture is
> likely to call them when overriding.

I agree; the whole point of __weak is to provide a "default"
implementation, so I hope you can just remove any empty default_*()
functions and fold the others into the arch_*() functions directly.

If there actually are callbacks from arch-specific strong functions
back to the stuff in the default_*() functions, that suggests that we
should refactor and rename that bit of functionality.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dear Bjorn Helgaas,

On Mon, 24 Jun 2013 19:52:45 -0600, Bjorn Helgaas wrote:

> I agree; the whole point of __weak is to provide a "default"
> implementation, so I hope you can just remove any empty default_*()
> functions and fold the others into the arch_*() functions directly.
> 
> If there actually are callbacks from arch-specific strong functions
> back to the stuff in the default_*() functions, that suggests that we
> should refactor and rename that bit of functionality.

There is one such case, as highlighted in the commit log of this patch:

"""
This is needed because the x86 Xen implementation of
teardown_msi_irqs() needs to do some work, and then call the default
implementation of this hook.
"""

Any suggestion on how to solve this particular case?

Thanks,

Thomas

On Tue, Jun 25, 2013 at 3:55 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Bjorn Helgaas,
>
> On Mon, 24 Jun 2013 19:52:45 -0600, Bjorn Helgaas wrote:
>
>> I agree; the whole point of __weak is to provide a "default"
>> implementation, so I hope you can just remove any empty default_*()
>> functions and fold the others into the arch_*() functions directly.
>>
>> If there actually are callbacks from arch-specific strong functions
>> back to the stuff in the default_*() functions, that suggests that we
>> should refactor and rename that bit of functionality.
>
> There is one such case, as highlighted in the commit log of this patch:
>
> """
> This is needed because the x86 Xen implementation of
> teardown_msi_irqs() needs to do some work, and then call the default
> implementation of this hook.
> """
>
> Any suggestion on how to solve this particular case?

Nope.  Just leave it as-is for that case and consolidate what you can.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dear Bjorn Helgaas,

On Tue, 25 Jun 2013 10:20:33 -0600, Bjorn Helgaas wrote:

> > Any suggestion on how to solve this particular case?
> 
> Nope.  Just leave it as-is for that case and consolidate what you can.

So I keep the weak arch_*() function and the strong default_*() for
this particular hook, and for all the other hooks, I only keep a weak
arch_*() function. Is this what you suggest for now?

Best regards,

Thomas

On Tue, Jun 25, 2013 at 10:44 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Bjorn Helgaas,
>
> On Tue, 25 Jun 2013 10:20:33 -0600, Bjorn Helgaas wrote:
>
>> > Any suggestion on how to solve this particular case?
>>
>> Nope.  Just leave it as-is for that case and consolidate what you can.
>
> So I keep the weak arch_*() function and the strong default_*() for
> this particular hook, and for all the other hooks, I only keep a weak
> arch_*() function. Is this what you suggest for now?

Yep, that sounds good.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html