Hi Fredrik,

On 16.05.2019 18:15, Fredrik Noring wrote:
> Hi Laurentiu,
> 
>> I took your code, added the missing mapping and placed it in a patch.
>> Please see attached (compile tested only).
> 
> Thanks! Unfortunately, the OHCI fails with errors such as
> 
> 	usb 1-1: device descriptor read/64, error -12

Alright, at least the crash went away. :-)

> that I tracked down to the calls
> 
> 	   hub_port_init
> 	-> usb_control_msg
> 	-> usb_internal_control_msg
> 	-> usb_start_wait_urb
> 	-> usb_submit_urb
> 	-> usb_hcd_submit_urb
> 	-> hcd->driver->urb_enqueue
> 	-> ohci_urb_enqueue
> 	-> ed_get
> 	-> ed_alloc
> 	-> dma_pool_zalloc
> 	-> dma_pool_alloc
> 	-> pool_alloc_page,
> 
> which returns NULL. Then I noticed
> 
> 	/* pool_alloc_page() might sleep, so temporarily drop &pool->lock */
> that might be a problem considering that the HCD handles pool memory in
> IRQ handlers, for instance:
> 
> 	   do_IRQ
> 	-> generic_handle_irq
> 	-> handle_level_irq
> 	-> handle_irq_event
> 	-> handle_irq_event_percpu
> 	-> __handle_irq_event_percpu
> 	-> usb_hcd_irq
> 	-> ohci_irq
> 	-> ohci_work
> 	-> finish_urb
> 	-> __usb_hcd_giveback_urb
> 	-> usb_hcd_unmap_urb_for_dma
> 	-> hcd_buffer_free


That looks indeed worrisome but I'd expect that it's not related to 
these genalloc changes that I'm working on. I'll try to look into it but 
as I'm not familiar with the area maybe others will comment.

> Also, DMA_BUFFER_SIZE in ohci-ps2.c is only 256 KiB in total. Is the new
> pool implementation at least as efficient as the previous one?

I don't see any obvious reasons for genalloc to be less efficient.

One thing I noticed between genalloc and the original implementation is 
that previously the device memory was mapped with memremap() while with 
genalloc I'm doing a devm_ioremap(). I can't think of a relevant 
difference except that memremap() maps with WC buth maybe there are 
other arch specific subtleties. I've attached a new version of the 
ohci-ps2 patch replacing the devm_ioremap() with devm_memremap() to 
better match the original implementation. Please test if you find some time.

---
Thanks & Best Regards, Laurentiu