Hi Bjorn,
On 13/07/15 00:11, Bjorn Helgaas wrote:
> Weak header file declarations are error-prone because they make every
> definition weak, and the linker chooses one based on link order (see
> 10629d711ed7 ("PCI: Remove __weak annotation from pcibios_get_phb_of_node
> decl")).
>
> mips_cdmm_phys_base() is defined only in arch/mips/mti-malta/malta-memory.c
> so there's no problem with multiple definitions. But it works better to
> have a weak default implementation and allow a strong function to override
> it. Then we don't have to test whether a definition is present, and if
> there are ever multiple strong definitions, we get a link error instead of
> calling a random definition.
>
> Add a weak mips_cdmm_phys_base() definition and remove the weak annotation
> from the declaration in arch/mips/include/asm/cdmm.h.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> CC: James Hogan <james.hogan@xxxxxxxxxx>
Reviewed-by: James Hogan <james.hogan@xxxxxxxxxx>
Thanks for pointing out these problems. Hopefully eventually some of
these uses of weak symbols can be moved to device tree (its almost like
we need a DT binding for "this physical address isn't used for anything,
and would be suitable for an overlay").
Cheers
James
> ---
> arch/mips/include/asm/cdmm.h | 4 ++--
> drivers/bus/mips_cdmm.c | 14 +++++++++++++-
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/arch/mips/include/asm/cdmm.h b/arch/mips/include/asm/cdmm.h
> index 16e22ce..bece206 100644
> --- a/arch/mips/include/asm/cdmm.h
> +++ b/arch/mips/include/asm/cdmm.h
> @@ -53,7 +53,7 @@ struct mips_cdmm_driver {
> * mips_cdmm_phys_base() - Choose a physical base address for CDMM region.
> *
> * Picking a suitable physical address at which to map the CDMM region is
> - * platform specific, so this weak function can be defined by platform code to
> + * platform specific, so this function can be defined by platform code to
> * pick a suitable value if none is configured by the bootloader.
> *
> * This address must be 32kB aligned, and the region occupies a maximum of 32kB
> @@ -61,7 +61,7 @@ struct mips_cdmm_driver {
> *
> * Returns: Physical base address for CDMM region, or 0 on failure.
> */
> -phys_addr_t __weak mips_cdmm_phys_base(void);
> +phys_addr_t mips_cdmm_phys_base(void);
>
> extern struct bus_type mips_cdmm_bustype;
> void __iomem *mips_cdmm_early_probe(unsigned int dev_type);
> diff --git a/drivers/bus/mips_cdmm.c b/drivers/bus/mips_cdmm.c
> index ab3bde1..1c543ef 100644
> --- a/drivers/bus/mips_cdmm.c
> +++ b/drivers/bus/mips_cdmm.c
> @@ -332,6 +332,18 @@ static phys_addr_t mips_cdmm_cur_base(void)
> }
>
> /**
> + * mips_cdmm_phys_base() - Choose a physical base address for CDMM region.
> + *
> + * Picking a suitable physical address at which to map the CDMM region is
> + * platform specific, so this weak function can be overridden by platform
> + * code to pick a suitable value if none is configured by the bootloader.
> + */
> +phys_addr_t __weak mips_cdmm_phys_base(void)
> +{
> + return 0;
> +}
> +
> +/**
> * mips_cdmm_setup() - Ensure the CDMM bus is initialised and usable.
> * @bus: Pointer to bus information for current CPU.
> * IS_ERR(bus) is checked, so no need for caller to check.
> @@ -368,7 +380,7 @@ static int mips_cdmm_setup(struct mips_cdmm_bus *bus)
> if (!bus->phys)
> bus->phys = mips_cdmm_cur_base();
> /* Otherwise, ask platform code for suggestions */
> - if (!bus->phys && mips_cdmm_phys_base)
> + if (!bus->phys)
> bus->phys = mips_cdmm_phys_base();
> /* Otherwise, copy what other CPUs have done */
> if (!bus->phys)
>
Attachment:
signature.asc
Description: OpenPGP digital signature
