| 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
| ||
|
Date: Tue, 8 Apr 2014 16:24:21 -0500
From: Felipe Balbi <balbi@...com>
To: sundeep subbaraya <sundeep.lkml@...il.com>
CC: <balbi@...com>,
Subbaraya Sundeep Bhatta <subbaraya.sundeep.bhatta@...inx.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Michal Simek <michals@...inx.com>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Subbaraya Sundeep Bhatta <sbhatta@...inx.com>
Subject: Re: Fwd: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device
support
Hi,
On Tue, Apr 08, 2014 at 09:31:29PM +0530, sundeep subbaraya wrote:
> >> >> +static const struct usb_gadget_ops xusb_udc_ops = {
> >> >> + .get_frame = xudc_get_frame,
> >> >> + .wakeup = xudc_wakeup,
> >> >> + .udc_start = xudc_start,
> >> >> + .udc_stop = xudc_stop,
> >> >
> >> > no pullup ??? What gives ? This HW doesn't support it ? really ?
> >>
> >> Yes, there is no pull up. writing to control register to start udc is
> >> sufficient for this IP.
> >
> > right, but is there a bit inside control register which actually starts
> > the IP ? You might want to move that to ->pullup(), see how we did on
> > drivers/usb/dwc3/gadget.c::dwc3_gadget_pullup(); we're basically using
> > that to control RUN/STOP bit in DCTL register. That bit has two
> > functions: a) actually enable the ip; b) connect data pullups.
> >
> > You can actually see with a scope that the line goes high and low when
> > you mess with that bit.
> >
> > The reason I suggest this is because you don't want to let your host see
> > a connection until your gadget driver is registered and ready to go and
> > that's what the pullup method would guarantee.
>
> No.There are only two bits in Control register, one for Ready bit, another for
> sending remote wakeup and remaining are reserved as zeroes. Until ready is
> set host do not see the gadget.
so this READY bit is what you want to toggle on pullup().
> >> >> + }
> >> >> + if (intrstatus & XUSB_STATUS_SUSPEND_MASK) {
> >> >> +
> >> >> + DBG("Suspend\n");
> >> >> +
> >> >> + /* Enable the reset and resume */
> >> >> + intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET);
> >> >> + intrreg |= XUSB_STATUS_RESET_MASK | XUSB_STATUS_RESUME_MASK;
> >> >> + udc->write_fn(udc->base_address, XUSB_IER_OFFSET, intrreg);
> >> >> + udc->usb_state = USB_STATE_SUSPENDED;
> >> >> +
> >> >> + if (udc->driver->suspend)
> >> >> + udc->driver->suspend(&udc->gadget);
> >> >> + }
> >> >
> >> > when are you going to call driver->resume() ??
> >>
> >> When an interrupt occurs we first check if udc->usb_state =
> >> USB_STATE_SUSPENDED,
> >> if yes driver->resume is called. Also if Resume bit is set then it is
> >> cleared too. Resume status bit is set
> >> when device is resumed by host after device sends Remote wakeup
> >> signalling to host.
> >
> > <snip>
> >
> >> >> +static irqreturn_t xudc_irq(int irq, void *_udc)
> >> >> +{
> >> >> + struct xusb_udc *udc = _udc;
> >> >> + u32 intrstatus;
> >> >> + u32 intrreg;
> >> >> + u8 index;
> >> >> + u32 bufintr;
> >> >> +
> >> >> + spin_lock(&(udc->lock));
> >> >> +
> >> >> + intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET);
> >> >> + intrreg &= ~XUSB_STATUS_INTR_EVENT_MASK;
> >> >> + udc->write_fn(udc->base_address, XUSB_IER_OFFSET, intrreg);
> >> >> +
> >> >> + /* Read the Interrupt Status Register.*/
> >> >> + intrstatus = udc->read_fn(udc->base_address + XUSB_STATUS_OFFSET);
> >> >> +
> >> >> + if (udc->usb_state == USB_STATE_SUSPENDED) {
> >> >> +
> >> >> + DBG("Resume\n");
> >> >> +
> >> >> + if (intrstatus & XUSB_STATUS_RESUME_MASK) {
> >> >> + /* Enable the reset and suspend */
> >> >> + intrreg = udc->read_fn(udc->base_address +
> >> >> + XUSB_IER_OFFSET);
> >> >> + intrreg |= XUSB_STATUS_RESET_MASK |
> >> >> + XUSB_STATUS_SUSPEND_MASK;
> >> >> + udc->write_fn(udc->base_address, XUSB_IER_OFFSET,
> >> >> + intrreg);
> >> >> + }
> >> >> + udc->usb_state = 0;
> >> >> +
> >> >> + if (udc->driver->resume)
> >> >> + udc->driver->resume(&udc->gadget);
> >
> > this is wrong, note that you would call ->resume() *every time*
> > usb_state == SUSPENDED and there's an interrupt. This means that if
> > gadget is suspended and you remove the cable, then you first call
> > ->resume() and then ->disconnect().
> >
> >> Here. calling driver->resume.
> >
> > Here's what I would do:
> >
> > if (intrstatus & XUSB_STATUS_RESUME_MASK) {
> > bool condition = udc->usb_state != USB_STATE_SUSPENDED;
> > dev_WARN_ONCE(dev, condition, "Resume IRQ while not suspended\n");
> >
> > /* Enable the reset and suspend */
> > intrreg = udc->read_fn(udc->base_address + XUSB_IER_OFFSET);
> > intrreg |= XUSB_STATUS_RESET_MASK | XUSB_STATUS_SUSPEND_MASK;
> > udc->write_fn(udc->base_address, XUSB_IER_OFFSET, intrreg);
> >
> > if (udc->driver->resume)
> > udc->driver_resume(&udc->gadget);
> > }
>
> Resume Interrupt bit is set only when Resume happens by device sending
> Remote wakeup.
so if the host drives resume signalling there will be no interrupt ?
Well, this is wrong :-) The gadget driver needs to know about that too.
The host side can decide to wake you up.
> I am assuming we need to call driver->resume for every
> driver->suspend. Hence I implemented
no you don't, you need to let host wake you up or you drive remote
wakeup on the bus. The thing is: if the bus is suspended, there will be
*no* activity on the bus prior to host driving Resume signal or device
driving Remote Wakeup signal.
Enable RESUME IRQ on probe and never disable it, it'll be a lot easier
to implement it.
--
balbi
Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)
Powered by blists - more mailing lists