Re: [PATCH 8/9] MIPS: Remove "weak" from mips_cdmm_phys_base() declaration

[James Hogan]

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