Re: [PATCH v2] Move DWC2 driver out of staging

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux