| Message ID | 20160918145316.23471-1-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | 7c22e26ec5badd564cfb674827ac11ac4e943e9f |
| Delegated to: | Hans de Goede |
| Headers | show |
On Sun, 2016-09-18 at 16:53 +0200, Hans de Goede wrote: > With sunxi-musb musb_lowlevel_init() can fail when a charger; or no > cable > is plugged into the otg port. > > To avoid leaking the struct musb allocated by musb_init_controller() > on repeated musb_usb_probe() calls, we were caching its result. > But musb_init_controller() does more, such as calling > sunxi_musb_init() > which enables the clocks. > > Not calling sunxi_musb_init() causes the musb controller to stop > working > after a "usb reset" since that calls musb_usb_remove() which disables > the > clocks. > > This commit fixes this by removing the caching of the struct returned > from musb_init_controller(), it replaces this by free-ing the > allocated > memory in musb_usb_remove() and calling musb_usb_remove() on > musb_usb_probe() errors to ensure proper cleanup. > > While at it also make musb_usb_probe() and musb_usb_remove() static. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> LGTM in so far as it seems like a more normal way to do things, but maybe an Ack from a USB-in-uboot-savvy person might be nice? Anyway: Acked-by: Ian Campbell <ijc@hellion.org.uk>
Hi, On 18-09-16 17:00, Ian Campbell wrote: > On Sun, 2016-09-18 at 16:53 +0200, Hans de Goede wrote: >> With sunxi-musb musb_lowlevel_init() can fail when a charger; or no >> cable >> is plugged into the otg port. >> >> To avoid leaking the struct musb allocated by musb_init_controller() >> on repeated musb_usb_probe() calls, we were caching its result. >> But musb_init_controller() does more, such as calling >> sunxi_musb_init() >> which enables the clocks. >> >> Not calling sunxi_musb_init() causes the musb controller to stop >> working >> after a "usb reset" since that calls musb_usb_remove() which disables >> the >> clocks. >> >> This commit fixes this by removing the caching of the struct returned >> from musb_init_controller(), it replaces this by free-ing the >> allocated >> memory in musb_usb_remove() and calling musb_usb_remove() on >> musb_usb_probe() errors to ensure proper cleanup. >> >> While at it also make musb_usb_probe() and musb_usb_remove() static. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > LGTM in so far as it seems like a more normal way to do things, Yeah my original hack here was bad, and something which I should never have done, ain't hindsight a wonderful thing :) Regards, Hans
diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c index dece781..469377f 100644 --- a/drivers/usb/musb-new/sunxi.c +++ b/drivers/usb/musb-new/sunxi.c @@ -201,7 +201,6 @@ static irqreturn_t sunxi_musb_interrupt(int irq, void *__hci) /* musb_core does not call enable / disable in a balanced manner <sigh> */ static bool enabled = false; -static struct musb *sunxi_musb; static int sunxi_musb_enable(struct musb *musb) { @@ -309,7 +308,9 @@ static struct musb_hdrc_platform_data musb_plat = { }; #ifdef CONFIG_USB_MUSB_HOST -int musb_usb_probe(struct udevice *dev) +static int musb_usb_remove(struct udevice *dev); + +static int musb_usb_probe(struct udevice *dev) { struct musb_host_data *host = dev_get_priv(dev); struct usb_bus_priv *priv = dev_get_uclass_priv(dev); @@ -317,23 +318,21 @@ int musb_usb_probe(struct udevice *dev) priv->desc_before_addr = true; - if (!sunxi_musb) { - sunxi_musb = musb_init_controller(&musb_plat, NULL, - (void *)SUNXI_USB0_BASE); - } - - host->host = sunxi_musb; + host->host = musb_init_controller(&musb_plat, NULL, + (void *)SUNXI_USB0_BASE); if (!host->host) return -EIO; ret = musb_lowlevel_init(host); if (ret == 0) printf("MUSB OTG\n"); + else + musb_usb_remove(dev); return ret; } -int musb_usb_remove(struct udevice *dev) +static int musb_usb_remove(struct udevice *dev) { struct musb_host_data *host = dev_get_priv(dev); struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE; @@ -346,6 +345,9 @@ int musb_usb_remove(struct udevice *dev) #endif clrbits_le32(&ccm->ahb_gate0, 1 << AHB_GATE_OFFSET_USB0); + free(host->host); + host->host = NULL; + return 0; }
With sunxi-musb musb_lowlevel_init() can fail when a charger; or no cable is plugged into the otg port. To avoid leaking the struct musb allocated by musb_init_controller() on repeated musb_usb_probe() calls, we were caching its result. But musb_init_controller() does more, such as calling sunxi_musb_init() which enables the clocks. Not calling sunxi_musb_init() causes the musb controller to stop working after a "usb reset" since that calls musb_usb_remove() which disables the clocks. This commit fixes this by removing the caching of the struct returned from musb_init_controller(), it replaces this by free-ing the allocated memory in musb_usb_remove() and calling musb_usb_remove() on musb_usb_probe() errors to ensure proper cleanup. While at it also make musb_usb_probe() and musb_usb_remove() static. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/usb/musb-new/sunxi.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)