[PATCH v3 0/3] usb: dwc3: dual-role support

[PATCH v3 0/3] usb: dwc3: dual-role support

[Roger Quadros]

Hi,

Taking a simplistic approach this time. We don't use the OTG controller
block at all. Instead we just rely on ID events via extcon framework.

I tried to get rid of workqueue but unfortunately it causes too much pain
when the UDC is unregistered.

https://hastebin.com/upugaqogol.xml
https://hastebin.com/yayaduqodo.xml

So I've still kept the workqueue around.

We also have debugfs role switching, but I don't yet see how we
can use this for real testing as I still need to manually plug/unplug
the USB cables (host vs device) to test switching :).

v3:
- restructure and simplify. Remove OTG controller code, only rely on extcon.

cheers,
-roger

Roger Quadros (3):
  usb: udc: allow adding and removing the same gadget device
  usb: dwc3: make role-switching work with debugfs/mode
  usb: dwc3: Add dual-role support

 drivers/usb/dwc3/Makefile     |   4 +
 drivers/usb/dwc3/core.c       |  18 ++---
 drivers/usb/dwc3/core.h       |  22 ++++++
 drivers/usb/dwc3/debugfs.c    |  49 +++++++++++--
 drivers/usb/dwc3/drd.c        | 167 ++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/core.c |   1 +
 6 files changed, 242 insertions(+), 19 deletions(-)
 create mode 100644 drivers/usb/dwc3/drd.c

-- 
2.7.4

[PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Roger Quadros]

allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
repeatedly on the same gadget->dev structure.

We need to clear the gadget->dev structure so that kobject_init()
doesn't complain about already initialized object.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/gadget/udc/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index d685d82..efce68e 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
 	flush_work(&gadget->work);
 	device_unregister(&udc->dev);
 	device_unregister(&gadget->dev);
+	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 
-- 
2.7.4

[PATCH v3 2/3] usb: dwc3: make role-switching work with debugfs/mode

[Roger Quadros]

If dr_mode == "otg", we start by default in PERIPHERAL mode.
Keep track of current role in "current_dr_role" whenever dwc3_set_mode()
is called.

When debugfs/mode is changed AND we're in dual-role mode,
handle the switch by stopping and starting the respective
host/gadget controllers.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.c    | 21 +++++++++++---------
 drivers/usb/dwc3/core.h    |  3 +++
 drivers/usb/dwc3/debugfs.c | 49 +++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 369bab1..bf917c2 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
 	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
 	reg |= DWC3_GCTL_PRTCAPDIR(mode);
 	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+
+	dwc->current_dr_role = mode;
 }
 
 u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
@@ -277,7 +279,7 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned length)
  *
  * Returns 0 on success otherwise negative errno.
  */
-static int dwc3_event_buffers_setup(struct dwc3 *dwc)
+int dwc3_event_buffers_setup(struct dwc3 *dwc)
 {
 	struct dwc3_event_buffer	*evt;
 
@@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		ret = dwc3_host_init(dwc);
-		if (ret) {
-			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to initialize host\n");
-			return ret;
-		}
-
+		/* start in peripheral role by default */
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
 		ret = dwc3_gadget_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
@@ -894,7 +891,13 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 		dwc3_host_exit(dwc);
 		break;
 	case USB_DR_MODE_OTG:
-		dwc3_host_exit(dwc);
+		/* role might have changed since start */
+		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
+			dwc3_host_exit(dwc);
+			/* Add back UDC to match dwc3_gadget_exit() */
+			usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+		}
+
 		dwc3_gadget_exit(dwc);
 		break;
 	default:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 7ffdee5..adb04af 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -785,6 +785,7 @@ struct dwc3_scratchpad_array {
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
+ * @current_dr_role: current role of operation when in dual-role mode
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
@@ -901,6 +902,7 @@ struct dwc3 {
 	size_t			regs_size;
 
 	enum usb_dr_mode	dr_mode;
+	u32			current_dr_role;
 	enum usb_phy_interface	hsphy_mode;
 
 	u32			fladj;
@@ -1167,6 +1169,7 @@ struct dwc3_gadget_ep_cmd_params {
 /* prototypes */
 void dwc3_set_mode(struct dwc3 *dwc, u32 mode);
 u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type);
+int dwc3_event_buffers_setup(struct dwc3 *dwc);
 
 /* check whether we are on the DWC_usb3 core */
 static inline bool dwc3_is_usb3(struct dwc3 *dwc)
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 31926dd..a74a83b 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,
 		return -EFAULT;
 
 	if (!strncmp(buf, "host", 4))
-		mode |= DWC3_GCTL_PRTCAP_HOST;
+		mode = DWC3_GCTL_PRTCAP_HOST;
 
 	if (!strncmp(buf, "device", 6))
-		mode |= DWC3_GCTL_PRTCAP_DEVICE;
+		mode = DWC3_GCTL_PRTCAP_DEVICE;
 
 	if (!strncmp(buf, "otg", 3))
-		mode |= DWC3_GCTL_PRTCAP_OTG;
+		mode = DWC3_GCTL_PRTCAP_OTG;
 
-	if (mode) {
-		spin_lock_irqsave(&dwc->lock, flags);
-		dwc3_set_mode(dwc, mode);
-		spin_unlock_irqrestore(&dwc->lock, flags);
+	if (!mode)
+		return -EINVAL;
+
+	if (mode == dwc->current_dr_role)
+		goto exit;
+
+	/* prevent role switching if we're not dual-role */
+	if (dwc->dr_mode != USB_DR_MODE_OTG)
+		return -EINVAL;
+
+	/* stop old role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_exit(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		usb_del_gadget_udc(&dwc->gadget);
+		break;
+	default:
+		break;
+	}
+
+	/* switch PRTCAP mode. updates current_dr_role */
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_set_mode(dwc, mode);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	/* start new role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_init(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		dwc3_event_buffers_setup(dwc);
+		usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+		break;
+	default:
+		break;
 	}
+exit:
 	return count;
 }
 
-- 
2.7.4

[PATCH v3 3/3] usb: dwc3: Add dual-role support

[Roger Quadros]

If dr_mode is "otg" then support dual role mode of operation.
Currently this mode is only supported when an extcon handle is
present in the dwc3 device tree node. This is needed to
get the ID status events of the port.

We're using a workqueue to manage the dual-role state transitions
as the extcon notifier (dwc3_drd_notifier) is called in an atomic
context by extcon_sync() and this doesn't go well with
usb_del_gadget_udc() causing a lockdep and softirq warning.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/Makefile |   4 ++
 drivers/usb/dwc3/core.c   |  15 +----
 drivers/usb/dwc3/core.h   |  19 ++++++
 drivers/usb/dwc3/drd.c    | 167 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 193 insertions(+), 12 deletions(-)
 create mode 100644 drivers/usb/dwc3/drd.c

diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index ffca340..f15fabb 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -17,6 +17,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) $(CONFIG_USB_DWC3_DUAL_ROLE)),)
 	dwc3-y				+= gadget.o ep0.o
 endif
 
+ifneq ($(CONFIG_USB_DWC3_DUAL_ROLE),)
+	dwc3-y				+= drd.o
+endif
+
 ifneq ($(CONFIG_USB_DWC3_ULPI),)
 	dwc3-y				+= ulpi.o
 endif
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index bf917c2..15c05a1 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -864,12 +864,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		/* start in peripheral role by default */
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
-		ret = dwc3_gadget_init(dwc);
+		ret = dwc3_drd_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to initialize gadget\n");
+				dev_err(dev, "failed to initialize dual-role\n");
 			return ret;
 		}
 		break;
@@ -891,14 +889,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 		dwc3_host_exit(dwc);
 		break;
 	case USB_DR_MODE_OTG:
-		/* role might have changed since start */
-		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
-			dwc3_host_exit(dwc);
-			/* Add back UDC to match dwc3_gadget_exit() */
-			usb_add_gadget_udc(dwc->dev, &dwc->gadget);
-		}
-
-		dwc3_gadget_exit(dwc);
+		dwc3_drd_exit(dwc);
 		break;
 	default:
 		/* do nothing */
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index adb04af..8402d8f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -786,6 +786,10 @@ struct dwc3_scratchpad_array {
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
  * @current_dr_role: current role of operation when in dual-role mode
+ * @edev: extcon handle
+ * @edev_nb: extcon notifier
+ * @drd_work: dual-role work
+ * @drd_prevent_change: flag to prevent dual-role state change
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
@@ -903,6 +907,11 @@ struct dwc3 {
 
 	enum usb_dr_mode	dr_mode;
 	u32			current_dr_role;
+	struct extcon_dev	*edev;
+	struct notifier_block	edev_nb;
+	bool			drd_prevent_change;
+	struct work_struct	drd_work;
+
 	enum usb_phy_interface	hsphy_mode;
 
 	u32			fladj;
@@ -1225,6 +1234,16 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
 { return 0; }
 #endif
 
+#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
+int dwc3_drd_init(struct dwc3 *dwc);
+void dwc3_drd_exit(struct dwc3 *dwc);
+#else
+static inline int dwc3_drd_init(struct dwc3 *dwc)
+{ return 0; }
+static void dwc3_drd_exit(struct dwc3 *dwc);
+{ }
+#endif
+
 /* power management interface */
 #if !IS_ENABLED(CONFIG_USB_DWC3_HOST)
 int dwc3_gadget_suspend(struct dwc3 *dwc);
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
new file mode 100644
index 0000000..c9b02a3
--- /dev/null
+++ b/drivers/usb/dwc3/drd.c
@@ -0,0 +1,167 @@
+/**
+ * drd.c - DesignWare USB3 DRD Controller Dual-role support
+ *
+ * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Authors: Roger Quadros <rogerq@ti.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/extcon.h>
+
+#include "debug.h"
+#include "core.h"
+#include "gadget.h"
+
+static void dwc3_drd_update(struct dwc3 *dwc)
+{
+	int id;
+	int new_role;
+	unsigned long flags;
+
+	if (dwc->drd_prevent_change)
+		return;
+
+	id = extcon_get_state(dwc->edev, EXTCON_USB_HOST);
+	/* Host means ID is 0 */
+	id = !id;
+
+	if (!id)
+		new_role = DWC3_GCTL_PRTCAP_HOST;
+	else
+		new_role = DWC3_GCTL_PRTCAP_DEVICE;
+
+	if (dwc->current_dr_role == new_role)
+		return;
+
+	/* stop old role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_exit(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		usb_del_gadget_udc(&dwc->gadget);
+		break;
+	default:
+		break;
+	}
+
+	/* switch PRTCAP mode. updates current_dr_role */
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_set_mode(dwc, new_role);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	/* start new role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_init(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		dwc3_event_buffers_setup(dwc);
+		usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+		break;
+	default:
+		break;
+	}
+}
+
+static void dwc3_drd_work(struct work_struct *work)
+{
+	struct dwc3 *dwc = container_of(work, struct dwc3,
+					drd_work);
+	dwc3_drd_update(dwc);
+}
+
+static int dwc3_drd_notifier(struct notifier_block *nb,
+			     unsigned long event, void *ptr)
+{
+	struct dwc3 *dwc = container_of(nb, struct dwc3, edev_nb);
+
+	queue_work(system_power_efficient_wq, &dwc->drd_work);
+
+	return NOTIFY_DONE;
+}
+
+int dwc3_drd_init(struct dwc3 *dwc)
+{
+	int ret;
+	int id;
+	struct device *dev = dwc->dev;
+
+	INIT_WORK(&dwc->drd_work, dwc3_drd_work);
+
+	if (dev->of_node) {
+		if (of_property_read_bool(dev->of_node, "extcon"))
+			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
+
+		if (IS_ERR(dwc->edev))
+			return PTR_ERR(dwc->edev);
+	} else {
+		return -ENODEV;
+	}
+
+	dwc->edev_nb.notifier_call = dwc3_drd_notifier;
+	ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
+				       &dwc->edev_nb);
+	if (ret < 0) {
+		dev_err(dwc->dev, "Couldn't register USB-HOST cable notifier\n");
+		return -ENODEV;
+	}
+
+	/* sanity check id & vbus states */
+	id = extcon_get_state(dwc->edev, EXTCON_USB_HOST);
+	if (id < 0) {
+		dev_err(dwc->dev, "Invalid USB cable state. ID %d", id);
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	/* start in peripheral role by default */
+	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+	ret = dwc3_gadget_init(dwc);
+	if (ret)
+		goto fail;
+
+	/* check & update drd state */
+	dwc3_drd_update(dwc);
+
+	return 0;
+
+fail:
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
+				   &dwc->edev_nb);
+
+	return ret;
+}
+
+void dwc3_drd_exit(struct dwc3 *dwc)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc->drd_prevent_change = true;
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
+				   &dwc->edev_nb);
+
+	/* role might have changed since start, stop active controller */
+	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
+		dwc3_host_exit(dwc);
+		/* Add back UDC to match dwc3_gadget_exit() */
+		usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+	}
+
+	dwc3_gadget_exit(dwc);
+}
-- 
2.7.4

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Mon, 3 Apr 2017, Roger Quadros wrote:

> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
> repeatedly on the same gadget->dev structure.
> 
> We need to clear the gadget->dev structure so that kobject_init()
> doesn't complain about already initialized object.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/gadget/udc/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index d685d82..efce68e 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>  	flush_work(&gadget->work);
>  	device_unregister(&udc->dev);
>  	device_unregister(&gadget->dev);
> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>  }
>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);

Isn't this dangerous?  It's quite possible that the device_unregister() 
call on the previous line invokes the gadget->dev.release callback, 
which might deallocate gadget.  If that happens, your new memset will 
oops.

In general, if an object relies on reference counting for its lifetime, 
you cannot register and unregister it more than once.  A typical issue 
is that some code retains a reference to the old instance and tries to 
use it after the new instance has been registered, thereby messing up 
the new instance.  I don't know if that is possible in this case, but 
it is something to watch out for.

Alan Stern

Re: [PATCH v3 3/3] usb: dwc3: Add dual-role support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2018 bytes --]

Hi Roger,

[auto build test ERROR on balbi-usb/next]
[also build test ERROR on v4.11-rc5 next-20170403]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Roger-Quadros/usb-dwc3-dual-role-support/20170404-022401
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
config: i386-randconfig-x010-201714 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from drivers/usb/dwc3/core.c:44:0:
>> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token
    { }
    ^
>> drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' used but never defined
    static void dwc3_drd_exit(struct dwc3 *dwc);
                ^~~~~~~~~~~~~
--
   In file included from drivers/usb/dwc3/trace.h:27:0,
                    from drivers/usb/dwc3/trace.c:19:
>> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token
    { }
    ^
   In file included from drivers/usb/dwc3/trace.h:27:0,
                    from drivers/usb/dwc3/trace.c:19:
   drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' declared 'static' but never defined [-Wunused-function]
    static void dwc3_drd_exit(struct dwc3 *dwc);
                ^~~~~~~~~~~~~

vim +1243 drivers/usb/dwc3/core.h

  1236	#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
  1237	int dwc3_drd_init(struct dwc3 *dwc);
  1238	void dwc3_drd_exit(struct dwc3 *dwc);
  1239	#else
  1240	static inline int dwc3_drd_init(struct dwc3 *dwc)
  1241	{ return 0; }
> 1242	static void dwc3_drd_exit(struct dwc3 *dwc);
> 1243	{ }
  1244	#endif
  1245	
  1246	/* power management interface */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27087 bytes --]

Re: [PATCH v3 3/3] usb: dwc3: Add dual-role support

[Roger Quadros]

Felipe,

I'll send a revised patch to fix this.

cheers,
-roger

On 03/04/17 22:21, kbuild test robot wrote:
> Hi Roger,
> 
> [auto build test ERROR on balbi-usb/next]
> [also build test ERROR on v4.11-rc5 next-20170403]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Roger-Quadros/usb-dwc3-dual-role-support/20170404-022401
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
> config: i386-randconfig-x010-201714 (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    In file included from drivers/usb/dwc3/core.c:44:0:
>>> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token
>     { }
>     ^
>>> drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' used but never defined
>     static void dwc3_drd_exit(struct dwc3 *dwc);
>                 ^~~~~~~~~~~~~
> --
>    In file included from drivers/usb/dwc3/trace.h:27:0,
>                     from drivers/usb/dwc3/trace.c:19:
>>> drivers/usb/dwc3/core.h:1243:1: error: expected identifier or '(' before '{' token
>     { }
>     ^
>    In file included from drivers/usb/dwc3/trace.h:27:0,
>                     from drivers/usb/dwc3/trace.c:19:
>    drivers/usb/dwc3/core.h:1242:13: warning: 'dwc3_drd_exit' declared 'static' but never defined [-Wunused-function]
>     static void dwc3_drd_exit(struct dwc3 *dwc);
>                 ^~~~~~~~~~~~~
> 
> vim +1243 drivers/usb/dwc3/core.h
> 
>   1236	#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>   1237	int dwc3_drd_init(struct dwc3 *dwc);
>   1238	void dwc3_drd_exit(struct dwc3 *dwc);
>   1239	#else
>   1240	static inline int dwc3_drd_init(struct dwc3 *dwc)
>   1241	{ return 0; }
>> 1242	static void dwc3_drd_exit(struct dwc3 *dwc);
>> 1243	{ }
>   1244	#endif
>   1245	
>   1246	/* power management interface */
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 

[PATCH v4 3/3] usb: dwc3: Add dual-role support

[Roger Quadros]

If dr_mode is "otg" then support dual role mode of operation.
Currently this mode is only supported when an extcon handle is
present in the dwc3 device tree node. This is needed to
get the ID status events of the port.

We're using a workqueue to manage the dual-role state transitions
as the extcon notifier (dwc3_drd_notifier) is called in an atomic
context by extcon_sync() and this doesn't go well with
usb_del_gadget_udc() causing a lockdep and softirq warning.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v4:
-fix build error if CONFIG_USB_DWC3_DUAL_ROLE is not set

 drivers/usb/dwc3/Makefile |   4 ++
 drivers/usb/dwc3/core.c   |  15 +----
 drivers/usb/dwc3/core.h   |  19 ++++++
 drivers/usb/dwc3/drd.c    | 167 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 193 insertions(+), 12 deletions(-)
 create mode 100644 drivers/usb/dwc3/drd.c

diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index ffca340..f15fabb 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -17,6 +17,10 @@ ifneq ($(filter y,$(CONFIG_USB_DWC3_GADGET) $(CONFIG_USB_DWC3_DUAL_ROLE)),)
 	dwc3-y				+= gadget.o ep0.o
 endif
 
+ifneq ($(CONFIG_USB_DWC3_DUAL_ROLE),)
+	dwc3-y				+= drd.o
+endif
+
 ifneq ($(CONFIG_USB_DWC3_ULPI),)
 	dwc3-y				+= ulpi.o
 endif
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index bf917c2..15c05a1 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -864,12 +864,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		/* start in peripheral role by default */
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
-		ret = dwc3_gadget_init(dwc);
+		ret = dwc3_drd_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to initialize gadget\n");
+				dev_err(dev, "failed to initialize dual-role\n");
 			return ret;
 		}
 		break;
@@ -891,14 +889,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 		dwc3_host_exit(dwc);
 		break;
 	case USB_DR_MODE_OTG:
-		/* role might have changed since start */
-		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
-			dwc3_host_exit(dwc);
-			/* Add back UDC to match dwc3_gadget_exit() */
-			usb_add_gadget_udc(dwc->dev, &dwc->gadget);
-		}
-
-		dwc3_gadget_exit(dwc);
+		dwc3_drd_exit(dwc);
 		break;
 	default:
 		/* do nothing */
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index adb04af..b1be4a3 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -786,6 +786,10 @@ struct dwc3_scratchpad_array {
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
  * @current_dr_role: current role of operation when in dual-role mode
+ * @edev: extcon handle
+ * @edev_nb: extcon notifier
+ * @drd_work: dual-role work
+ * @drd_prevent_change: flag to prevent dual-role state change
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
@@ -903,6 +907,11 @@ struct dwc3 {
 
 	enum usb_dr_mode	dr_mode;
 	u32			current_dr_role;
+	struct extcon_dev	*edev;
+	struct notifier_block	edev_nb;
+	bool			drd_prevent_change;
+	struct work_struct	drd_work;
+
 	enum usb_phy_interface	hsphy_mode;
 
 	u32			fladj;
@@ -1225,6 +1234,16 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
 { return 0; }
 #endif
 
+#if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
+int dwc3_drd_init(struct dwc3 *dwc);
+void dwc3_drd_exit(struct dwc3 *dwc);
+#else
+static inline int dwc3_drd_init(struct dwc3 *dwc)
+{ return 0; }
+static inline void dwc3_drd_exit(struct dwc3 *dwc)
+{ }
+#endif
+
 /* power management interface */
 #if !IS_ENABLED(CONFIG_USB_DWC3_HOST)
 int dwc3_gadget_suspend(struct dwc3 *dwc);
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
new file mode 100644
index 0000000..c9b02a3
--- /dev/null
+++ b/drivers/usb/dwc3/drd.c
@@ -0,0 +1,167 @@
+/**
+ * drd.c - DesignWare USB3 DRD Controller Dual-role support
+ *
+ * Copyright (C) 2017 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Authors: Roger Quadros <rogerq@ti.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/extcon.h>
+
+#include "debug.h"
+#include "core.h"
+#include "gadget.h"
+
+static void dwc3_drd_update(struct dwc3 *dwc)
+{
+	int id;
+	int new_role;
+	unsigned long flags;
+
+	if (dwc->drd_prevent_change)
+		return;
+
+	id = extcon_get_state(dwc->edev, EXTCON_USB_HOST);
+	/* Host means ID is 0 */
+	id = !id;
+
+	if (!id)
+		new_role = DWC3_GCTL_PRTCAP_HOST;
+	else
+		new_role = DWC3_GCTL_PRTCAP_DEVICE;
+
+	if (dwc->current_dr_role == new_role)
+		return;
+
+	/* stop old role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_exit(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		usb_del_gadget_udc(&dwc->gadget);
+		break;
+	default:
+		break;
+	}
+
+	/* switch PRTCAP mode. updates current_dr_role */
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_set_mode(dwc, new_role);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	/* start new role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_init(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		dwc3_event_buffers_setup(dwc);
+		usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+		break;
+	default:
+		break;
+	}
+}
+
+static void dwc3_drd_work(struct work_struct *work)
+{
+	struct dwc3 *dwc = container_of(work, struct dwc3,
+					drd_work);
+	dwc3_drd_update(dwc);
+}
+
+static int dwc3_drd_notifier(struct notifier_block *nb,
+			     unsigned long event, void *ptr)
+{
+	struct dwc3 *dwc = container_of(nb, struct dwc3, edev_nb);
+
+	queue_work(system_power_efficient_wq, &dwc->drd_work);
+
+	return NOTIFY_DONE;
+}
+
+int dwc3_drd_init(struct dwc3 *dwc)
+{
+	int ret;
+	int id;
+	struct device *dev = dwc->dev;
+
+	INIT_WORK(&dwc->drd_work, dwc3_drd_work);
+
+	if (dev->of_node) {
+		if (of_property_read_bool(dev->of_node, "extcon"))
+			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
+
+		if (IS_ERR(dwc->edev))
+			return PTR_ERR(dwc->edev);
+	} else {
+		return -ENODEV;
+	}
+
+	dwc->edev_nb.notifier_call = dwc3_drd_notifier;
+	ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
+				       &dwc->edev_nb);
+	if (ret < 0) {
+		dev_err(dwc->dev, "Couldn't register USB-HOST cable notifier\n");
+		return -ENODEV;
+	}
+
+	/* sanity check id & vbus states */
+	id = extcon_get_state(dwc->edev, EXTCON_USB_HOST);
+	if (id < 0) {
+		dev_err(dwc->dev, "Invalid USB cable state. ID %d", id);
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	/* start in peripheral role by default */
+	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+	ret = dwc3_gadget_init(dwc);
+	if (ret)
+		goto fail;
+
+	/* check & update drd state */
+	dwc3_drd_update(dwc);
+
+	return 0;
+
+fail:
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
+				   &dwc->edev_nb);
+
+	return ret;
+}
+
+void dwc3_drd_exit(struct dwc3 *dwc)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc->drd_prevent_change = true;
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
+				   &dwc->edev_nb);
+
+	/* role might have changed since start, stop active controller */
+	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
+		dwc3_host_exit(dwc);
+		/* Add back UDC to match dwc3_gadget_exit() */
+		usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+	}
+
+	dwc3_gadget_exit(dwc);
+}
-- 
2.7.4

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, 3 Apr 2017, Roger Quadros wrote:
>
>> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
>> repeatedly on the same gadget->dev structure.
>> 
>> We need to clear the gadget->dev structure so that kobject_init()
>> doesn't complain about already initialized object.
>> 
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/usb/gadget/udc/core.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index d685d82..efce68e 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>>  	flush_work(&gadget->work);
>>  	device_unregister(&udc->dev);
>>  	device_unregister(&gadget->dev);
>> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>>  }
>>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>
> Isn't this dangerous?  It's quite possible that the device_unregister() 

not on the gadget API, no.

> call on the previous line invokes the gadget->dev.release callback, 
> which might deallocate gadget.  If that happens, your new memset will 
> oops.

that won't happen. struct usb_gadget is a member of the UDC's private
structure, like this:

struct dwc3 {
	[...]
	struct usb_gadget	gadget;
	struct usb_gadget_driver *gadget_driver;
	[...]
};

I'm actually thinking that struct usb_gadget shouldn't have a struct
device at all. Just a pointer to a device, that would solve all these
issues.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Tue, 4 Apr 2017, Felipe Balbi wrote:

> Hi,
> 
> Alan Stern <stern@rowland.harvard.edu> writes:
> > On Mon, 3 Apr 2017, Roger Quadros wrote:
> >
> >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
> >> repeatedly on the same gadget->dev structure.
> >> 
> >> We need to clear the gadget->dev structure so that kobject_init()
> >> doesn't complain about already initialized object.
> >> 
> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >> ---
> >>  drivers/usb/gadget/udc/core.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> >> index d685d82..efce68e 100644
> >> --- a/drivers/usb/gadget/udc/core.c
> >> +++ b/drivers/usb/gadget/udc/core.c
> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> >>  	flush_work(&gadget->work);
> >>  	device_unregister(&udc->dev);
> >>  	device_unregister(&gadget->dev);
> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
> >>  }
> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >
> > Isn't this dangerous?  It's quite possible that the device_unregister() 
> 
> not on the gadget API, no.
> 
> > call on the previous line invokes the gadget->dev.release callback, 
> > which might deallocate gadget.  If that happens, your new memset will 
> > oops.
> 
> that won't happen. struct usb_gadget is a member of the UDC's private
> structure, like this:
> 
> struct dwc3 {
> 	[...]
> 	struct usb_gadget	gadget;
> 	struct usb_gadget_driver *gadget_driver;
> 	[...]
> };

Yes.  So what?  Can't the UDC driver use the refcount inside struct 
usb_gadget to control the lifetime of its private structure?

(By the way, can you tell what's going on in net2280.c?  I must be
missing something; it looks like gadget_release() would quickly run
into problems because it calls dev_get_drvdata() for &gadget->dev, but
net2280_probe() never calls dev_set_drvdata() for that device.  
Furthermore, net2280_remove() continues to reference the net2280 struct
after calling usb_del_gadget_udc(), and it never does seem to do a
final put.)

> I'm actually thinking that struct usb_gadget shouldn't have a struct
> device at all. Just a pointer to a device, that would solve all these
> issues.

A pointer to which device?  The UDC?  That would change the directory 
layout in sysfs.

Or a pointer to a separate dynamically allocated device (the way struct 
usb_hcd contains a pointer to the root hub device)?  That would work.  
If the UDC driver wanted to re-register the gadget, it would have to 
allocate a new device.

Alan Stern

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3181 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> >> allow usb_del_gadget_udc() and usb add_gadget_udc() to be called
>> >> repeatedly on the same gadget->dev structure.
>> >> 
>> >> We need to clear the gadget->dev structure so that kobject_init()
>> >> doesn't complain about already initialized object.
>> >> 
>> >> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> >> ---
>> >>  drivers/usb/gadget/udc/core.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >> 
>> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> >> index d685d82..efce68e 100644
>> >> --- a/drivers/usb/gadget/udc/core.c
>> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>> >>  	flush_work(&gadget->work);
>> >>  	device_unregister(&udc->dev);
>> >>  	device_unregister(&gadget->dev);
>> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>> >>  }
>> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >
>> > Isn't this dangerous?  It's quite possible that the device_unregister() 
>> 
>> not on the gadget API, no.
>> 
>> > call on the previous line invokes the gadget->dev.release callback, 
>> > which might deallocate gadget.  If that happens, your new memset will 
>> > oops.
>> 
>> that won't happen. struct usb_gadget is a member of the UDC's private
>> structure, like this:
>> 
>> struct dwc3 {
>> 	[...]
>> 	struct usb_gadget	gadget;
>> 	struct usb_gadget_driver *gadget_driver;
>> 	[...]
>> };
>
> Yes.  So what?  Can't the UDC driver use the refcount inside struct 
> usb_gadget to control the lifetime of its private structure?

nope, not being used. At least not yet.

> (By the way, can you tell what's going on in net2280.c?  I must be
> missing something; it looks like gadget_release() would quickly run
> into problems because it calls dev_get_drvdata() for &gadget->dev, but
> net2280_probe() never calls dev_set_drvdata() for that device.  
> Furthermore, net2280_remove() continues to reference the net2280 struct
> after calling usb_del_gadget_udc(), and it never does seem to do a
> final put.)

static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
	struct net2280		*dev;
	unsigned long		resource, len;
	void			__iomem *base = NULL;
	int			retval, i;

	/* alloc, and start init */
	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
	if (dev == NULL) {
		retval = -ENOMEM;
		goto done;
	}

	pci_set_drvdata(pdev, dev);
	^^^^^^^^^^^^^^^^^^^^^^^^^^^

>> I'm actually thinking that struct usb_gadget shouldn't have a struct
>> device at all. Just a pointer to a device, that would solve all these
>> issues.
>
> A pointer to which device?  The UDC?  That would change the directory 
> layout in sysfs.

indeed. Would that be a problem?

> Or a pointer to a separate dynamically allocated device (the way struct 
> usb_hcd contains a pointer to the root hub device)?  That would work.  
> If the UDC driver wanted to re-register the gadget, it would have to 
> allocate a new device.

That could be done as well, if maintaining the directory structure is a
must.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Wed, 5 Apr 2017, Felipe Balbi wrote:

> >> >> --- a/drivers/usb/gadget/udc/core.c
> >> >> +++ b/drivers/usb/gadget/udc/core.c
> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> >> >>  	flush_work(&gadget->work);
> >> >>  	device_unregister(&udc->dev);
> >> >>  	device_unregister(&gadget->dev);
> >> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
> >> >>  }
> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >> >
> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
> >> 
> >> not on the gadget API, no.
> >> 
> >> > call on the previous line invokes the gadget->dev.release callback, 
> >> > which might deallocate gadget.  If that happens, your new memset will 
> >> > oops.
> >> 
> >> that won't happen. struct usb_gadget is a member of the UDC's private
> >> structure, like this:
> >> 
> >> struct dwc3 {
> >> 	[...]
> >> 	struct usb_gadget	gadget;
> >> 	struct usb_gadget_driver *gadget_driver;
> >> 	[...]
> >> };
> >
> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
> > usb_gadget to control the lifetime of its private structure?
> 
> nope, not being used. At least not yet.

I'm not convinced (yet)...

> > (By the way, can you tell what's going on in net2280.c?  I must be
> > missing something; it looks like gadget_release() would quickly run
> > into problems because it calls dev_get_drvdata() for &gadget->dev, but
> > net2280_probe() never calls dev_set_drvdata() for that device.  
> > Furthermore, net2280_remove() continues to reference the net2280 struct
> > after calling usb_del_gadget_udc(), and it never does seem to do a
> > final put.)
> 
> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> 	struct net2280		*dev;
> 	unsigned long		resource, len;
> 	void			__iomem *base = NULL;
> 	int			retval, i;
> 
> 	/* alloc, and start init */
> 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> 	if (dev == NULL) {
> 		retval = -ENOMEM;
> 		goto done;
> 	}
> 
> 	pci_set_drvdata(pdev, dev);
> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^

That sets the driver data in the struct pci_dev, not in
dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
driver data in dev->gadget.dev.

(Even after all these years, I still get bothered by the way Dave 
Brownell used to call everything "dev"...  IIRC, at one time he had a 
line of code that went something like:  dev->dev.dev = &pdev->dev !)

> >> I'm actually thinking that struct usb_gadget shouldn't have a struct
> >> device at all. Just a pointer to a device, that would solve all these
> >> issues.
> >
> > A pointer to which device?  The UDC?  That would change the directory 
> > layout in sysfs.
> 
> indeed. Would that be a problem?

Possibly for some userspace tool.

> > Or a pointer to a separate dynamically allocated device (the way struct 
> > usb_hcd contains a pointer to the root hub device)?  That would work.  
> > If the UDC driver wanted to re-register the gadget, it would have to 
> > allocate a new device.
> 
> That could be done as well, if maintaining the directory structure is a
> must.

Alan Stern

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3471 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Wed, 5 Apr 2017, Felipe Balbi wrote:
>
>> >> >> --- a/drivers/usb/gadget/udc/core.c
>> >> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>> >> >>  	flush_work(&gadget->work);
>> >> >>  	device_unregister(&udc->dev);
>> >> >>  	device_unregister(&gadget->dev);
>> >> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>> >> >>  }
>> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >> >
>> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
>> >> 
>> >> not on the gadget API, no.
>> >> 
>> >> > call on the previous line invokes the gadget->dev.release callback, 
>> >> > which might deallocate gadget.  If that happens, your new memset will 
>> >> > oops.
>> >> 
>> >> that won't happen. struct usb_gadget is a member of the UDC's private
>> >> structure, like this:
>> >> 
>> >> struct dwc3 {
>> >> 	[...]
>> >> 	struct usb_gadget	gadget;
>> >> 	struct usb_gadget_driver *gadget_driver;
>> >> 	[...]
>> >> };
>> >
>> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
>> > usb_gadget to control the lifetime of its private structure?
>> 
>> nope, not being used. At least not yet.
>
> I'm not convinced (yet)...
>
>> > (By the way, can you tell what's going on in net2280.c?  I must be
>> > missing something; it looks like gadget_release() would quickly run
>> > into problems because it calls dev_get_drvdata() for &gadget->dev, but
>> > net2280_probe() never calls dev_set_drvdata() for that device.  
>> > Furthermore, net2280_remove() continues to reference the net2280 struct
>> > after calling usb_del_gadget_udc(), and it never does seem to do a
>> > final put.)
>> 
>> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> {
>> 	struct net2280		*dev;
>> 	unsigned long		resource, len;
>> 	void			__iomem *base = NULL;
>> 	int			retval, i;
>> 
>> 	/* alloc, and start init */
>> 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> 	if (dev == NULL) {
>> 		retval = -ENOMEM;
>> 		goto done;
>> 	}
>> 
>> 	pci_set_drvdata(pdev, dev);
>> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> That sets the driver data in the struct pci_dev, not in
> dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
> driver data in dev->gadget.dev.

hmmm, indeed. The same is happening with other callers of
usb_add_gadget_udc_release().

I guess this should be enough?

@@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
 
 static void gadget_release(struct device *_dev)
 {
-	struct net2280	*dev = dev_get_drvdata(_dev);
+	struct net2280	*dev = dev_get_drvdata(_dev->parent);
 
 	kfree(dev);
 }


> (Even after all these years, I still get bothered by the way Dave 
> Brownell used to call everything "dev"...  IIRC, at one time he had a 
> line of code that went something like:  dev->dev.dev = &pdev->dev !)

:-)

>> >> I'm actually thinking that struct usb_gadget shouldn't have a struct
>> >> device at all. Just a pointer to a device, that would solve all these
>> >> issues.
>> >
>> > A pointer to which device?  The UDC?  That would change the directory 
>> > layout in sysfs.
>> 
>> indeed. Would that be a problem?
>
> Possibly for some userspace tool.

yeah, we can do dynamic allocation of the device pointer, no issue.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Mon, 10 Apr 2017, Felipe Balbi wrote:

> Hi,
> 
> Alan Stern <stern@rowland.harvard.edu> writes:
> > On Wed, 5 Apr 2017, Felipe Balbi wrote:
> >
> >> >> >> --- a/drivers/usb/gadget/udc/core.c
> >> >> >> +++ b/drivers/usb/gadget/udc/core.c
> >> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
> >> >> >>  	flush_work(&gadget->work);
> >> >> >>  	device_unregister(&udc->dev);
> >> >> >>  	device_unregister(&gadget->dev);
> >> >> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
> >> >> >>  }
> >> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> >> >> >
> >> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
> >> >> 
> >> >> not on the gadget API, no.
> >> >> 
> >> >> > call on the previous line invokes the gadget->dev.release callback, 
> >> >> > which might deallocate gadget.  If that happens, your new memset will 
> >> >> > oops.
> >> >> 
> >> >> that won't happen. struct usb_gadget is a member of the UDC's private
> >> >> structure, like this:
> >> >> 
> >> >> struct dwc3 {
> >> >> 	[...]
> >> >> 	struct usb_gadget	gadget;
> >> >> 	struct usb_gadget_driver *gadget_driver;
> >> >> 	[...]
> >> >> };
> >> >
> >> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
> >> > usb_gadget to control the lifetime of its private structure?
> >> 
> >> nope, not being used. At least not yet.
> >
> > I'm not convinced (yet)...
> >
> >> > (By the way, can you tell what's going on in net2280.c?  I must be
> >> > missing something; it looks like gadget_release() would quickly run
> >> > into problems because it calls dev_get_drvdata() for &gadget->dev, but
> >> > net2280_probe() never calls dev_set_drvdata() for that device.  
> >> > Furthermore, net2280_remove() continues to reference the net2280 struct
> >> > after calling usb_del_gadget_udc(), and it never does seem to do a
> >> > final put.)
> >> 
> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >> {
> >> 	struct net2280		*dev;
> >> 	unsigned long		resource, len;
> >> 	void			__iomem *base = NULL;
> >> 	int			retval, i;
> >> 
> >> 	/* alloc, and start init */
> >> 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >> 	if (dev == NULL) {
> >> 		retval = -ENOMEM;
> >> 		goto done;
> >> 	}
> >> 
> >> 	pci_set_drvdata(pdev, dev);
> >> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > That sets the driver data in the struct pci_dev, not in
> > dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
> > driver data in dev->gadget.dev.
> 
> hmmm, indeed. The same is happening with other callers of
> usb_add_gadget_udc_release().
> 
> I guess this should be enough?
> 
> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>  
>  static void gadget_release(struct device *_dev)
>  {
> -	struct net2280	*dev = dev_get_drvdata(_dev);
> +	struct net2280	*dev = dev_get_drvdata(_dev->parent);
>  
>  	kfree(dev);
>  }

Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
up every time it gets called, in its current form.

And it doesn't help with the fact that net2280_remove() continues to 
access the private data structure after calling usb_del_gadget_udc().  
Strictly speaking, that routine should do

	get_device(&dev->gadget.dev);

at the start, with a corresponding put_device() at the end.

There's another problem.  Suppose a call to 
usb_add_gadget_udc_release() fails.  At the end of that routine, the 
error pathway does put_device(&gadget->dev).  This will invoke the 
release callback, deallocating the private data structure without 
giving the caller (i.e., the UDC driver) a chance to clean up.

Alan Stern

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5229 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> >> >> >> --- a/drivers/usb/gadget/udc/core.c
>> >> >> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>> >> >> >>  	flush_work(&gadget->work);
>> >> >> >>  	device_unregister(&udc->dev);
>> >> >> >>  	device_unregister(&gadget->dev);
>> >> >> >> +	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>> >> >> >>  }
>> >> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >> >> >
>> >> >> > Isn't this dangerous?  It's quite possible that the device_unregister() 
>> >> >> 
>> >> >> not on the gadget API, no.
>> >> >> 
>> >> >> > call on the previous line invokes the gadget->dev.release callback, 
>> >> >> > which might deallocate gadget.  If that happens, your new memset will 
>> >> >> > oops.
>> >> >> 
>> >> >> that won't happen. struct usb_gadget is a member of the UDC's private
>> >> >> structure, like this:
>> >> >> 
>> >> >> struct dwc3 {
>> >> >> 	[...]
>> >> >> 	struct usb_gadget	gadget;
>> >> >> 	struct usb_gadget_driver *gadget_driver;
>> >> >> 	[...]
>> >> >> };
>> >> >
>> >> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
>> >> > usb_gadget to control the lifetime of its private structure?
>> >> 
>> >> nope, not being used. At least not yet.
>> >
>> > I'm not convinced (yet)...
>> >
>> >> > (By the way, can you tell what's going on in net2280.c?  I must be
>> >> > missing something; it looks like gadget_release() would quickly run
>> >> > into problems because it calls dev_get_drvdata() for &gadget->dev, but
>> >> > net2280_probe() never calls dev_set_drvdata() for that device.  
>> >> > Furthermore, net2280_remove() continues to reference the net2280 struct
>> >> > after calling usb_del_gadget_udc(), and it never does seem to do a
>> >> > final put.)
>> >> 
>> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> >> {
>> >> 	struct net2280		*dev;
>> >> 	unsigned long		resource, len;
>> >> 	void			__iomem *base = NULL;
>> >> 	int			retval, i;
>> >> 
>> >> 	/* alloc, and start init */
>> >> 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> >> 	if (dev == NULL) {
>> >> 		retval = -ENOMEM;
>> >> 		goto done;
>> >> 	}
>> >> 
>> >> 	pci_set_drvdata(pdev, dev);
>> >> 	^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >
>> > That sets the driver data in the struct pci_dev, not in
>> > dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
>> > driver data in dev->gadget.dev.
>> 
>> hmmm, indeed. The same is happening with other callers of
>> usb_add_gadget_udc_release().
>> 
>> I guess this should be enough?
>> 
>> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>>  
>>  static void gadget_release(struct device *_dev)
>>  {
>> -	struct net2280	*dev = dev_get_drvdata(_dev);
>> +	struct net2280	*dev = dev_get_drvdata(_dev->parent);
>>  
>>  	kfree(dev);
>>  }
>
> Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> up every time it gets called, in its current form.

Well, it does :-)

dev_get_drvdata(_dev) -> NULL -> kfree(NULL)

We're just leaking memory. I guess a patch like below would be best:

diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index 3828c2ec8623..4dc04253da61 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
 
 /*-------------------------------------------------------------------------*/
 
-static void gadget_release(struct device *_dev)
-{
-	struct net2280	*dev = dev_get_drvdata(_dev);
-
-	kfree(dev);
-}
-
 /* tear down the binding between this driver and the pci device */
 
 static void net2280_remove(struct pci_dev *pdev)
@@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
 	device_remove_file(&pdev->dev, &dev_attr_registers);
 
 	ep_info(dev, "unbind\n");
+
+	kfree(dev);
 }
 
 /* wrap this driver around the specified device, but
@@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (retval)
 		goto done;
 
-	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
-			gadget_release);
+	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
 	if (retval)
 		goto done;
 	return 0;

> And it doesn't help with the fact that net2280_remove() continues to 
> access the private data structure after calling usb_del_gadget_udc().  
> Strictly speaking, that routine should do
>
> 	get_device(&dev->gadget.dev);
>
> at the start, with a corresponding put_device() at the end.
>
> There's another problem.  Suppose a call to 
> usb_add_gadget_udc_release() fails.  At the end of that routine, the 
> error pathway does put_device(&gadget->dev).  This will invoke the 
> release callback, deallocating the private data structure without 
> giving the caller (i.e., the UDC driver) a chance to clean up.

it won't deallocate anything :-) dev_set_drvdata() was never called,
we will endup with kfree(NULL) which is safe and just silently returns.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Tue, 11 Apr 2017, Felipe Balbi wrote:

> > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> > up every time it gets called, in its current form.
> 
> Well, it does :-)
> 
> dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
> 
> We're just leaking memory. I guess a patch like below would be best:
> 
> diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
> index 3828c2ec8623..4dc04253da61 100644
> --- a/drivers/usb/gadget/udc/net2280.c
> +++ b/drivers/usb/gadget/udc/net2280.c
> @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>  
>  /*-------------------------------------------------------------------------*/
>  
> -static void gadget_release(struct device *_dev)
> -{
> -	struct net2280	*dev = dev_get_drvdata(_dev);
> -
> -	kfree(dev);
> -}
> -
>  /* tear down the binding between this driver and the pci device */
>  
>  static void net2280_remove(struct pci_dev *pdev)
> @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
>  	device_remove_file(&pdev->dev, &dev_attr_registers);
>  
>  	ep_info(dev, "unbind\n");
> +
> +	kfree(dev);
>  }
>  
>  /* wrap this driver around the specified device, but
> @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (retval)
>  		goto done;
>  
> -	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
> -			gadget_release);
> +	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
>  	if (retval)
>  		goto done;
>  	return 0;

Maybe...  But I can't shake the feeling that Greg KH would strongly 
disagree.  Hasn't he said, many times in the past, that any dynamically 
allocated device structure _must_ have a real release routine?  
usb_udc_nop_release() doesn't qualify.

The issue is outstanding references to gadget.dev.  The driver core
does not guarantee that all references will have been dropped by the
time device_unregister() returns.  So what if some other part of the
kernel still has a reference to gadget.dev when we reach the end of
net2280_remove() and deallocate the private structure?

Unless you can be certain that there are no outstanding references when
the gadget is unregistered, this approach isn't safe.  (And even if you
can be certain, how can you know that future changes to the kernel
won't affect the situation?)

> > And it doesn't help with the fact that net2280_remove() continues to 
> > access the private data structure after calling usb_del_gadget_udc().  
> > Strictly speaking, that routine should do
> >
> > 	get_device(&dev->gadget.dev);
> >
> > at the start, with a corresponding put_device() at the end.
> >
> > There's another problem.  Suppose a call to 
> > usb_add_gadget_udc_release() fails.  At the end of that routine, the 
> > error pathway does put_device(&gadget->dev).  This will invoke the 
> > release callback, deallocating the private data structure without 
> > giving the caller (i.e., the UDC driver) a chance to clean up.
> 
> it won't deallocate anything :-) dev_set_drvdata() was never called,
> we will endup with kfree(NULL) which is safe and just silently returns.

But if you change gadget_release() to use the parent's drvdata field, 
like you proposed earlier, and fix up the refcounting in 
net2280_remove() so that the structure doesn't get deallocated too 
quickly, then this does become a real problem.

And it can affect other UDC drivers, not just net2280.

Alan Stern

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Greg KH]

On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
> On Tue, 11 Apr 2017, Felipe Balbi wrote:
> 
> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> > > up every time it gets called, in its current form.
> > 
> > Well, it does :-)
> > 
> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
> > 
> > We're just leaking memory. I guess a patch like below would be best:
> > 
> > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
> > index 3828c2ec8623..4dc04253da61 100644
> > --- a/drivers/usb/gadget/udc/net2280.c
> > +++ b/drivers/usb/gadget/udc/net2280.c
> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
> >  
> >  /*-------------------------------------------------------------------------*/
> >  
> > -static void gadget_release(struct device *_dev)
> > -{
> > -	struct net2280	*dev = dev_get_drvdata(_dev);
> > -
> > -	kfree(dev);
> > -}
> > -
> >  /* tear down the binding between this driver and the pci device */
> >  
> >  static void net2280_remove(struct pci_dev *pdev)
> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
> >  	device_remove_file(&pdev->dev, &dev_attr_registers);
> >  
> >  	ep_info(dev, "unbind\n");
> > +
> > +	kfree(dev);
> >  }
> >  
> >  /* wrap this driver around the specified device, but
> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >  	if (retval)
> >  		goto done;
> >  
> > -	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
> > -			gadget_release);
> > +	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
> >  	if (retval)
> >  		goto done;
> >  	return 0;
> 
> Maybe...  But I can't shake the feeling that Greg KH would strongly 
> disagree.  Hasn't he said, many times in the past, that any dynamically 
> allocated device structure _must_ have a real release routine?  
> usb_udc_nop_release() doesn't qualify.

Aw, I wanted to publically yell at someone like the kernel documentation
says I am allowed to do so if anyone does such a foolish thing :)

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]


Hi,

Greg KH <greg@kroah.com> writes:
> On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
>> On Tue, 11 Apr 2017, Felipe Balbi wrote:
>> 
>> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
>> > > up every time it gets called, in its current form.
>> > 
>> > Well, it does :-)
>> > 
>> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
>> > 
>> > We're just leaking memory. I guess a patch like below would be best:
>> > 
>> > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
>> > index 3828c2ec8623..4dc04253da61 100644
>> > --- a/drivers/usb/gadget/udc/net2280.c
>> > +++ b/drivers/usb/gadget/udc/net2280.c
>> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>> >  
>> >  /*-------------------------------------------------------------------------*/
>> >  
>> > -static void gadget_release(struct device *_dev)
>> > -{
>> > -	struct net2280	*dev = dev_get_drvdata(_dev);
>> > -
>> > -	kfree(dev);
>> > -}
>> > -
>> >  /* tear down the binding between this driver and the pci device */
>> >  
>> >  static void net2280_remove(struct pci_dev *pdev)
>> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
>> >  	device_remove_file(&pdev->dev, &dev_attr_registers);
>> >  
>> >  	ep_info(dev, "unbind\n");
>> > +
>> > +	kfree(dev);
>> >  }
>> >  
>> >  /* wrap this driver around the specified device, but
>> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> >  	if (retval)
>> >  		goto done;
>> >  
>> > -	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
>> > -			gadget_release);
>> > +	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
>> >  	if (retval)
>> >  		goto done;
>> >  	return 0;
>> 
>> Maybe...  But I can't shake the feeling that Greg KH would strongly 
>> disagree.  Hasn't he said, many times in the past, that any dynamically 
>> allocated device structure _must_ have a real release routine?  
>> usb_udc_nop_release() doesn't qualify.
>
> Aw, I wanted to publically yell at someone like the kernel documentation
> says I am allowed to do so if anyone does such a foolish thing :)

heh, except that we're not dynamically allocating struct device at all
:-) Here's what we have for most UDCs (net2280.c included):

	struct my_udc {
        	struct gadget gadget;
                [...]
	};

	probe()
        {
        	struct my_udc *u;

		u = kzalloc(sizeof(*u), GFP_KERNEL);
                [...]
		return 0;
	}

Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
this result on a functionally equivalent execution to the patch I
proposed above?

Iff we change struct gadget to contain a struct device *dev instead of a
struct device dev, then sure, we will need to cope with proper
->release() implementations.

As it is, it brings nothing to the table, IMO.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Greg KH]

On Wed, Apr 12, 2017 at 09:01:44AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Greg KH <greg@kroah.com> writes:
> > On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
> >> On Tue, 11 Apr 2017, Felipe Balbi wrote:
> >> 
> >> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> >> > > up every time it gets called, in its current form.
> >> > 
> >> > Well, it does :-)
> >> > 
> >> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
> >> > 
> >> > We're just leaking memory. I guess a patch like below would be best:
> >> > 
> >> > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
> >> > index 3828c2ec8623..4dc04253da61 100644
> >> > --- a/drivers/usb/gadget/udc/net2280.c
> >> > +++ b/drivers/usb/gadget/udc/net2280.c
> >> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
> >> >  
> >> >  /*-------------------------------------------------------------------------*/
> >> >  
> >> > -static void gadget_release(struct device *_dev)
> >> > -{
> >> > -	struct net2280	*dev = dev_get_drvdata(_dev);
> >> > -
> >> > -	kfree(dev);
> >> > -}
> >> > -
> >> >  /* tear down the binding between this driver and the pci device */
> >> >  
> >> >  static void net2280_remove(struct pci_dev *pdev)
> >> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
> >> >  	device_remove_file(&pdev->dev, &dev_attr_registers);
> >> >  
> >> >  	ep_info(dev, "unbind\n");
> >> > +
> >> > +	kfree(dev);
> >> >  }
> >> >  
> >> >  /* wrap this driver around the specified device, but
> >> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >> >  	if (retval)
> >> >  		goto done;
> >> >  
> >> > -	retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
> >> > -			gadget_release);
> >> > +	retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
> >> >  	if (retval)
> >> >  		goto done;
> >> >  	return 0;
> >> 
> >> Maybe...  But I can't shake the feeling that Greg KH would strongly 
> >> disagree.  Hasn't he said, many times in the past, that any dynamically 
> >> allocated device structure _must_ have a real release routine?  
> >> usb_udc_nop_release() doesn't qualify.
> >
> > Aw, I wanted to publically yell at someone like the kernel documentation
> > says I am allowed to do so if anyone does such a foolish thing :)
> 
> heh, except that we're not dynamically allocating struct device at all
> :-)

Please don't say that, that's even worse :(

> Here's what we have for most UDCs (net2280.c included):
> 
> 	struct my_udc {
>         	struct gadget gadget;
>                 [...]
> 	};
> 
> 	probe()
>         {
>         	struct my_udc *u;
> 
> 		u = kzalloc(sizeof(*u), GFP_KERNEL);
>                 [...]
> 		return 0;
> 	}
> 
> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
> this result on a functionally equivalent execution to the patch I
> proposed above?
> 
> Iff we change struct gadget to contain a struct device *dev instead of a
> struct device dev, then sure, we will need to cope with proper
> ->release() implementations.
> 
> As it is, it brings nothing to the table, IMO.

You always have to have a release function for a kobject, no matter
where it is, as it is being reference counted.  To not do so, is a huge
indication of a problem in the design.

thanks,

greg k-h

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]


Hi,

Greg KH <greg@kroah.com> writes:
>> Here's what we have for most UDCs (net2280.c included):
>> 
>> 	struct my_udc {
>>         	struct gadget gadget;
>>                 [...]
>> 	};
>> 
>> 	probe()
>>         {
>>         	struct my_udc *u;
>> 
>> 		u = kzalloc(sizeof(*u), GFP_KERNEL);
>>                 [...]
>> 		return 0;
>> 	}
>> 
>> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
>> this result on a functionally equivalent execution to the patch I
>> proposed above?
>> 
>> Iff we change struct gadget to contain a struct device *dev instead of a
>> struct device dev, then sure, we will need to cope with proper
>> ->release() implementations.
>> 
>> As it is, it brings nothing to the table, IMO.
>
> You always have to have a release function for a kobject, no matter
> where it is, as it is being reference counted.  To not do so, is a huge
> indication of a problem in the design.

okay, this goes all the way back to when Dave B wrote the API, it has
always had empty ->release() functions (not all UDCs, though). I'll make
sure to review all of this for v4.13.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Alan Stern]

On Wed, 12 Apr 2017, Felipe Balbi wrote:

> >> Maybe...  But I can't shake the feeling that Greg KH would strongly 
> >> disagree.  Hasn't he said, many times in the past, that any dynamically 
> >> allocated device structure _must_ have a real release routine?  
> >> usb_udc_nop_release() doesn't qualify.
> >
> > Aw, I wanted to publically yell at someone like the kernel documentation
> > says I am allowed to do so if anyone does such a foolish thing :)
> 
> heh, except that we're not dynamically allocating struct device at all
> :-) Here's what we have for most UDCs (net2280.c included):
> 
> 	struct my_udc {
>         	struct gadget gadget;
>                 [...]
> 	};
> 
> 	probe()
>         {
>         	struct my_udc *u;
> 
> 		u = kzalloc(sizeof(*u), GFP_KERNEL);
>                 [...]
> 		return 0;
> 	}

Allow me to point out that the struct device is embedded inside the
struct gadget (actually struct usb_gadget) embedded inside the struct
my_udc, which _is_ dynamically allocated.  Therefore the struct device
is located in dynamically allocated memory.

> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't
> this result on a functionally equivalent execution to the patch I
> proposed above?

It would, and it would be equally wrong.

Alan Stern