Re: chipidea: udc: kernel panic in isr_setup_status_phase

[Peter Chen]

On Fri, Sep 02, 2016 at 06:42:43PM +0200, Clemens Gruber wrote:
> On Fri, Sep 02, 2016 at 09:55:52AM +0800, Peter Chen wrote:
> > Do you have other 5V to USB_H1_VBUS? USB PHY needs 5V input voltage
> > as the source for USB LDO (3.0v), either from OTG or Host 1. I suspect
> > the lower vbus voltage causes the USB LDO voltage less than 3.0v, then
> > cause the unstable for USB PHY. If possible, you can connect MAIN 5V
> > (if it exists) directly to USB_H1_VBUS to see if this problem is fixed.
> 
> Yes, USB_H1_VBUS is supplied from internal 5V and it is accurate (+/-
> 0.03V). The USB_H_EN signal enables USB_H1_VBUS through a MIC2026-1.
> 
> The USB_OTG_VBUS signal from the picture with 4.69V came from the host
> PC and is not under our control.
> 
> I read in the ReferenceManual that the USBPHY defaults to USB_H1_VBUS if
> both USB_OTG_VBUS and USB_H1_VBUS are available.
> If we imagine the following situation: USB OTG cable is plugged-in, the
> board is powered on, USB_OTG_VBUS is supplied externally and USBPHY uses
> it because USB_H1_EN is not high yet and did not enable USB_H1_VBUS.
> If the USBPHY started out being supplied by USB_OTG_VBUS and later on,
> USB_H1_VBUS comes up, will the USBPHY switch to USB_H1_VBUS immediately
> or stay at being supplied by USB_OTG_VBUS as long as that is available?

The USB LDO will use the higher voltage as input.

> 
> > Great. Can you see the sudden lower for vbus again? If it still exists, it may
> > be GND issue.
> 
> Yes it is still visible. Does not seem to be a problem though.
> 
> > Avoid NULL pointer deference is necessary. Patch is welcome :)
> 
> Yes, but should we also stop the gadget driver and print an error
> message? Or just return from the isr and neither report nor stop?
> 

How about below, it will set halt for device, and host will get stall
from the device.

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 0f692fc..3c46ccb 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -946,6 +946,11 @@ static int isr_setup_status_phase(struct ci_hdrc *ci)
 	int retval;
 	struct ci_hw_ep *hwep;
 
+	if (!ci->status) {
+		WARN_ON(1);
+		return -EPIPE;
+	}
+
 	hwep = (ci->ep0_dir == TX) ? ci->ep0out : ci->ep0in;
 	ci->status->context = ci;
 	ci->status->complete = isr_setup_status_complete;

-- 

Best Regards,
Peter Chen