I'll sort out the issues and resubmit. Perhaps from a different email client. Thanks! John > -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Tuesday, July 14, 2009 12:15 PM > To: John Youn > Cc: linux-usb@xxxxxxxxxxxxxxx > Subject: Re: [RFC] Scratchpad buffer allocation > > Hi John, > > Looks good, aside from a couple small things. See my inline comments. > > Your patch included all spaces instead of tabs; maybe your mail client > mangled it? I've had trouble with Outlook 2007 mail servers converting > all tabs to spaces. That's why I use my linux.intel.com address to > send > and receive patches. > You should probably run scripts/checkpatch.pl over this patch too. > > You might want to elaborate on what a scatchpad buffer array is in the > commit message for those people that don't have the xHCI spec. It's > basically just an array of 64-bit pointers to chunks of memory that the > xHCI hardware can use "as a scratchpad", correct? > > > On Mon, Jul 13, 2009 at 02:10:10PM -0700, John Youn wrote: > > Allocation and initialization of scratchpad buffer array. > > XHCI 4.20. > > > > Signed-off-by: John Youn <johnyoun@xxxxxxxxxxxx> > > --- > > > > drivers/usb/host/xhci-mem.c | 93 > +++++++++++++++++++++++++++++++++++++++++++ > > drivers/usb/host/xhci.h | 11 +++++ > > 2 files changed, 104 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci- > mem.c > > index d90baaa..88d5794 100644 > > --- a/drivers/usb/host/xhci-mem.c > > +++ b/drivers/usb/host/xhci-mem.c > > @@ -544,6 +544,93 @@ void xhci_endpoint_zero(struct xhci_hcd *xhci, > > */ > > } > > > > + > > +/* Set up the scratchpad buffer array and scratchpad buffers, if > needed. */ > > +static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags) > > +{ > > + int i; > > + struct device *dev = xhci_to_hcd(xhci)->self.controller; > > + int num_sp = HCS_MAX_SCRATCHPAD(xhci->hcs_params2); > > + > > + xhci_dbg(xhci, "Allocating %d scratchpad buffers\n", num_sp); > > + > > + if (!num_sp) > > + return 0; > > + > > + xhci->scratchpad = kzalloc(sizeof(*xhci->scratchpad), flags); > > + if (!xhci->scratchpad) > > + goto fail_sp; > > + > > + xhci->scratchpad->sp_array = > > + pci_alloc_consistent(to_pci_dev(dev), > > + num_sp * sizeof(u64), > > + &xhci->scratchpad->sp_dma); > > + if (!xhci->scratchpad->sp_array) > > + goto fail_sp2; > > + > > + xhci->scratchpad->sp_buffers = kzalloc(sizeof(void *) * > num_sp, flags); > > + if (!xhci->scratchpad->sp_buffers) > > + goto fail_sp3; > > + > > + xhci->dcbaa->dev_context_ptrs[0] = xhci->scratchpad->sp_dma; > > + for (i = 0; i < num_sp; i++) { > > + dma_addr_t dma; > > + void *buf = pci_alloc_consistent(to_pci_dev(dev), > > + PAGE_SIZE, &dma); > > PAGE_SIZE should be xhci->page_size instead. We want the internal xHCI > page size to always be 4K, but the driver needs to run on systems that > have a bigger page size. (Yes, xHCI implementations can have a bigger > page size, but the driver doesn't support a page size that's bigger > than > 4KB right now.) > > > + if (!buf) > > + goto fail_sp4; > > + > > + xhci->scratchpad->sp_array[i] = dma; > > + xhci->scratchpad->sp_buffers[i] = buf; > > + } > > + > > + return 0; > > + > > + fail_sp4: > > + for (i = i - 1; i >= 0; i --) { > > checkpatch error: > ERROR: space prohibited before that '--' (ctx:WxB) > #62: FILE: drivers/usb/host/xhci-mem.c:590: > + for (i = i - 1; i >= 0; i --) { > ^ > > > + pci_free_consistent(to_pci_dev(dev), PAGE_SIZE, > > + xhci->scratchpad->sp_buffers[i], > > + xhci->scratchpad->sp_array[i]); > > Use xhci->page_size. > > > + } > > + kfree(xhci->scratchpad->sp_buffers); > > + > > + fail_sp3: > > + pci_free_consistent(to_pci_dev(dev), num_sp * sizeof(u64), > > + xhci->scratchpad->sp_array, > > + xhci->scratchpad->sp_dma); > > + > > + fail_sp2: > > + kfree(xhci->scratchpad); > > + xhci->scratchpad = NULL; > > + > > + fail_sp: > > + return -1; > > You should return -ENOMEM here instead of -1. It's not a bug, it's > just > common practice. > > > +} > > + > > +static void scratchpad_free(struct xhci_hcd *xhci) > > +{ > > + int num_sp; > > + int i; > > + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)- > >self.controller); > > + > > + if (!xhci->scratchpad) > > + return; > > + > > + num_sp = HCS_MAX_SCRATCHPAD(xhci->hcs_params2); > > + > > + for (i = 0; i < num_sp; i++) { > > + pci_free_consistent(pdev, PAGE_SIZE, > > Use xhci->page_size. > > > + xhci->scratchpad->sp_buffers[i], > > + xhci->scratchpad->sp_array[i]); > > So if the xHCI hardware has barfed into the DMA pointers in the > scratchpad context array, you're just going to use those addresses to > free memory? You might want to keep an array of buffer DMA pointers in > struct xhci_scratchpad. It's not pretty, but I'm not sure how much I > trust the hardware yet. ;) > > > + } > > + kfree(xhci->scratchpad->sp_buffers); > > + pci_free_consistent(pdev, num_sp * sizeof(u64), > > + xhci->scratchpad->sp_array, > > + xhci->scratchpad->sp_dma); > > + kfree(xhci->scratchpad); > > + xhci->scratchpad = NULL; > > +} > > + > > void xhci_mem_cleanup(struct xhci_hcd *xhci) > > { > > struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)- > >self.controller); > > @@ -592,6 +679,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci) > > > > xhci->page_size = 0; > > xhci->page_shift = 0; > > + scratchpad_free(xhci); > > } > > > > int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > > @@ -754,7 +842,12 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t > flags) > > for (i = 0; i < MAX_HC_SLOTS; ++i) > > xhci->devs[i] = 0; > > > > + if (scratchpad_alloc(xhci, flags)) > > + goto fail; > > + > > + > > return 0; > > + > > fail: > > xhci_warn(xhci, "Couldn't initialize memory\n"); > > xhci_mem_cleanup(xhci); > > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > > index 341c907..c37b31a 100644 > > --- a/drivers/usb/host/xhci.h > > +++ b/drivers/usb/host/xhci.h > > @@ -89,6 +89,7 @@ struct xhci_cap_regs { > > #define HCS_ERST_MAX(p) (((p) >> 4) & 0xf) > > /* bit 26 Scratchpad restore - for save/restore HW state - not used > yet */ > > /* bits 27:31 number of Scratchpad buffers SW must allocate for the > HW */ > > +#define HCS_MAX_SCRATCHPAD(p) (((p) >> 27) & 0x1f) > > > > /* HCSPARAMS3 - hcs_params3 - bitmasks */ > > /* bits 0:7, Max U1 to U0 latency for the roothub ports */ > > @@ -968,6 +969,12 @@ struct xhci_erst { > > unsigned int erst_size; > > }; > > > > +struct xhci_scratchpad { > > + u64 *sp_array; > > + dma_addr_t sp_dma; > > + void **sp_buffers; > > +}; > > + > > /* > > * Each segment table entry is 4*32bits long. 1K seems like an ok > size: > > * (1K bytes * 8bytes/bit) / (4*32 bits) = 64 segment entries in the > table, > > @@ -1022,6 +1029,9 @@ struct xhci_hcd { > > struct xhci_ring *cmd_ring; > > struct xhci_ring *event_ring; > > struct xhci_erst erst; > > + /* Scratchpad */ > > + struct xhci_scratchpad *scratchpad; > > + > > /* slot enabling and address device helpers */ > > struct completion addr_dev; > > int slot_id; > > @@ -1032,6 +1042,7 @@ struct xhci_hcd { > > struct dma_pool *device_pool; > > struct dma_pool *segment_pool; > > > > + > > Whitespace fixes should go in separate patches. > > > #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING > > /* Poll the rings - for debugging */ > > struct timer_list event_ring_timer; > > -- > > 1.6.2.4 > > Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
