On 28.4.2022 6.03, Jung Daehwan wrote: > On Wed, Apr 27, 2022 at 07:25:21PM +0300, Mathias Nyman wrote: >> On 26.4.2022 12.18, Daehwan Jung wrote: >>> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat >>> driver mainly and extends some functions by xhci hooks and overrides. >>> >>> It supports USB Audio offload with Co-processor. It only cares DCBAA, >>> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated >>> on specific address with xhci hooks. Co-processor could use them directly >>> without xhci driver after then. >>> >>> Signed-off-by: Daehwan Jung <dh10.jung@xxxxxxxxxxx> >> >> I have to agree with Krzysztof's comments, this is an odd driver stub. >> >> Perhaps open up a bit how the Exynos offloading works so we can figure out >> in more detail what the hardware needs from software. >> >> (...) > >>> +static int xhci_alloc_segments_for_ring_uram(struct xhci_hcd *xhci, >>> + struct xhci_segment **first, struct xhci_segment **last, >>> + unsigned int num_segs, unsigned int cycle_state, >>> + enum xhci_ring_type type, unsigned int max_packet, gfp_t flags, >>> + u32 endpoint_type) >>> +{ >>> + struct xhci_segment *prev; >>> + bool chain_links = false; >>> + >>> + while (num_segs > 0) { >>> + struct xhci_segment *next = NULL; >>> + >>> + if (!next) { >>> + prev = *first; >>> + while (prev) { >>> + next = prev->next; >>> + xhci_segment_free(xhci, prev); >>> + prev = next; >>> + } >>> + return -ENOMEM; >> >> This always return -ENOMEM > > Yes. it's right to return error here. > Still don't think that is the case. So if the num_segs value passed to a function named xhci_alloc_segments_for_ring_uram() is anything else than 0, it will automatically return -ENOMEM? >> >> Also this whole function never allocates or remaps any memory. > > This fuctions is for link segments. Right below function(xhci_ring_alloc_uram) > allocates. Still doesn't allocate any ring segments. Below function only allocates memory for the ring structure that contains pointers to segments. > >> >>> + } >>> + xhci_link_segments(prev, next, type, chain_links); >>> + >>> + prev = next; >>> + num_segs--; >>> + } >>> + xhci_link_segments(prev, *first, type, chain_links); >>> + *last = prev; >>> + >>> + return 0; >>> +} >>> + >>> +static struct xhci_ring *xhci_ring_alloc_uram(struct xhci_hcd *xhci, >>> + unsigned int num_segs, unsigned int cycle_state, >>> + enum xhci_ring_type type, unsigned int max_packet, gfp_t flags, >>> + u32 endpoint_type) >>> +{ >>> + struct xhci_ring *ring; >>> + int ret; >>> + struct device *dev = xhci_to_hcd(xhci)->self.sysdev; >>> + >>> + ring = kzalloc_node(sizeof(*ring), flags, dev_to_node(dev)); >>> + if (!ring) >>> + return NULL; >>> + >>> + ring->num_segs = num_segs; >>> + ring->bounce_buf_len = max_packet; >>> + INIT_LIST_HEAD(&ring->td_list); >>> + ring->type = type; >>> + if (num_segs == 0) >>> + return ring; >>> + >>> + ret = xhci_alloc_segments_for_ring_uram(xhci, &ring->first_seg, >>> + &ring->last_seg, num_segs, cycle_state, type, >>> + max_packet, flags, endpoint_type); >>> + if (ret) >>> + goto fail; >>> + >>> + /* Only event ring does not use link TRB */ >>> + if (type != TYPE_EVENT) { >>> + /* See section 4.9.2.1 and 6.4.4.1 */ >>> + ring->last_seg->trbs[TRBS_PER_SEGMENT - 1].link.control |= >>> + cpu_to_le32(LINK_TOGGLE); >> >> No memory was allocated for trbs > > Allcation function for trbs are missed. It's done by ioremap. > I will add it on next submission. Thanks for the comment. > >> >> A lot of this code seems to exists just to avoid xhci driver from allocating >> dma capable memory, we can refactor the existing xhci_mem_init() and move >> dcbaa and event ring allocation and other code to their own overridable >> functions. >> >> This way we can probably get rid of a lot of the code in this series. > > Yes right. I think it's proper. Do you agree with it or have better way > to do it? Could be, but I don't have a good picture of how this Exynos audio offloading works, so it's hard to guess. Thanks -Mathias