On Mon, Jan 19, 2009 at 12:07:04PM +1100, Michael Ellerman wrote:
> Weak functions aren't all they're cracked up to be. They lead to
> incorrect binaries with some toolchains, they require us to have empty
> functions we otherwise wouldn't, and the unused code is not elided
> (as of gcc 4.3.2 anyway).
>
> So replace the weak MSI arch hooks with the #define foo foo idiom. We no
> longer need empty versions of arch_setup/teardown_msi_irq().
Wait a moment. A week or so ago we just had a thread where I understood
the outcome was to:
1) black list known buggy gcc versions for this exact problem.
(http://lkml.org/lkml/2008/12/31/243)
2) add a sparse rule to catch other weak symbol abuse.
http://lkml.org/lkml/2008/12/31/189
Start of thread:
http://lkml.org/lkml/2008/12/29/267
Linus shot down a particular use of weak symbols:
http://lkml.org/lkml/2008/12/31/174
The URL in (2) also includes this output:
drivers/pci/msi.c:49:27: warning: Calling weak function 'arch_setup_msi_irq' in file scope
drivers/pci/msi.c:69:25: warning: Calling weak function 'arch_teardown_msi_irq' in file scope
drivers/pci/msi.c:421:27: warning: Calling weak function 'arch_setup_msi_irqs' in file scope
drivers/pci/msi.c:492:27: warning: Calling weak function 'arch_setup_msi_irqs' in file scope
drivers/pci/msi.c:562:29: warning: Calling weak function 'arch_msi_check_device' in file scope
drivers/pci/msi.c:654:24: warning: Calling weak function 'arch_teardown_msi_irqs' in file scope
Is this patch a response to fix the sparse warning?
If so, can you include the offending sparse warning in the commit log?
> This is less source (by 1 line!), and results in smaller binaries too:
>
> text data bss dec hex filename
> 9354300 1693916 678424 11726640 b2ef30 build/powerpc/vmlinux-before
> 9354052 1693852 678424 11726328 b2edf8 build/powerpc/vmlinux-after
The fact that gcc isn't removing unused code sounds like a different bug
and not a kernel problem per se.
I don't object to this patch in general - it's the same result as far
as I'm concerned. I'd just like a consistent story on "weak symbols".
thanks,
grant
>
> Also smaller on x86_64 and arm (iop13xx).
>
> Signed-off-by: Michael Ellerman <michael@xxxxxxxxxxxxxx>
> ---
> arch/powerpc/include/asm/pci.h | 4 ++++
> arch/x86/include/asm/pci.h | 3 +++
> drivers/pci/msi.c | 26 +++++++++-----------------
> 3 files changed, 16 insertions(+), 17 deletions(-)
>
> This is the same patch I sent 20 minutes ago, but sent to the correct
> address for linux-pci.
>
> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
> index 3548159..ba17d5d 100644
> --- a/arch/powerpc/include/asm/pci.h
> +++ b/arch/powerpc/include/asm/pci.h
> @@ -114,6 +114,10 @@ extern int pci_domain_nr(struct pci_bus *bus);
> /* Decide whether to display the domain number in /proc */
> extern int pci_proc_domain(struct pci_bus *bus);
>
> +/* MSI arch hooks */
> +#define arch_setup_msi_irqs arch_setup_msi_irqs
> +#define arch_teardown_msi_irqs arch_teardown_msi_irqs
> +#define arch_msi_check_device arch_msi_check_device
>
> struct vm_area_struct;
> /* Map a range of PCI memory or I/O space for a device into user space */
> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
> index a977de2..a0301bf 100644
> --- a/arch/x86/include/asm/pci.h
> +++ b/arch/x86/include/asm/pci.h
> @@ -86,6 +86,9 @@ static inline void early_quirks(void) { }
>
> extern void pci_iommu_alloc(void);
>
> +/* MSI arch hook */
> +#define arch_setup_msi_irqs arch_setup_msi_irqs
> +
> #endif /* __KERNEL__ */
>
> #ifdef CONFIG_X86_32
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index b4a90ba..aa791db 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -27,20 +27,15 @@ static int pci_msi_enable = 1;
>
> /* Arch hooks */
>
> -int __attribute__ ((weak))
> -arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +#ifndef arch_msi_check_device
> +int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> {
> return 0;
> }
> +#endif
>
> -int __attribute__ ((weak))
> -arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *entry)
> -{
> - return 0;
> -}
> -
> -int __attribute__ ((weak))
> -arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> +#ifndef arch_setup_msi_irqs
> +int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> {
> struct msi_desc *entry;
> int ret;
> @@ -53,14 +48,10 @@ arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>
> return 0;
> }
> +#endif
>
> -void __attribute__ ((weak)) arch_teardown_msi_irq(unsigned int irq)
> -{
> - return;
> -}
> -
> -void __attribute__ ((weak))
> -arch_teardown_msi_irqs(struct pci_dev *dev)
> +#ifndef arch_teardown_msi_irqs
> +void arch_teardown_msi_irqs(struct pci_dev *dev)
> {
> struct msi_desc *entry;
>
> @@ -69,6 +60,7 @@ arch_teardown_msi_irqs(struct pci_dev *dev)
> arch_teardown_msi_irq(entry->irq);
> }
> }
> +#endif
>
> static void __msi_set_enable(struct pci_dev *dev, int pos, int enable)
> {
> --
> 1.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
