From: Takashi Iwai on
At Wed, 7 Apr 2010 11:55:19 -0400 (EDT),
Alan Stern wrote:
>
> On Wed, 7 Apr 2010, Greg KH wrote:
>
> > Yeah, I really don't want to have to change every driver in different
> > ways just depending on if someone thinks it is going to need to run on
> > this wierd hardware.
>
> It's not weird hardware, as far as I know. It's just a 64-bit system
> with a 32-bit USB host controller.
>
> (And remember, while there are 64-bit EHCI controllers, there are not
> any 64-bit OHCI or UHCI controllers. So whenever somebody plugs a
> full-speed or low-speed device into a 64-bit machine, they will face
> this problem. It's like the old problem of ISA devices that could
> only do DMA to addresses in the first 16 MB of memory -- what the
> original GFP_DMA flag was intended for.)
>
> > Alan, any objection to just using usb_buffer_alloc() for every driver?
> > Or is that too much overhead?
>
> I don't know what the overhead is. But usb_buffer_alloc() requires the
> caller to keep track of the buffer's DMA address, so it's not a simple
> plug-in replacement. In addition, the consistent memory that
> usb_buffer_alloc() provides is a scarce resource on some platforms.

Yeah, also the area is aligned to kernel pages, and it may be much
bigger than the requested (power-of-two). If not needed, we should
avoid it.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Takashi Iwai on
At Wed, 7 Apr 2010 19:59:35 +0200,
Daniel Mack wrote:
>
> On Wed, Apr 07, 2010 at 07:55:20PM +0200, Takashi Iwai wrote:
> > At Wed, 7 Apr 2010 18:16:03 +0200,
> > Daniel Mack wrote:
> > >
> > > On Wed, Apr 07, 2010 at 11:55:19AM -0400, Alan Stern wrote:
> > > > On Wed, 7 Apr 2010, Greg KH wrote:
> > > >
> > > > > Alan, any objection to just using usb_buffer_alloc() for every driver?
> > > > > Or is that too much overhead?
> > > >
> > > > I don't know what the overhead is. But usb_buffer_alloc() requires the
> > > > caller to keep track of the buffer's DMA address, so it's not a simple
> > > > plug-in replacement. In addition, the consistent memory that
> > > > usb_buffer_alloc() provides is a scarce resource on some platforms.
> > > >
> > > > Writing new functions is the way to go.
> > >
> > > Ok, I'll write some dummies for usb_malloc() and usb_zalloc() which
> > > will just call kmalloc() with GFP_DMA32 for now.
> >
> > Can't we provide only zalloc() variant? Zero'ing doesn't cost much,
> > and the buffer allocation shouldn't be called too often.
> >
> > > And while at it,
> > > usb_alloc_buffer() will be renamed to usb_alloc_consistent().
> >
> > Most of recent functions are named with "coherent".
>
> I agree to both points, will do so unless anyone has a harsh opinion
> about that.
>
> Another thing: I guess we don't need a corresponding free() function
> that just calls kfree(), right? Or should we introduce it now to be
> flexible for future extensions?

Well, I would implement the corresponding free. It could be simply
a macro calling kfree(), but it's saner to provide it for API
uniformity, IMO.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on
On Wed, 7 Apr 2010, Takashi Iwai wrote:

> > Ok, I'll write some dummies for usb_malloc() and usb_zalloc() which
> > will just call kmalloc() with GFP_DMA32 for now.
>
> Can't we provide only zalloc() variant? Zero'ing doesn't cost much,
> and the buffer allocation shouldn't be called too often.

Linus specifically requested us to avoid using kzalloc in usbfs. I
can't find the message in the email archives, but Greg KH should be
able to confirm it.

As long as we're imitating kmalloc for one use, we might as well make
it available to all.

> > And while at it,
> > usb_alloc_buffer() will be renamed to usb_alloc_consistent().
>
> Most of recent functions are named with "coherent".

Yes, the terminology got a little confused between the PCI and DMA
realms. I agree, "coherent" is better.

BTW, although some EHCI controllers may support 64-bit DMA, the driver
contains this:

if (HCC_64BIT_ADDR(hcc_params)) {
ehci_writel(ehci, 0, &ehci->regs->segment);
#if 0
// this is deeply broken on almost all architectures
if (!dma_set_mask(hcd->self.controller, DMA_BIT_MASK(64)))
ehci_info(ehci, "enabled 64bit DMA\n");
#endif
}

I don't know if the comment is still true, but until the "#if 0" is
removed, ehci-hcd won't make use of 64-bit DMA.

Note also that just before the lines above, ehci-hcd.c contains this
comment:

* pci_pool consistent memory always uses segment zero.

(Nowadays the driver uses dma_pool, not pci_pool.) As far as I can
tell, this comment isn't true on 64-bit systems, but nevertheless,
ehci-hcd relies on it. Of course it _is_ true for devices whose DMA
mask indicates they can't use memory above 4 GB -- but if ehci-hcd is
changed to enlarge the DMA mask then how do we force dma_pool memory to
lie below 4 GB?

[Actually it isn't _really_ necessary for the dma_pool to lie below 4
GB. But ehci-hcd allocates several pools, and it _is_ necessary for
all of them to lie in the _same_ 4 GB segment. The easiest way to do
that is to put them all in segment 0.]

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Robert Hancock on
On 04/07/2010 08:59 AM, Alan Stern wrote:
> On Wed, 7 Apr 2010, Daniel Mack wrote:
>
>> Hi,
>>
>> I was pointed to https://bugzilla.kernel.org/show_bug.cgi?id=15580 by
>> Pedro Ribeiro, and unfortunately I'm pretty late in the game. I wasn't
>> aware of that thread until yesterday.
>>
>> While the report is quite confusing, the reason seams pretty clear to me
>> as I've been thru quite some time-demanding debugging of a very similar
>> issue on Mac OS X. But I'm not totally sure whether we really hit the
>> same issue here, so I'd like to have your opinions first.
>>
>> The problem is appearantly the way the transfer buffer is allocated in
>> the drivers. In the snd-usb-caiaq driver, I used kzalloc() to get memory
>> which works fine on 32bit systems. On x86_64, however, it seems that
>> kzalloc() hands out memory beyond the 32bit addressable boundary, which
>> the DMA controller of the 32bit PCI-connected EHCI controller is unable
>> to write to or read from. Am I correct on this conclusion?
>
> That seems like the right answer. You are correct that an EHCI
> controller capable only of 32-bit memory accesses would not be able to
> use a buffer above the 4 GB line.
>
>> Depending on the condition of the memory management, things might work
>> or not, and especially right after a reboot, there's a better chance to
>> get lower memory.
>>
>> The fix is to use usb_buffer_alloc() for that purpose which ensures
>> memory that is suitable for DMA. And on x86_64, this also means that the
>> upper 32 bits of the address returned are all 0's.
>
> That is not a good fix. usb_buffer_alloc() provides coherent memory,
> which is not what we want. I believe the correct fix is to specify the
> GFP_DMA32 flag in the kzalloc() call.
>
> Of course, some EHCI hardware _is_ capable of using 64-bit addresses.
> But not all, and other controller types aren't. In principle we could
> create a new allocation routine, which would take a pointer to the USB
> bus as an additional argument and use it to decide whether the memory
> needs to lie below 4 GB. I'm not sure adding this extra complexity
> would be worthwhile.

AFAIK, the driver shouldn't have to worry about this at all. When the
buffer gets DMA-mapped for the controller, the DMA mapping code should
see that the device has a 32-bit DMA mask and either bounce or IOMMU-map
the memory so that it appears below 4GB.

Not to say that something might not be sabotaging this somehow, but this
complexity really shouldn't be needed.

>
>> If what I've stated is true, there are quite some more drivers affected
>> by this issue.
>
> Practically every USB driver, I should think. And maybe a lot of
> non-USB drivers too.
>
>> I collected a list of places where similar fixes are
>> needed, and I can send patches if I get a thumbs-up.
>>
>> Pedro is currently testing a patch I sent out yesterday.
>
> Good work -- it certainly would have taken me a long time to figure
> this out.
>
> Alan Stern
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Robert Hancock on
On 04/07/2010 01:13 PM, Alan Stern wrote:
> On Wed, 7 Apr 2010, Takashi Iwai wrote:
>
>>> Ok, I'll write some dummies for usb_malloc() and usb_zalloc() which
>>> will just call kmalloc() with GFP_DMA32 for now.
>>
>> Can't we provide only zalloc() variant? Zero'ing doesn't cost much,
>> and the buffer allocation shouldn't be called too often.
>
> Linus specifically requested us to avoid using kzalloc in usbfs. I
> can't find the message in the email archives, but Greg KH should be
> able to confirm it.
>
> As long as we're imitating kmalloc for one use, we might as well make
> it available to all.
>
>>> And while at it,
>>> usb_alloc_buffer() will be renamed to usb_alloc_consistent().
>>
>> Most of recent functions are named with "coherent".
>
> Yes, the terminology got a little confused between the PCI and DMA
> realms. I agree, "coherent" is better.
>
> BTW, although some EHCI controllers may support 64-bit DMA, the driver
> contains this:
>
> if (HCC_64BIT_ADDR(hcc_params)) {
> ehci_writel(ehci, 0,&ehci->regs->segment);
> #if 0
> // this is deeply broken on almost all architectures
> if (!dma_set_mask(hcd->self.controller, DMA_BIT_MASK(64)))
> ehci_info(ehci, "enabled 64bit DMA\n");
> #endif
> }
>
> I don't know if the comment is still true, but until the "#if 0" is
> removed, ehci-hcd won't make use of 64-bit DMA.

The comment is wrong (or at least outdated or based on an incorrect
assumption), but you're right, currently 64-bit DMA is not used on any
EHCI controllers. It could be, but it sounded like the consensus was it
wasn't worth the risk. Apparently Windows 7 started using it, and then
had to put out a patch because some NVidia EHCI controllers indicated
64-bit support but it didn't work properly. So you'd have to blacklist
those controllers, at least.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/