linux-kernel - Re: [PATCH 3/3] usb: musb: fix setting JZ4740 gadget periphal mode on reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20131217021416.GA21787@saruman.home>
Date:	Mon, 16 Dec 2013 20:14:16 -0600
From:	Felipe Balbi <balbi@...com>
To:	Apelete Seketeli <apelete@...eteli.net>
CC:	Felipe Balbi <balbi@...com>, <linux-usb@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>,
	Lars-Peter Clausen <lars@...afoo.de>
Subject: Re: [PATCH 3/3] usb: musb: fix setting JZ4740 gadget periphal mode
 on reset

Hi,

On Tue, Dec 17, 2013 at 02:31:00AM +0100, Apelete Seketeli wrote:
> On 16-Dec-13, Felipe Balbi wrote:
> > On Sat, Dec 14, 2013 at 04:48:38AM +0100, Apelete Seketeli wrote:
> > > JZ4740 USB Device Controller is not OTG compatible and does not have DEVCTL
> > > register in silicon.
> > > 
> > > During ethernet-over-usb transactions, on reset, musb driver tries to
> > > read from DEVCTL and consequently sets device as host (A-Device)
> > > instead of peripheral (B-Device), which makes it a composite device to
> > > the USB gadget driver.
> > > This induces a kernel panic during power down where the USB gadget
> > > driver does a null pointer dereference when trying to access the
> > > composite device configuration.
> > > 
> > > On reset, do not rely on DEVCTL value for setting gadget peripheral
> > > mode: hardcode it instead to B-Device.
> > > 
> > > Signed-off-by: Apelete Seketeli <apelete@...eteli.net>
> > > ---
> > >  drivers/usb/musb/musb_gadget.c |    9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > > index 32fb057..b4bea7a 100644
> > > --- a/drivers/usb/musb/musb_gadget.c
> > > +++ b/drivers/usb/musb/musb_gadget.c
> > > @@ -2119,6 +2119,14 @@ __acquires(musb->lock)
> > >  	/* Normal reset, as B-Device;
> > >  	 * or else after HNP, as A-Device
> > >  	 */
> > > +#if defined(CONFIG_USB_MUSB_JZ4740) || defined(CONFIG_USB_MUSB_JZ4740_MODULE)
> > 
> > NAK, no ifdefs in this driver. Pass a quirk flag through platform_data
> > or something similar.
> 
> I get that, makes sense to me, but problem is the driver has to read a
> valid value from DEVCTL hardware register when musb_g_reset() is
> called, and I do not see how this can be achieved through
> platform_data.

why not ?

	/*
	 * JZ4740 UDC Controller is not OTG compatible as does not
	 * have a DEVCTL register in silicon. Due to that, we must
	 * NOT rely on that register for setting peripheral mode.
	 */
	if (unlikely(musb->quirk_broken_devctl)) {
		musb->xceiv->state = OTG_STATE_B_PERIPHERAL;
		musb->g_is_a_peripheral = 0;
	else if (devctl & MUSB_DEVCTL_BDEVICE) {
		musb->xceiv->state = OTG_STATE_B_PERIPHERAL;
		musb->g_is_a_peripheral = 0;
	} else {
		musb->xceiv->state = OTG_STATE_A_PERIPHERAL;
		musb->g_is_a_peripheral = 1;
	}

> Is it ok to use ifdefs in musb_regs.h to add specific hardware
> register indexes for JZ4740 instead ?

you guys changed the register file ? Why ? Is my pain not enough
already ? :-p

> I am actually thinking about fooling the musb driver into reading a
> valid value from another register instead of DEVCTL.

which register would that be ? If the register file is different, we
need to find a way to support it, but you gotta fix a few other things
first before I look into that, I don't want to see any more hacks and
ifdeffery hell around this driver. It's already painful enough to
support all HW variants it already supports.

cheers

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)