On Fri, Nov 15, 2013 at 07:48:30PM +0000, Paul Zimmerman wrote: > > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > > Sent: Thursday, November 14, 2013 8:33 PM > > > > On Thu, Nov 14, 2013 at 02:50:24PM -0800, Paul Zimmerman wrote: > > > DWC2 driver should be in good enough shape to move out of staging > > > > > > Signed-off-by: Paul Zimmerman <paulz@xxxxxxxxxxxx> > > > --- > > > Greg, is this the proper method for moving a driver out of staging? > > > > Yes, that's the way to do it. > > > > But, are all of the TODO entries really done? I don't want to have that > > file in drivers/usb/dwc2/ as that doesn't make much sense, right? > > I didn't realize the TODO file was only for things that needed to be > fixed before the driver can be moved from staging. In that case I > wouldn't have added some of the items, like the Raspberry Pi stuff or > the DT stuff. > > Other than those, I think the only remaining item is the first one. > Himangi's patch addressed that one, but I'm not sure if it fixes > all the concerns that Dan had. > I've added the staging mailing list. Potential use after free: drivers/staging/dwc2/hcd_ddma.c:1120 dwc2_complete_non_isoc_xfer_ddma() warn: 'qtd' was already freed. It's complaining because "qtd" gets freed on some error paths in dwc2_process_non_isoc_desc(). DWC2_PARAM_TEST() is not a very good name. Maybe: if (OUT_OF_BOUNDS(val, 0, 1)) { Some of the dwc2_set_param_max_packet_count() type functions could be simplified a little if the "valid" variable were removed. We don't care about "retval" in any of those functions so that could be removed as well from all of them. The NO_FS_PHY_HW_CHECKS could be be removed. u16 dwc2_get_otg_version(struct dwc2_hsotg *hsotg) { return (u16)(hsotg->core_params->otg_ver == 1 ? 0x0200 : 0x0103); ^^^^^ Remove the superfluous cast. } Rename dwc2_check_core_status() to dwc2_controller_connected() and make it return a bool. Remove the ifdef in dwc2_op_state_str(). drivers/staging/dwc2/hcd.c 370 retval = dwc2_hcd_qtd_add(hsotg, qtd, (struct dwc2_qh **)ep_handle, 371 mem_flags); 372 if (retval < 0) { 373 dev_err(hsotg->dev, 374 "DWC OTG HCD URB Enqueue failed adding QTD. Error status %d\n", 375 retval); 376 kfree(qtd); 377 return retval; 378 } 379 380 intr_mask = readl(hsotg->regs + GINTMSK); 381 if (!(intr_mask & GINTSTS_SOF) && retval == 0) { ^^^^^^^^^^^ This is always here. 382 enum dwc2_transaction_type tr_type; 383 384 if (qtd->qh->ep_type == USB_ENDPOINT_XFER_BULK && 385 !(qtd->urb->flags & URB_GIVEBACK_ASAP)) 386 /* 387 * Do not schedule SG transactions until qtd has 388 * URB_GIVEBACK_ASAP set 389 */ 390 return 0; 391 392 spin_lock_irqsave(&hsotg->lock, flags); 393 tr_type = dwc2_hcd_select_transactions(hsotg); 394 if (tr_type != DWC2_TRANSACTION_NONE) 395 dwc2_hcd_queue_transactions(hsotg, tr_type); 396 spin_unlock_irqrestore(&hsotg->lock, flags); 397 } 398 399 return retval; Just "return 0;" here. 400 } I'm half way through looking at the driver and that's all I've found so far. Looks pretty good to me so far. regards, dan carpenter -- 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
