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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAFp+6iHEJvCPHbSvqYhjuTjzvkKQVuKz8f8TupqFcZ9gdvpt-A@mail.gmail.com>
Date:	Fri, 11 Jan 2013 19:13:55 +0530
From:	Vivek Gautam <gautamvivek1987@...il.com>
To:	balbi@...com
Cc:	Vivek Gautam <gautam.vivek@...sung.com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
	gregkh@...uxfoundation.org, Doug Anderson <dianders@...omium.org>
Subject: Re: [PATCH RFC] usb: dwc3: Remove dwc3 dependency on gadget.

Hi Felipe,


On Thu, Jan 10, 2013 at 6:32 PM, Felipe Balbi <balbi@...com> wrote:
> Hi,
>
> On Mon, Dec 24, 2012 at 07:28:33PM +0530, Vivek Gautam wrote:
>> DWC3 controller curretly depends on CONFIG_USB and CONFIG_USB_GADGET.
>> Some hardware may like to use only host feature on dwc3.
>> So, removing the dependency of USB_DWC3 on USB_GADGET
>> and further modulating the dwc3 core to enable gadget features
>> only with USB_GADGET.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@...sung.com>
>> CC: Doug Anderson <dianders@...omium.org>
>
> right, right... Eventually we need to do it, but you're only making
> gadget side optional. Host side should be optional too, but then you
> need to make sure we don't build dwc3 without gadget and host.
>

Yes, true we need to make host side also optional, build dwc3 only when
either of host or gadget are built.

> Maybe make a mode selection for Host-only, Peripheral-only, Dual-Role
> Device ??
>

True this will be good idea to use modes: host only, gadget only or dual role.
May be the platform glue layers can use them later to put the controller
in a specific mode ?

> More comments below...
>
>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>> index f6a6e07..b70dcf1 100644
>> --- a/drivers/usb/dwc3/Kconfig
>> +++ b/drivers/usb/dwc3/Kconfig
>> @@ -1,7 +1,7 @@
>>  config USB_DWC3
>>       tristate "DesignWare USB3 DRD Core Support"
>> -     depends on (USB && USB_GADGET)
>> -     select USB_OTG_UTILS
>> +     depends on USB
>> +     select USB_OTG_UTILS if USB_GADGET
>
> we want USB_OTG_UTILS also on host-only builds because host side,
> eventually, will have to learn how to control its PHYs.
>

Ok.

>>       select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>       help
>>         Say Y or M here if your system has a Dual Role SuperSpeed
>> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
>> index 4502648..8f469cb 100644
>> --- a/drivers/usb/dwc3/Makefile
>> +++ b/drivers/usb/dwc3/Makefile
>> @@ -5,7 +5,7 @@ obj-$(CONFIG_USB_DWC3)                        += dwc3.o
>>
>>  dwc3-y                                       := core.o
>>  dwc3-y                                       += host.o
>> -dwc3-y                                       += gadget.o ep0.o
>> +obj-$(CONFIG_USB_GADGET)               += gadget.o ep0.o
>
> this is wrong. CONFIG_USB_GADGET is tristate, so this should be:
>
> ifneq($(CONFIG_USB_GADGET),)
>  dwc3-y += gadget.o ep0.o
> endif
>
> or something similar
>

True, missed taking into account that CONFIG_USB_GADGET is tristate :-(
Will amend this as suggested.

>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 4999563..4dc7ef2 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -865,7 +865,14 @@ int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc);
>>  int dwc3_host_init(struct dwc3 *dwc);
>>  void dwc3_host_exit(struct dwc3 *dwc);
>>
>> +#ifdef CONFIG_USB_GADGET
>
> not taking into account module builds.
>

ditto

>>  int dwc3_gadget_init(struct dwc3 *dwc);
>>  void dwc3_gadget_exit(struct dwc3 *dwc);
>> +#else
>> +static inline int dwc3_gadget_init(struct dwc3 *dwc)
>> +{ return -EINVAL; }
>> +static inline void dwc3_gadget_exit(struct dwc3 *dwc)
>> +{ }
>> +#endif
>>
>>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>> index d4a30f1..553bbaa 100644
>> --- a/drivers/usb/dwc3/debugfs.c
>> +++ b/drivers/usb/dwc3/debugfs.c
>> @@ -535,7 +535,8 @@ static ssize_t dwc3_testmode_write(struct file *file,
>>               testmode = 0;
>>
>>       spin_lock_irqsave(&dwc->lock, flags);
>> -     dwc3_gadget_set_test_mode(dwc, testmode);
>> +     if (dwc3_gadget_set_test_mode(dwc, testmode))
>> +             dev_dbg(dwc->dev, "host: Invalid request\n");
>>       spin_unlock_irqrestore(&dwc->lock, flags);
>>
>>       return count;
>
> wrong, if you don't have gadget mode, you just don't create this file.
>

dwc3-core makes call to dwc3_debugfs_init() invariably depending on DEBUG_FS.
Will this not go ahead and create this file ?
I think i am missing here something.  :-(

>> @@ -638,7 +639,8 @@ static ssize_t dwc3_link_state_write(struct file *file,
>>               return -EINVAL;
>>
>>       spin_lock_irqsave(&dwc->lock, flags);
>> -     dwc3_gadget_set_link_state(dwc, state);
>> +     if (dwc3_gadget_set_link_state(dwc, state))
>> +             dev_dbg(dwc->dev, "host: Invalid request\n");
>
> likewise.
>
>>       spin_unlock_irqrestore(&dwc->lock, flags);
>>
>>       return count;
>> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
>> index 99e6d72..28e82db 100644
>> --- a/drivers/usb/dwc3/gadget.h
>> +++ b/drivers/usb/dwc3/gadget.h
>> @@ -105,8 +105,16 @@ static inline void dwc3_gadget_move_request_queued(struct dwc3_request *req)
>>  void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
>>               int status);
>>
>> +#ifdef CONFIG_USB_GADGET
>
> not taking into account CONFIG_USB_GADGET=m
>
missed taking into account that CONFIG_USB_GADGET is tristate :-(

Will amend this patch accordingly.


-- 
Thanks & Regards
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists