I've been playing with randconfig builds and found an interesting problem
with the combination of:
CONFIG_ARM_PATCH_PHYS_VIRT=y
CONFIG_HOTPLUG=n
CONFIG_DMABOUNCE=n
CONFIG_MMC_SPI=y
The problem is shown with this code:
static int __devexit mmc_spi_remove(struct spi_device *spi)
{
...
dma_unmap_single(host->dma_dev, host->ones_dma,
MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
dma_unmap_single(host->dma_dev, host->data_dma,
sizeof(*host->data), DMA_BIDIRECTIONAL);
...
}
and the error message (in case someone looks for this using google) is
`.devexit.text' referenced in section `.pv_table' of drivers/built-in.o: defined in discarded section `.devexit.text' of drivers/built-in.o
`.devexit.text' referenced in section `.pv_table' of drivers/built-in.o: defined in discarded section `.devexit.text' of drivers/built-in.o
What happens is that dma_unmap_single() calls
__dma_single_dev_to_cpu(dma_to_virt(dev, handle), size, dir), which requires
patching in the caller. However, due to CONFIG_HOTPLUG being disabled, the
__devexit section gets discarded, and the linker cannot create an entry in the
.pvtable section for the mmc_spi_remove function.
I don't know if the same problem exists in other places in the code, but it's
entirely possible. I also couldn't think of a good solution for this, short of
moving the definition of dma_unmap_single() to out of line code.
Arnd
On Wed, 8 Jun 2011, Arnd Bergmann wrote:
> I've been playing with randconfig builds and found an interesting problem
> with the combination of:
>
> CONFIG_ARM_PATCH_PHYS_VIRT=y
> CONFIG_HOTPLUG=n
> CONFIG_DMABOUNCE=n
> CONFIG_MMC_SPI=y
>
> The problem is shown with this code:
>
> static int __devexit mmc_spi_remove(struct spi_device *spi)
> {
> ...
> dma_unmap_single(host->dma_dev, host->ones_dma,
> MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
> dma_unmap_single(host->dma_dev, host->data_dma,
> sizeof(*host->data), DMA_BIDIRECTIONAL);
> ...
> }
>
> and the error message (in case someone looks for this using google) is
>
> `.devexit.text' referenced in section `.pv_table' of drivers/built-in.o: defined in discarded section `.devexit.text' of drivers/built-in.o
> `.devexit.text' referenced in section `.pv_table' of drivers/built-in.o: defined in discarded section `.devexit.text' of drivers/built-in.o
>
> What happens is that dma_unmap_single() calls
> __dma_single_dev_to_cpu(dma_to_virt(dev, handle), size, dir), which requires
> patching in the caller. However, due to CONFIG_HOTPLUG being disabled, the
> __devexit section gets discarded, and the linker cannot create an entry in the
> .pvtable section for the mmc_spi_remove function.
We really need to push for better toolchain support here. This could
easily be solved with some way to query the currently active section
from the assembler and use that to construct the argument to the
.pushsection directive. This way, the .pvtable section could become
.pvtable.__devexit when the reference is made from a __devexit section,
and that could be discarded altogether at link time if unneeded.
Dave Martin proposed an extension to gas here (also sent to
[email protected]):
http://article.gmane.org/gmane.linux.linaro.toolchain/701
But as far as I know, nothing further happened. And this is not an ARM
specific issue either as the X86 alternates have the same problem.
> I don't know if the same problem exists in other places in the code, but it's
> entirely possible. I also couldn't think of a good solution for this, short of
> moving the definition of dma_unmap_single() to out of line code.
Probably the best interim solution would be:
diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index dfbb377..f231c92 100644
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -21,7 +21,8 @@
#define ARM_CPU_KEEP(x)
#endif
-#if defined(CONFIG_SMP_ON_UP) && !defined(CONFIG_DEBUG_SPINLOCK)
+#if (defined(CONFIG_SMP_ON_UP) && !defined(CONFIG_DEBUG_SPINLOCK)) || \
+ defined(CONFIG_ARM_PATCH_PHYS_VIRT)
#define ARM_EXIT_KEEP(x) x
#else
#define ARM_EXIT_KEEP(x)
But clearly the toolchain should be more accommodating instead.
Nicolas
On Wednesday 08 June 2011, Nicolas Pitre wrote:
> Dave Martin proposed an extension to gas here (also sent to
> [email protected]):
>
> http://article.gmane.org/gmane.linux.linaro.toolchain/701
>
> But as far as I know, nothing further happened. And this is not an ARM
> specific issue either as the X86 alternates have the same problem.
Ah, yes. That would be really nice to have. Dave, do you still have hopes
that this patch gets merged?
> > I don't know if the same problem exists in other places in the code, but it's
> > entirely possible. I also couldn't think of a good solution for this, short of
> > moving the definition of dma_unmap_single() to out of line code.
>
> Probably the best interim solution would be:
>
> diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
> index dfbb377..f231c92 100644
> --- a/arch/arm/kernel/vmlinux.lds.S
> +++ b/arch/arm/kernel/vmlinux.lds.S
> @@ -21,7 +21,8 @@
> #define ARM_CPU_KEEP(x)
> #endif
>
> -#if defined(CONFIG_SMP_ON_UP) && !defined(CONFIG_DEBUG_SPINLOCK)
> +#if (defined(CONFIG_SMP_ON_UP) && !defined(CONFIG_DEBUG_SPINLOCK)) || \
> + defined(CONFIG_ARM_PATCH_PHYS_VIRT)
> #define ARM_EXIT_KEEP(x) x
> #else
> #define ARM_EXIT_KEEP(x)
>
> But clearly the toolchain should be more accommodating instead.
>
10000 randconfig kernel build later without that patch, I'm rather sure
that there is only a single driver that is suffering from this. While your
patch absolutely makes sense, a less invasive workaround would be to just
not mark mmc_spi_remove as __devexit until Dave's patch gets in.
Arnd
On Thursday 16 June 2011, Arnd Bergmann wrote:
> 10000 randconfig kernel build later without that patch, I'm rather sure
> that there is only a single driver that is suffering from this. While your
> patch absolutely makes sense, a less invasive workaround would be to just
> not mark mmc_spi_remove as __devexit until Dave's patch gets in.
Scratch that. Even when Dave's patch gets into binutils, we'd still need to
make a compile time decision to figure out if the linker is new enough, so
we definitely need Nicolas' patch as well.
Arnd
On Thu, Jun 16, 2011 at 05:25:03PM +0200, Arnd Bergmann wrote:
> On Thursday 16 June 2011, Arnd Bergmann wrote:
> > 10000 randconfig kernel build later without that patch, I'm rather sure
> > that there is only a single driver that is suffering from this. While your
> > patch absolutely makes sense, a less invasive workaround would be to just
> > not mark mmc_spi_remove as __devexit until Dave's patch gets in.
>
> Scratch that. Even when Dave's patch gets into binutils, we'd still need to
> make a compile time decision to figure out if the linker is new enough, so
> we definitely need Nicolas' patch as well.
>
> Arnd
It would be nice to get it merged for the longer term, but we'll have
quite a long transitional period while the change propagates.
So we'd still need some other workaround for now.
Cheers
---Dave
On Thu, 16 Jun 2011, Dave Martin wrote:
> On Thu, Jun 16, 2011 at 05:25:03PM +0200, Arnd Bergmann wrote:
> > On Thursday 16 June 2011, Arnd Bergmann wrote:
> > > 10000 randconfig kernel build later without that patch, I'm rather sure
> > > that there is only a single driver that is suffering from this. While your
> > > patch absolutely makes sense, a less invasive workaround would be to just
> > > not mark mmc_spi_remove as __devexit until Dave's patch gets in.
> >
> > Scratch that. Even when Dave's patch gets into binutils, we'd still need to
> > make a compile time decision to figure out if the linker is new enough, so
> > we definitely need Nicolas' patch as well.
> >
> > Arnd
>
> It would be nice to get it merged for the longer term, but we'll have
> quite a long transitional period while the change propagates.
That is assuming such a change gets merged to start with.
Nicolas