On Mon, Feb 18 2008 at 14:57 +0200, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Sun, 17 Feb 2008 18:46:03 +0200 Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
>
>> ...
>>
>> All my testers have reported back that with these 5 patches applied they can
>> now run with a 2.6.24 kernel the same way they ran before. However there is
>> that reported issue, with the dma_free_coherent WARN_ON (above). The code was
>> like that from day one and it is a very old issue, however it is a regression
>> because 2.6.24 introduced that new WARN_ON.
>> (infamous commit aa24886e379d2b641c5117e178b15ce1d5d366ba)
>> >From posts on lkml and even recent one in linux-scsi about the arcmsr driver
>> it looks that all a driver can do is work around it with different kernel mechanisms
>> and driver rewrites. I'm afraid I need your help here. I'm not sure I understand
>> why does the gdth driver uses the pci_{alloc,free}_consistent() API's, and what
>> is needed to replace it. Could you please have a look in gdth_proc.c and also in
>> gdth.c for all the places that call gdth_ioctl_alloc/gdth_ioctl_free, and advise
>> what can I do in it's place. Please bear in mind that we need it for 2.6.24, as
>> a bugfix.
>>
>> Apart from the above issue, please accept patches 3,4,5 above they have now
>> been tested and are reported to bring broken system back to production.
>> (Given that you approve off course). And mark them for inclusion to the
>> 2.6.24 stable releases. (Or is there some thing that I should do)
>>
>> ---
>> Meanwhile on x86 systems I understand the WARN_ON is cosmetic, and does not
>> pose any harm. Some people have reported stability with temporarily disabling
>> it. For testers that want to try, here it is below. At your own risk.
>>
>> ---
>> >From 50d3657bf6a138ee63ad1ce00052380edc75ace7 Mon Sep 17 00:00:00 2001
>> From: Boaz Harrosh <bharrosh@xxxxxxxxxxx>
>> Date: Sun, 17 Feb 2008 12:49:35 +0200
>> Subject: [PATCH] gdth: Hack to remove WARN_ON in arch/x86/kernel/pci-dma_32.c
>>
>> gdth uses dma_free_coherent() with interrupts disabled. Which
>> is not portable, but is safe on the HW that supports gdth.
>>
>> NOT Signed-off-by: Boaz Harrosh <bharrosh@xxxxxxxxxxx>
>> ---
>> arch/x86/kernel/pci-dma_32.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kernel/pci-dma_32.c b/arch/x86/kernel/pci-dma_32.c
>> index 5133032..350dcfd 100644
>> --- a/arch/x86/kernel/pci-dma_32.c
>> +++ b/arch/x86/kernel/pci-dma_32.c
>> @@ -63,7 +63,7 @@ void dma_free_coherent(struct device *dev, size_t size,
>> struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
>> int order = get_order(size);
>>
>> - WARN_ON(irqs_disabled()); /* for portability */
>> +/* WARN_ON(irqs_disabled());*/ /* for portability */
>> if (mem && vaddr >= mem->virt_base && vaddr < (mem->virt_base + (mem->size << PAGE_SHIFT))) {
>> int page = (vaddr - mem->virt_base) >> PAGE_SHIFT;
>>
>
> Yes. Let's reprise aa24886e379d2b641c5117e178b15ce1d5d366ba:
>
> : commit aa24886e379d2b641c5117e178b15ce1d5d366ba
> : Author: David Brownell <david-b@xxxxxxxxxxx>
> : Date: Fri Aug 10 13:10:27 2007 -0700
> :
> : dma_free_coherent() needs irqs enabled (sigh)
> :
> : On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish
> : call context requirement: unlike its dma_alloc_coherent() sibling, it may
> : not be called with IRQs disabled. (This was new behavior on ARM as of late
> : 2005, caused by ARM SMP updates.) This little surprise can be annoyingly
> : driver-visible.
> :
> : Since it looks like that restriction won't be removed, this patch changes
> : the definition of the API to include that requirement. Also, to help catch
> : nonportable drivers, it updates the x86 and swiotlb versions to include the
> : relevant warnings. (I already observed that it trips on the
> : bus_reset_tasklet of the new firewire_ohci driver.)
> :
>
> In general, all Linux memory-freeing functions can be called from all
> contexts. (vfree is an irritating exception). This is good, and provides
> maximum usefulness to callees, as all utility functions should seek to do.
> It would be best to fix arm and mips.
>
> But arm and mips require enabled local irqs because their
> dma_free_coherent() needs to do a cross-cpu IPI call. Presumably because
> of certain unusual TLB protocols.
>
> I'm not sure what we should do about this. Presumably the gdth-on-arm
> usage base is, umm, zero, so we could lamely add
> CONFIG_DMA_FREE_COHERENT_WITH_LOCAL_IRQS_DISABLED_IS_OK and then use that
> to disable gdth (and similar) on arm amd mips. But ugh.
>
> Russell, Ralf: is there something we can do here to relax this requirement?
>
> I'm thinking that perhaps we can do some rcu/refcounting tricks: launch the
> IPI from within dma_free_coherent(), but don't wait for it to complete.
> When all CPUs have handled the IPI then (and only then) the virtual address
> becomes recyclable, or something like that?
>
> <double-checks>
>
> Actually I think David might have been wrong about mips. afaict its
> dma_free_coherent() is callable under local_irq_disable(), so ARM SMP is
> the sole exception?
>
>
Thank you Andrew for your reply.
But actually James has resolved this issue for the gdth driver. And all testers
that witnessed a problem have reported success with his simple fix, which is for
sure a much more correct way then was done before.
So for gdth sake I is OK as it is, so far.
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
