Groups > linux.kernel > #1225208 > unrolled thread

[PATCH 00/26] usb: gadget: encapsulate ep enable/disable

Started by	Robert Baldyga <r.baldyga@samsung.com>
First post	2015-09-15 16:30 +0200
Last post	2015-09-15 16:50 +0200
Articles	16 — 3 participants

Back to article view | Back to linux.kernel

  [PATCH 00/26] usb: gadget: encapsulate ep enable/disable Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 16:30 +0200
    [PATCH 20/26] usb: gadget: f_uac1: eliminate abuse of ep->driver data Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 16:40 +0200
    [PATCH 21/26] usb: gadget: f_uac2: eliminate abuse of ep->driver data Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 16:40 +0200
    [PATCH 13/26] usb: gadget: f_obex: eliminate abuse of ep->driver data Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 16:40 +0200
    [PATCH 23/26] usb: gadget: u_ether: eliminate abuse of ep->driver data Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 16:40 +0200
    [PATCH 25/26] usb: gadget: legacy: dbgp: eliminate abuse of ep->driver  data Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 16:40 +0200
    [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 16:50 +0200
      Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct  usb_ep Krzysztof Opasiak <k.opasiak@samsung.com> - 2015-09-15 17:40 +0200
        Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct  usb_ep Felipe Balbi <balbi@ti.com> - 2015-09-15 17:50 +0200
          Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct  usb_ep Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 18:00 +0200
            Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct  usb_ep Felipe Balbi <balbi@ti.com> - 2015-09-15 18:10 +0200
          Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct  usb_ep Krzysztof Opasiak <k.opasiak@samsung.com> - 2015-09-15 18:20 +0200
            Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct  usb_ep Felipe Balbi <balbi@ti.com> - 2015-09-15 18:40 +0200
    [PATCH 02/26] usb: gadget: f_ncm: obtain cdev from function instead of  driver_data Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 16:50 +0200
    [PATCH 01/26] usb: gadget: fix few outdated comments Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 16:50 +0200
    [PATCH 07/26] usb: gadget: f_eem: eliminate abuse of ep->driver data Robert Baldyga <r.baldyga@samsung.com> - 2015-09-15 16:50 +0200

#1225208 — [PATCH 00/26] usb: gadget: encapsulate ep enable/disable

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 16:30 +0200
Subject	[PATCH 00/26] usb: gadget: encapsulate ep enable/disable
Message-ID	<q8Zp8-7qt-7@gated-at.bofh.it>

Hi Felipe,

There is my next patches series containing few fixes and slightly
reworking USB gadget function API. It introduces ep->enabled flag which
indicates whether endpointis enabled or not, and encapsulates its checking
and setting into usb_ep_enable() and usb_ep_disable() functions. So now
these functions can be used more safely.

Why is that needed?

For example, it's very common pattern in USB functions to re-enable
endpoints in set_alt(). Usually it looks like:

 usb_ep_disable()
 config_ep_by_speed()
 usb_ep_enable()

So far to avoid disabling endpoint which was already disabled there was
need to remember somehow which endpoints are enabled. Unfortunately for
this purpose no new flag in struct usb_ep was introduced, but instead
ep->driver_data was set and cleared to mark endpoints as enabled/disabled.
This made code a little messy considering that the same ep->driver_data
was used before in bind() to claim endpoints obtained from autoconfig,
and moreover many functions use the same ep->driver_data to save some
private pointer, accessible for example in complete() callback (what is,
I believe, driver_data was designed for).

So as now we have ep->claimed flag for marking claimed endpoints, and
ep->enabled flag to remember its enabled/disabled state, we can finally
use ep->driver_data to only contain pointer to private data.

To achieve this, this patch set modifies all USB functions where abuse
ep->driver_data had place, and leaves them in form, where internal endpoint
state is handled by gadget framework functions, and ep->driver_data has
no additional purpose over containing driver data pointer.

Best regards,
Robert Baldyga

Robert Baldyga (26):
  usb: gadget: fix few outdated comments
  usb: gadget: f_ncm: obtain cdev from function instead of driver_data
  usb: gadget: epautoconf: add usb_ep_autoconfig_release() function
  usb: gadget: introduce 'enabled' flag in struct usb_ep
  usb: gadget: f_ecm: eliminate abuse of ep->driver data
  usb: gadget: f_acm: eliminate abuse of ep->driver data
  usb: gadget: f_eem: eliminate abuse of ep->driver data
  usb: gadget: f_hid: eliminate abuse of ep->driver data
  usb: gadget: f_loopback: eliminate abuse of ep->driver data
  usb: gadget: f_mass_storage: eliminate abuse of ep->driver data
  usb: gadget: f_midi: eliminate abuse of ep->driver data
  usb: gadget: f_ncm: eliminate abuse of ep->driver data
  usb: gadget: f_obex: eliminate abuse of ep->driver data
  usb: gadget: f_phonet: eliminate abuse of ep->driver data
  usb: gadget: f_printer: eliminate abuse of ep->driver data
  usb: gadget: f_rndis: eliminate abuse of ep->driver data
  usb: gadget: f_serial: eliminate abuse of ep->driver data
  usb: gadget: f_sourcesink: eliminate abuse of ep->driver data
  usb: gadget: f_subset: eliminate abuse of ep->driver data
  usb: gadget: f_uac1: eliminate abuse of ep->driver data
  usb: gadget: f_uac2: eliminate abuse of ep->driver data
  usb: gadget: f_uvc: eliminate abuse of ep->driver data
  usb: gadget: u_ether: eliminate abuse of ep->driver data
  usb: gadget: u_serial: eliminate abuse of ep->driver data
  usb: gadget: legacy: dbgp: eliminate abuse of ep->driver data
  usb: gadget: legacy: tcm: eliminate abuse of ep->driver data

 drivers/usb/gadget/composite.c               |  4 +--
 drivers/usb/gadget/epautoconf.c              | 25 ++++++++++++++---
 drivers/usb/gadget/function/f_acm.c          | 23 +++------------
 drivers/usb/gadget/function/f_ecm.c          | 31 +++++---------------
 drivers/usb/gadget/function/f_eem.c          | 16 ++---------
 drivers/usb/gadget/function/f_hid.c          | 12 ++------
 drivers/usb/gadget/function/f_loopback.c     |  5 +---
 drivers/usb/gadget/function/f_mass_storage.c |  4 ---
 drivers/usb/gadget/function/f_midi.c         | 14 ++--------
 drivers/usb/gadget/function/f_ncm.c          | 29 +++++--------------
 drivers/usb/gadget/function/f_obex.c         | 10 +------
 drivers/usb/gadget/function/f_phonet.c       |  8 +-----
 drivers/usb/gadget/function/f_printer.c      |  2 --
 drivers/usb/gadget/function/f_rndis.c        | 24 ++++------------
 drivers/usb/gadget/function/f_serial.c       | 10 +------
 drivers/usb/gadget/function/f_sourcesink.c   | 29 +++++--------------
 drivers/usb/gadget/function/f_subset.c       | 10 +------
 drivers/usb/gadget/function/f_uac1.c         |  4 ---
 drivers/usb/gadget/function/f_uac2.c         | 11 --------
 drivers/usb/gadget/function/f_uvc.c          | 42 ++++++----------------------
 drivers/usb/gadget/function/u_ether.c        |  2 --
 drivers/usb/gadget/function/u_serial.c       |  5 ----
 drivers/usb/gadget/legacy/dbgp.c             | 18 ++----------
 drivers/usb/gadget/legacy/tcm_usb_gadget.c   | 18 ------------
 include/linux/usb/gadget.h                   | 23 +++++++++++++--
 25 files changed, 95 insertions(+), 284 deletions(-)

-- 
1.9.1

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

[toc] | [next] | [standalone]

#1225212 — [PATCH 20/26] usb: gadget: f_uac1: eliminate abuse of ep->driver data

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 16:40 +0200
Subject	[PATCH 20/26] usb: gadget: f_uac1: eliminate abuse of ep->driver data
Message-ID	<q8ZyN-7BR-7@gated-at.bofh.it>
In reply to	#1225208

Since ep->driver_data is not used for endpoint claiming, neither for
enabled/disabled state storing, we can reduce number of places where
we read or modify it's value, as now it has no particular meaning for
function or framework logic.

In case of f_uac1, ep->driver_data was used only for endpoint claiming
and marking endpoints as enabled, so we can simplify code by reducing
it.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_uac1.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac1.c b/drivers/usb/gadget/function/f_uac1.c
index 7856b33..8ee7019 100644
--- a/drivers/usb/gadget/function/f_uac1.c
+++ b/drivers/usb/gadget/function/f_uac1.c
@@ -593,7 +593,6 @@ static int f_audio_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 				return err;
 
 			usb_ep_enable(out_ep);
-			out_ep->driver_data = audio;
 			audio->copy_buf = f_audio_buffer_alloc(audio_buf_size);
 			if (IS_ERR(audio->copy_buf))
 				return -ENOMEM;
@@ -718,7 +717,6 @@ f_audio_bind(struct usb_configuration *c, struct usb_function *f)
 		goto fail;
 	audio->out_ep = ep;
 	audio->out_ep->desc = &as_out_ep_desc;
-	ep->driver_data = cdev;	/* claim */
 
 	status = -ENOMEM;
 
@@ -730,8 +728,6 @@ f_audio_bind(struct usb_configuration *c, struct usb_function *f)
 
 fail:
 	gaudio_cleanup(&audio->card);
-	if (ep)
-		ep->driver_data = NULL;
 	return status;
 }
 
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1225219 — [PATCH 21/26] usb: gadget: f_uac2: eliminate abuse of ep->driver data

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 16:40 +0200
Subject	[PATCH 21/26] usb: gadget: f_uac2: eliminate abuse of ep->driver data
Message-ID	<q8ZyO-7BR-21@gated-at.bofh.it>
In reply to	#1225208

Since ep->driver_data is not used for endpoint claiming, neither for
enabled/disabled state storing, we can reduce number of places where
we read or modify it's value, as now it has no particular meaning for
function or framework logic.

In case of f_uac2, ep->driver_data was used only for endpoint claiming
and marking endpoints as enabled, so we can simplify code by reducing
it.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_uac2.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index f8de7ea..63336e26 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -1081,14 +1081,12 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 		dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
 		goto err;
 	}
-	agdev->out_ep->driver_data = agdev;
 
 	agdev->in_ep = usb_ep_autoconfig(gadget, &fs_epin_desc);
 	if (!agdev->in_ep) {
 		dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
 		goto err;
 	}
-	agdev->in_ep->driver_data = agdev;
 
 	uac2->p_prm.uac2 = uac2;
 	uac2->c_prm.uac2 = uac2;
@@ -1132,10 +1130,6 @@ err_free_descs:
 err:
 	kfree(agdev->uac2.p_prm.rbuf);
 	kfree(agdev->uac2.c_prm.rbuf);
-	if (agdev->in_ep)
-		agdev->in_ep->driver_data = NULL;
-	if (agdev->out_ep)
-		agdev->out_ep->driver_data = NULL;
 	return -EINVAL;
 }
 
@@ -1583,11 +1577,6 @@ static void afunc_unbind(struct usb_configuration *c, struct usb_function *f)
 	prm = &agdev->uac2.c_prm;
 	kfree(prm->rbuf);
 	usb_free_all_descriptors(f);
-
-	if (agdev->in_ep)
-		agdev->in_ep->driver_data = NULL;
-	if (agdev->out_ep)
-		agdev->out_ep->driver_data = NULL;
 }
 
 static struct usb_function *afunc_alloc(struct usb_function_instance *fi)
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1225225 — [PATCH 13/26] usb: gadget: f_obex: eliminate abuse of ep->driver data

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 16:40 +0200
Subject	[PATCH 13/26] usb: gadget: f_obex: eliminate abuse of ep->driver data
Message-ID	<q8ZyP-7BR-43@gated-at.bofh.it>
In reply to	#1225208

Since ep->driver_data is not used for endpoint claiming, neither for
enabled/disabled state storing, we can reduce number of places where
we read or modify it's value, as now it has no particular meaning for
function or framework logic.

In case of f_obex, ep->driver_data was used only for endpoint claiming
and marking endpoints as enabled, so we can simplify code by reducing
it.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_obex.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/function/f_obex.c b/drivers/usb/gadget/function/f_obex.c
index 5460426..1c3d30a 100644
--- a/drivers/usb/gadget/function/f_obex.c
+++ b/drivers/usb/gadget/function/f_obex.c
@@ -206,7 +206,7 @@ static int obex_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		if (alt > 1)
 			goto fail;
 
-		if (obex->port.in->driver_data) {
+		if (obex->port.in->enabled) {
 			dev_dbg(&cdev->gadget->dev,
 				"reset obex ttyGS%d\n", obex->port_num);
 			gserial_disconnect(&obex->port);
@@ -348,13 +348,11 @@ static int obex_bind(struct usb_configuration *c, struct usb_function *f)
 	if (!ep)
 		goto fail;
 	obex->port.in = ep;
-	ep->driver_data = cdev;	/* claim */
 
 	ep = usb_ep_autoconfig(cdev->gadget, &obex_fs_ep_out_desc);
 	if (!ep)
 		goto fail;
 	obex->port.out = ep;
-	ep->driver_data = cdev;	/* claim */
 
 	/* support all relevant hardware speeds... we expect that when
 	 * hardware is dual speed, all bulk-capable endpoints work at
@@ -378,12 +376,6 @@ static int obex_bind(struct usb_configuration *c, struct usb_function *f)
 	return 0;
 
 fail:
-	/* we might as well release our claims on endpoints */
-	if (obex->port.out)
-		obex->port.out->driver_data = NULL;
-	if (obex->port.in)
-		obex->port.in->driver_data = NULL;
-
 	ERROR(cdev, "%s/%p: can't bind, err %d\n", f->name, f, status);
 
 	return status;
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1225227 — [PATCH 23/26] usb: gadget: u_ether: eliminate abuse of ep->driver data

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 16:40 +0200
Subject	[PATCH 23/26] usb: gadget: u_ether: eliminate abuse of ep->driver data
Message-ID	<q8ZyO-7BR-39@gated-at.bofh.it>
In reply to	#1225208

Since ep->driver_data is not used for endpoint claiming, neither for
enabled/disabled state storing, we can reduce number of places where
we read or modify it's value, as now it has no particular meaning for
function or framework logic.

In case of u_ether we only need to store in ep->driver_data pointer to
struct eth_dev, as it's used in rx_complete() and tx_complete() callbacks.
All other uses of ep->driver_data are now meaningless and can be safely
removed.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/u_ether.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index f1fd777..eccde2f 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -1144,7 +1144,6 @@ void gether_disconnect(struct gether *link)
 		spin_lock(&dev->req_lock);
 	}
 	spin_unlock(&dev->req_lock);
-	link->in_ep->driver_data = NULL;
 	link->in_ep->desc = NULL;
 
 	usb_ep_disable(link->out_ep);
@@ -1159,7 +1158,6 @@ void gether_disconnect(struct gether *link)
 		spin_lock(&dev->req_lock);
 	}
 	spin_unlock(&dev->req_lock);
-	link->out_ep->driver_data = NULL;
 	link->out_ep->desc = NULL;
 
 	/* finish forgetting about this USB link episode */
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1225234 — [PATCH 25/26] usb: gadget: legacy: dbgp: eliminate abuse of ep->driver data

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 16:40 +0200
Subject	[PATCH 25/26] usb: gadget: legacy: dbgp: eliminate abuse of ep->driver data
Message-ID	<q8ZyP-7BR-51@gated-at.bofh.it>
In reply to	#1225208

Since ep->driver_data is not used for endpoint claiming, neither for
enabled/disabled state storing, we can reduce number of places where
we read or modify it's value, as now it has no particular meaning for
function or framework logic.

In case of dbgp, ep->driver_data was used only for endpoint claiming
and marking endpoints as enabled, so we can simplify code by reducing
it.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/legacy/dbgp.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/gadget/legacy/dbgp.c b/drivers/usb/gadget/legacy/dbgp.c
index 5231a32..99ca3da 100644
--- a/drivers/usb/gadget/legacy/dbgp.c
+++ b/drivers/usb/gadget/legacy/dbgp.c
@@ -79,10 +79,7 @@ static int dbgp_consume(char *buf, unsigned len)
 
 static void __disable_ep(struct usb_ep *ep)
 {
-	if (ep && ep->driver_data == dbgp.gadget) {
-		usb_ep_disable(ep);
-		ep->driver_data = NULL;
-	}
+	usb_ep_disable(ep);
 }
 
 static void dbgp_disable_ep(void)
@@ -171,7 +168,6 @@ static int __enable_ep(struct usb_ep *ep, struct usb_endpoint_descriptor *desc)
 	int err;
 	ep->desc = desc;
 	err = usb_ep_enable(ep);
-	ep->driver_data = dbgp.gadget;
 	return err;
 }
 
@@ -229,8 +225,6 @@ static void dbgp_unbind(struct usb_gadget *gadget)
 		usb_ep_free_request(gadget->ep0, dbgp.req);
 		dbgp.req = NULL;
 	}
-
-	gadget->ep0->driver_data = NULL;
 }
 
 #ifdef CONFIG_USB_G_DBGP_SERIAL
@@ -249,18 +243,15 @@ static int dbgp_configure_endpoints(struct usb_gadget *gadget)
 		goto fail_1;
 	}
 
-	dbgp.i_ep->driver_data = gadget;
 	i_desc.wMaxPacketSize =
 		cpu_to_le16(USB_DEBUG_MAX_PACKET_SIZE);
 
 	dbgp.o_ep = usb_ep_autoconfig(gadget, &o_desc);
 	if (!dbgp.o_ep) {
-		dbgp.i_ep->driver_data = NULL;
 		stp = 2;
-		goto fail_2;
+		goto fail_1;
 	}
 
-	dbgp.o_ep->driver_data = gadget;
 	o_desc.wMaxPacketSize =
 		cpu_to_le16(USB_DEBUG_MAX_PACKET_SIZE);
 
@@ -277,8 +268,6 @@ static int dbgp_configure_endpoints(struct usb_gadget *gadget)
 
 	return 0;
 
-fail_2:
-	dbgp.i_ep->driver_data = NULL;
 fail_1:
 	dev_dbg(&dbgp.gadget->dev, "ep config: failure (%d)\n", stp);
 	return -ENODEV;
@@ -306,7 +295,6 @@ static int dbgp_bind(struct usb_gadget *gadget,
 	}
 
 	dbgp.req->length = DBGP_REQ_EP0_LEN;
-	gadget->ep0->driver_data = gadget;
 
 #ifdef CONFIG_USB_G_DBGP_SERIAL
 	dbgp.serial = kzalloc(sizeof(struct gserial), GFP_KERNEL);
@@ -356,8 +344,6 @@ static int dbgp_setup(struct usb_gadget *gadget,
 	void *data = NULL;
 	u16 len = 0;
 
-	gadget->ep0->driver_data = gadget;
-
 	if (request == USB_REQ_GET_DESCRIPTOR) {
 		switch (value>>8) {
 		case USB_DT_DEVICE:
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1225237 — [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 16:50 +0200
Subject	[PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep
Message-ID	<q8ZIu-7Nu-1@gated-at.bofh.it>
In reply to	#1225208

This patch introduces 'enabled' flag in struct usb_ep, and modifies
usb_ep_enable() and usb_ep_disable() functions to encapsulate endpoint
enabled/disabled state. It helps to avoid enabling endpoints which are
already enabled, and disabling endpoints which are already disables.

From now USB functions don't have to remember current endpoint
enable/disable state, as this state is now handled automatically which
makes this API less bug-prone.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 include/linux/usb/gadget.h | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 3f299e2..63375cd 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -215,6 +215,7 @@ struct usb_ep {
 	struct list_head	ep_list;
 	struct usb_ep_caps	caps;
 	bool			claimed;
+	bool			enabled;
 	unsigned		maxpacket:16;
 	unsigned		maxpacket_limit:16;
 	unsigned		max_streams:16;
@@ -264,7 +265,15 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep,
  */
 static inline int usb_ep_enable(struct usb_ep *ep)
 {
-	return ep->ops->enable(ep, ep->desc);
+	int ret = 0;
+
+	if (!ep->enabled) {
+		ret = ep->ops->enable(ep, ep->desc);
+		if (!ret)
+			ep->enabled = true;
+	}
+
+	return ret;
 }
 
 /**
@@ -281,7 +290,15 @@ static inline int usb_ep_enable(struct usb_ep *ep)
  */
 static inline int usb_ep_disable(struct usb_ep *ep)
 {
-	return ep->ops->disable(ep);
+	int ret = 0;
+
+	if (ep->enabled) {
+		ret = ep->ops->disable(ep);
+		if (!ret)
+			ep->enabled = false;
+	}
+
+	return ret;
 }
 
 /**
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1225320 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

From	Krzysztof Opasiak <k.opasiak@samsung.com>
Date	2015-09-15 17:40 +0200
Subject	Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep
Message-ID	<q90uR-xv-9@gated-at.bofh.it>
In reply to	#1225237

Hello,

On 09/15/2015 04:26 PM, Robert Baldyga wrote:
> This patch introduces 'enabled' flag in struct usb_ep, and modifies
> usb_ep_enable() and usb_ep_disable() functions to encapsulate endpoint
> enabled/disabled state. It helps to avoid enabling endpoints which are
> already enabled, and disabling endpoints which are already disables.
>
>>From now USB functions don't have to remember current endpoint
> enable/disable state, as this state is now handled automatically which
> makes this API less bug-prone.
>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>   include/linux/usb/gadget.h | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3f299e2..63375cd 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -215,6 +215,7 @@ struct usb_ep {
>   	struct list_head	ep_list;
>   	struct usb_ep_caps	caps;
>   	bool			claimed;
> +	bool			enabled;
>   	unsigned		maxpacket:16;
>   	unsigned		maxpacket_limit:16;
>   	unsigned		max_streams:16;
> @@ -264,7 +265,15 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep,
>    */
>   static inline int usb_ep_enable(struct usb_ep *ep)
>   {
> -	return ep->ops->enable(ep, ep->desc);
> +	int ret = 0;
> +
> +	if (!ep->enabled) {
> +		ret = ep->ops->enable(ep, ep->desc);
> +		if (!ret)
> +			ep->enabled = true;
> +	}
> +
> +	return ret;
>   }
>
>   /**
> @@ -281,7 +290,15 @@ static inline int usb_ep_enable(struct usb_ep *ep)
>    */
>   static inline int usb_ep_disable(struct usb_ep *ep)
>   {
> -	return ep->ops->disable(ep);
> +	int ret = 0;
> +
> +	if (ep->enabled) {
> +		ret = ep->ops->disable(ep);
> +		if (!ret)
> +			ep->enabled = false;
> +	}
> +
> +	return ret;
>   }
>

Personally I don't like this convention. In my opinion usb_ep_disable() 
& usb_ep_enable() should fail if ep is already disabled/enabled. Then in 
function code we should check if endpoint is enabled (maybe even we 
should have usb_ep_is_enabled()) and call disable only when it is really 
enabled.

Best Regards,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225333 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

From	Felipe Balbi <balbi@ti.com>
Date	2015-09-15 17:50 +0200
Subject	Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep
Message-ID	<q90Ex-Jd-7@gated-at.bofh.it>
In reply to	#1225320

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 15, 2015 at 05:37:27PM +0200, Krzysztof Opasiak wrote:
> Hello,
> 
> On 09/15/2015 04:26 PM, Robert Baldyga wrote:
> >This patch introduces 'enabled' flag in struct usb_ep, and modifies
> >usb_ep_enable() and usb_ep_disable() functions to encapsulate endpoint
> >enabled/disabled state. It helps to avoid enabling endpoints which are
> >already enabled, and disabling endpoints which are already disables.
> >
> >>From now USB functions don't have to remember current endpoint
> >enable/disable state, as this state is now handled automatically which
> >makes this API less bug-prone.
> >
> >Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> >---
> >  include/linux/usb/gadget.h | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> >
> >diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> >index 3f299e2..63375cd 100644
> >--- a/include/linux/usb/gadget.h
> >+++ b/include/linux/usb/gadget.h
> >@@ -215,6 +215,7 @@ struct usb_ep {
> >  	struct list_head	ep_list;
> >  	struct usb_ep_caps	caps;
> >  	bool			claimed;
> >+	bool			enabled;
> >  	unsigned		maxpacket:16;
> >  	unsigned		maxpacket_limit:16;
> >  	unsigned		max_streams:16;
> >@@ -264,7 +265,15 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep,
> >   */
> >  static inline int usb_ep_enable(struct usb_ep *ep)
> >  {
> >-	return ep->ops->enable(ep, ep->desc);
> >+	int ret = 0;
> >+
> >+	if (!ep->enabled) {
> >+		ret = ep->ops->enable(ep, ep->desc);
> >+		if (!ret)
> >+			ep->enabled = true;
> >+	}
> >+
> >+	return ret;
> >  }
> >
> >  /**
> >@@ -281,7 +290,15 @@ static inline int usb_ep_enable(struct usb_ep *ep)
> >   */
> >  static inline int usb_ep_disable(struct usb_ep *ep)
> >  {
> >-	return ep->ops->disable(ep);
> >+	int ret = 0;
> >+
> >+	if (ep->enabled) {
> >+		ret = ep->ops->disable(ep);
> >+		if (!ret)
> >+			ep->enabled = false;
> >+	}
> >+
> >+	return ret;
> >  }
> >
> 
> Personally I don't like this convention. In my opinion usb_ep_disable() &
> usb_ep_enable() should fail if ep is already disabled/enabled. Then in
> function code we should check if endpoint is enabled (maybe even we should
> have usb_ep_is_enabled()) and call disable only when it is really enabled.

usb_ep_is_enabled() should be a good addition but I don't see an issue
ignoring usb_ep_enabled() for something that's already enabled.

Imagine if you got an error when you tried to push the light switch to
the 'on' position while the light was already on :-p

I do think, though, that this can be simplified by returning early if
already enabled:

usb_ep_enable()
{
	if (ep->enabled)
		return 0;

	return ep->ops->enable(ep, ep->desc);
}

and likewise for usb_ep_disable()

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1225377 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 18:00 +0200
Subject	Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep
Message-ID	<q90Og-UN-61@gated-at.bofh.it>
In reply to	#1225333

On 09/15/2015 05:43 PM, Felipe Balbi wrote:
> On Tue, Sep 15, 2015 at 05:37:27PM +0200, Krzysztof Opasiak wrote:
>> Hello,
>>
>> On 09/15/2015 04:26 PM, Robert Baldyga wrote:
>>> This patch introduces 'enabled' flag in struct usb_ep, and modifies
>>> usb_ep_enable() and usb_ep_disable() functions to encapsulate endpoint
>>> enabled/disabled state. It helps to avoid enabling endpoints which are
>>> already enabled, and disabling endpoints which are already disables.
>>>
>>> >From now USB functions don't have to remember current endpoint
>>> enable/disable state, as this state is now handled automatically which
>>> makes this API less bug-prone.
>>>
>>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>>> ---
>>>  include/linux/usb/gadget.h | 21 +++++++++++++++++++--
>>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>> index 3f299e2..63375cd 100644
>>> --- a/include/linux/usb/gadget.h
>>> +++ b/include/linux/usb/gadget.h
>>> @@ -215,6 +215,7 @@ struct usb_ep {
>>>  	struct list_head	ep_list;
>>>  	struct usb_ep_caps	caps;
>>>  	bool			claimed;
>>> +	bool			enabled;
>>>  	unsigned		maxpacket:16;
>>>  	unsigned		maxpacket_limit:16;
>>>  	unsigned		max_streams:16;
>>> @@ -264,7 +265,15 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep,
>>>   */
>>>  static inline int usb_ep_enable(struct usb_ep *ep)
>>>  {
>>> -	return ep->ops->enable(ep, ep->desc);
>>> +	int ret = 0;
>>> +
>>> +	if (!ep->enabled) {
>>> +		ret = ep->ops->enable(ep, ep->desc);
>>> +		if (!ret)
>>> +			ep->enabled = true;
>>> +	}
>>> +
>>> +	return ret;
>>>  }
>>>
>>>  /**
>>> @@ -281,7 +290,15 @@ static inline int usb_ep_enable(struct usb_ep *ep)
>>>   */
>>>  static inline int usb_ep_disable(struct usb_ep *ep)
>>>  {
>>> -	return ep->ops->disable(ep);
>>> +	int ret = 0;
>>> +
>>> +	if (ep->enabled) {
>>> +		ret = ep->ops->disable(ep);
>>> +		if (!ret)
>>> +			ep->enabled = false;
>>> +	}
>>> +
>>> +	return ret;
>>>  }
>>>
>>
>> Personally I don't like this convention. In my opinion usb_ep_disable() &
>> usb_ep_enable() should fail if ep is already disabled/enabled. Then in
>> function code we should check if endpoint is enabled (maybe even we should
>> have usb_ep_is_enabled()) and call disable only when it is really enabled.
> 
> usb_ep_is_enabled() should be a good addition but I don't see an issue
> ignoring usb_ep_enabled() for something that's already enabled.
> 
> Imagine if you got an error when you tried to push the light switch to
> the 'on' position while the light was already on :-p
> 
> I do think, though, that this can be simplified by returning early if
> already enabled:
> 
> usb_ep_enable()
> {
> 	if (ep->enabled)
> 		return 0;
> 
> 	return ep->ops->enable(ep, ep->desc);
> }
> 
> and likewise for usb_ep_disable()

We can't do that, because we need to toggle ep->enable flag.

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

[toc] | [prev] | [next] | [standalone]

#1225379 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

From	Felipe Balbi <balbi@ti.com>
Date	2015-09-15 18:10 +0200
Subject	Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep
Message-ID	<q90XT-1l8-3@gated-at.bofh.it>
In reply to	#1225377

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Tue, Sep 15, 2015 at 05:57:53PM +0200, Robert Baldyga wrote:
> On 09/15/2015 05:43 PM, Felipe Balbi wrote:
> > On Tue, Sep 15, 2015 at 05:37:27PM +0200, Krzysztof Opasiak wrote:
> >> Hello,
> >>
> >> On 09/15/2015 04:26 PM, Robert Baldyga wrote:
> >>> This patch introduces 'enabled' flag in struct usb_ep, and modifies
> >>> usb_ep_enable() and usb_ep_disable() functions to encapsulate endpoint
> >>> enabled/disabled state. It helps to avoid enabling endpoints which are
> >>> already enabled, and disabling endpoints which are already disables.
> >>>
> >>> >From now USB functions don't have to remember current endpoint
> >>> enable/disable state, as this state is now handled automatically which
> >>> makes this API less bug-prone.
> >>>
> >>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> >>> ---
> >>>  include/linux/usb/gadget.h | 21 +++++++++++++++++++--
> >>>  1 file changed, 19 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> >>> index 3f299e2..63375cd 100644
> >>> --- a/include/linux/usb/gadget.h
> >>> +++ b/include/linux/usb/gadget.h
> >>> @@ -215,6 +215,7 @@ struct usb_ep {
> >>>  	struct list_head	ep_list;
> >>>  	struct usb_ep_caps	caps;
> >>>  	bool			claimed;
> >>> +	bool			enabled;
> >>>  	unsigned		maxpacket:16;
> >>>  	unsigned		maxpacket_limit:16;
> >>>  	unsigned		max_streams:16;
> >>> @@ -264,7 +265,15 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep,
> >>>   */
> >>>  static inline int usb_ep_enable(struct usb_ep *ep)
> >>>  {
> >>> -	return ep->ops->enable(ep, ep->desc);
> >>> +	int ret = 0;
> >>> +
> >>> +	if (!ep->enabled) {
> >>> +		ret = ep->ops->enable(ep, ep->desc);
> >>> +		if (!ret)
> >>> +			ep->enabled = true;
> >>> +	}
> >>> +
> >>> +	return ret;
> >>>  }
> >>>
> >>>  /**
> >>> @@ -281,7 +290,15 @@ static inline int usb_ep_enable(struct usb_ep *ep)
> >>>   */
> >>>  static inline int usb_ep_disable(struct usb_ep *ep)
> >>>  {
> >>> -	return ep->ops->disable(ep);
> >>> +	int ret = 0;
> >>> +
> >>> +	if (ep->enabled) {
> >>> +		ret = ep->ops->disable(ep);
> >>> +		if (!ret)
> >>> +			ep->enabled = false;
> >>> +	}
> >>> +
> >>> +	return ret;
> >>>  }
> >>>
> >>
> >> Personally I don't like this convention. In my opinion usb_ep_disable() &
> >> usb_ep_enable() should fail if ep is already disabled/enabled. Then in
> >> function code we should check if endpoint is enabled (maybe even we should
> >> have usb_ep_is_enabled()) and call disable only when it is really enabled.
> > 
> > usb_ep_is_enabled() should be a good addition but I don't see an issue
> > ignoring usb_ep_enabled() for something that's already enabled.
> > 
> > Imagine if you got an error when you tried to push the light switch to
> > the 'on' position while the light was already on :-p
> > 
> > I do think, though, that this can be simplified by returning early if
> > already enabled:
> > 
> > usb_ep_enable()
> > {
> > 	if (ep->enabled)
> > 		return 0;
> > 
> > 	return ep->ops->enable(ep, ep->desc);
> > }
> > 
> > and likewise for usb_ep_disable()
> 
> We can't do that, because we need to toggle ep->enable flag.

man, things have to be spelled out to the last comma... The point was to
avoid the extra identation level

usb_ep_enable()
{
	int ret;

	if (ep->enabled)
		return 0;

	ret = ep->ops->enable(ep, ep->desc);
	if (ret)
		return ret;

	ep->enabled = true;

	return 0;
}

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1225389 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

From	Krzysztof Opasiak <k.opasiak@samsung.com>
Date	2015-09-15 18:20 +0200
Subject	Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep
Message-ID	<q917A-1wr-17@gated-at.bofh.it>
In reply to	#1225333


On 09/15/2015 05:43 PM, Felipe Balbi wrote:
> On Tue, Sep 15, 2015 at 05:37:27PM +0200, Krzysztof Opasiak wrote:
>> Hello,
>>
>> On 09/15/2015 04:26 PM, Robert Baldyga wrote:
>>> This patch introduces 'enabled' flag in struct usb_ep, and modifies
>>> usb_ep_enable() and usb_ep_disable() functions to encapsulate endpoint
>>> enabled/disabled state. It helps to avoid enabling endpoints which are
>>> already enabled, and disabling endpoints which are already disables.
>>>
>>> >From now USB functions don't have to remember current endpoint
>>> enable/disable state, as this state is now handled automatically which
>>> makes this API less bug-prone.
>>>
>>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>>> ---
>>>   include/linux/usb/gadget.h | 21 +++++++++++++++++++--
>>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>> index 3f299e2..63375cd 100644
>>> --- a/include/linux/usb/gadget.h
>>> +++ b/include/linux/usb/gadget.h
>>> @@ -215,6 +215,7 @@ struct usb_ep {
>>>   	struct list_head	ep_list;
>>>   	struct usb_ep_caps	caps;
>>>   	bool			claimed;
>>> +	bool			enabled;
>>>   	unsigned		maxpacket:16;
>>>   	unsigned		maxpacket_limit:16;
>>>   	unsigned		max_streams:16;
>>> @@ -264,7 +265,15 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep,
>>>    */
>>>   static inline int usb_ep_enable(struct usb_ep *ep)
>>>   {
>>> -	return ep->ops->enable(ep, ep->desc);
>>> +	int ret = 0;
>>> +
>>> +	if (!ep->enabled) {
>>> +		ret = ep->ops->enable(ep, ep->desc);
>>> +		if (!ret)
>>> +			ep->enabled = true;
>>> +	}
>>> +
>>> +	return ret;
>>>   }
>>>
>>>   /**
>>> @@ -281,7 +290,15 @@ static inline int usb_ep_enable(struct usb_ep *ep)
>>>    */
>>>   static inline int usb_ep_disable(struct usb_ep *ep)
>>>   {
>>> -	return ep->ops->disable(ep);
>>> +	int ret = 0;
>>> +
>>> +	if (ep->enabled) {
>>> +		ret = ep->ops->disable(ep);
>>> +		if (!ret)
>>> +			ep->enabled = false;
>>> +	}
>>> +
>>> +	return ret;
>>>   }
>>>
>>
>> Personally I don't like this convention. In my opinion usb_ep_disable() &
>> usb_ep_enable() should fail if ep is already disabled/enabled. Then in
>> function code we should check if endpoint is enabled (maybe even we should
>> have usb_ep_is_enabled()) and call disable only when it is really enabled.
>
> usb_ep_is_enabled() should be a good addition but I don't see an issue
> ignoring usb_ep_enabled() for something that's already enabled.
>
> Imagine if you got an error when you tried to push the light switch to
> the 'on' position while the light was already on :-p
>

Hmmm not sure right now, didn't test this recently :D as usually I check 
if light isn't already "on" before I touch the switch to turn it on:P

Just joking. Personally I just prefer to don't touch things which are 
already in desired condition. Let's take close() as example which could 
be a little bit equivalent of our usb_ep_disable(). It is not legal to 
call it twice on some fd and second call ends up with error.

Best regards,

-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225403 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

From	Felipe Balbi <balbi@ti.com>
Date	2015-09-15 18:40 +0200
Subject	Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep
Message-ID	<q91qX-1Tn-47@gated-at.bofh.it>
In reply to	#1225389

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Tue, Sep 15, 2015 at 06:15:25PM +0200, Krzysztof Opasiak wrote:
> >>>+	}
> >>>+
> >>>+	return ret;
> >>>  }
> >>>
> >>
> >>Personally I don't like this convention. In my opinion usb_ep_disable() &
> >>usb_ep_enable() should fail if ep is already disabled/enabled. Then in
> >>function code we should check if endpoint is enabled (maybe even we should
> >>have usb_ep_is_enabled()) and call disable only when it is really enabled.
> >
> >usb_ep_is_enabled() should be a good addition but I don't see an issue
> >ignoring usb_ep_enabled() for something that's already enabled.
> >
> >Imagine if you got an error when you tried to push the light switch to
> >the 'on' position while the light was already on :-p
> >
> 
> Hmmm not sure right now, didn't test this recently :D as usually I check if
> light isn't already "on" before I touch the switch to turn it on:P

not my best analogy :)

> Just joking. Personally I just prefer to don't touch things which are
> already in desired condition. Let's take close() as example which
> could be a little bit equivalent of our usb_ep_disable(). It is not
> legal to call it twice on some fd and second call ends up with error.

I understand that, it's just the difference between adding
usb_ep_is_enabled() to every single gadget/function driver or adding it
to usb_ep_enable() itself to keep gadget/function drivers cleaner.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1225238 — [PATCH 02/26] usb: gadget: f_ncm: obtain cdev from function instead of driver_data

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 16:50 +0200
Subject	[PATCH 02/26] usb: gadget: f_ncm: obtain cdev from function instead of driver_data
Message-ID	<q8ZIu-7Nu-3@gated-at.bofh.it>
In reply to	#1225208

The 'driver_data' field in ep0 is never set to pointer to cdev, so we
have to obtain it from another source as in this context ep->driver_data
contains invalid data.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_ncm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 3f05c6bd..394cdbf 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -586,7 +586,7 @@ static void ncm_ep0out_complete(struct usb_ep *ep, struct usb_request *req)
 	unsigned		in_size;
 	struct usb_function	*f = req->context;
 	struct f_ncm		*ncm = func_to_ncm(f);
-	struct usb_composite_dev *cdev = ep->driver_data;
+	struct usb_composite_dev *cdev = f->config->cdev;
 
 	req->context = NULL;
 	if (req->status || req->actual != req->length) {
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1225252 — [PATCH 01/26] usb: gadget: fix few outdated comments

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 16:50 +0200
Subject	[PATCH 01/26] usb: gadget: fix few outdated comments
Message-ID	<q8ZIv-7Nu-39@gated-at.bofh.it>
In reply to	#1225208

Fix comments in code to make them up to date.

composite: claiming endpoint is now done by setting ep->claimed flag,
not ep->driver_data.

epautoconf: usb_ep_autoconfig() and usb_ep_autoconfig_ss() return
claimed endpoint with ep->claimed flag already set.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/composite.c  | 4 +---
 drivers/usb/gadget/epautoconf.c | 8 ++++----
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index b474499..3e95c0e 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -839,9 +839,7 @@ int usb_add_config(struct usb_composite_dev *cdev,
 		}
 	}
 
-	/* set_alt(), or next bind(), sets up
-	 * ep->driver_data as needed.
-	 */
+	/* set_alt(), or next bind(), sets up ep->claimed as needed */
 	usb_ep_autoconfig_reset(cdev->gadget);
 
 done:
diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
index 6399c10..0f4ece4 100644
--- a/drivers/usb/gadget/epautoconf.c
+++ b/drivers/usb/gadget/epautoconf.c
@@ -53,13 +53,13 @@
  * the restrictions that may apply. Some combinations of driver
  * and hardware won't be able to autoconfigure.
  *
- * On success, this returns an un-claimed usb_ep, and modifies the endpoint
+ * On success, this returns an claimed usb_ep, and modifies the endpoint
  * descriptor bEndpointAddress.  For bulk endpoints, the wMaxPacket value
  * is initialized as if the endpoint were used at full speed and
  * the bmAttribute field in the ep companion descriptor is
  * updated with the assigned number of streams if it is
  * different from the original value. To prevent the endpoint
- * from being returned by a later autoconfig call, claim it by
+ * from being returned by a later autoconfig call, claims it by
  * assigning ep->claimed to true.
  *
  * On failure, this returns a null endpoint descriptor.
@@ -154,10 +154,10 @@ EXPORT_SYMBOL_GPL(usb_ep_autoconfig_ss);
  * USB controller, and it can't know all the restrictions that may apply.
  * Some combinations of driver and hardware won't be able to autoconfigure.
  *
- * On success, this returns an un-claimed usb_ep, and modifies the endpoint
+ * On success, this returns an claimed usb_ep, and modifies the endpoint
  * descriptor bEndpointAddress.  For bulk endpoints, the wMaxPacket value
  * is initialized as if the endpoint were used at full speed.  To prevent
- * the endpoint from being returned by a later autoconfig call, claim it
+ * the endpoint from being returned by a later autoconfig call, claims it
  * by assigning ep->claimed to true.
  *
  * On failure, this returns a null endpoint descriptor.
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1225258 — [PATCH 07/26] usb: gadget: f_eem: eliminate abuse of ep->driver data

From	Robert Baldyga <r.baldyga@samsung.com>
Date	2015-09-15 16:50 +0200
Subject	[PATCH 07/26] usb: gadget: f_eem: eliminate abuse of ep->driver data
Message-ID	<q8ZIv-7Nu-43@gated-at.bofh.it>
In reply to	#1225208

Since ep->driver_data is not used for endpoint claiming, neither for
enabled/disabled state storing, we can reduce number of places where
we read or modify it's value, as now it has no particular meaning for
function or framework logic.

In case of f_ecm, ep->driver_data was used only for endpoint claiming
and marking endpoints as enabled, so we can simplify code by reducing
it.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/usb/gadget/function/f_eem.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/function/f_eem.c b/drivers/usb/gadget/function/f_eem.c
index c9e90de..9a55757 100644
--- a/drivers/usb/gadget/function/f_eem.c
+++ b/drivers/usb/gadget/function/f_eem.c
@@ -195,11 +195,8 @@ static int eem_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		goto fail;
 
 	if (intf == eem->ctrl_id) {
-
-		if (eem->port.in_ep->driver_data) {
-			DBG(cdev, "reset eem\n");
-			gether_disconnect(&eem->port);
-		}
+		DBG(cdev, "reset eem\n");
+		gether_disconnect(&eem->port);
 
 		if (!eem->port.in_ep->desc || !eem->port.out_ep->desc) {
 			DBG(cdev, "init eem\n");
@@ -237,7 +234,7 @@ static void eem_disable(struct usb_function *f)
 
 	DBG(cdev, "eem deactivated\n");
 
-	if (eem->port.in_ep->driver_data)
+	if (eem->port.in_ep->enabled)
 		gether_disconnect(&eem->port);
 }
 
@@ -293,13 +290,11 @@ static int eem_bind(struct usb_configuration *c, struct usb_function *f)
 	if (!ep)
 		goto fail;
 	eem->port.in_ep = ep;
-	ep->driver_data = cdev;	/* claim */
 
 	ep = usb_ep_autoconfig(cdev->gadget, &eem_fs_out_desc);
 	if (!ep)
 		goto fail;
 	eem->port.out_ep = ep;
-	ep->driver_data = cdev;	/* claim */
 
 	status = -ENOMEM;
 
@@ -325,11 +320,6 @@ static int eem_bind(struct usb_configuration *c, struct usb_function *f)
 	return 0;
 
 fail:
-	if (eem->port.out_ep)
-		eem->port.out_ep->driver_data = NULL;
-	if (eem->port.in_ep)
-		eem->port.in_ep->driver_data = NULL;
-
 	ERROR(cdev, "%s: can't bind, err %d\n", f->name, status);
 
 	return status;
-- 
1.9.1

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

[toc] | [prev] | [standalone]

csiph-web

[PATCH 00/26] usb: gadget: encapsulate ep enable/disable

Contents

#1225208 — [PATCH 00/26] usb: gadget: encapsulate ep enable/disable

#1225212 — [PATCH 20/26] usb: gadget: f_uac1: eliminate abuse of ep->driver data

#1225219 — [PATCH 21/26] usb: gadget: f_uac2: eliminate abuse of ep->driver data

#1225225 — [PATCH 13/26] usb: gadget: f_obex: eliminate abuse of ep->driver data

#1225227 — [PATCH 23/26] usb: gadget: u_ether: eliminate abuse of ep->driver data

#1225234 — [PATCH 25/26] usb: gadget: legacy: dbgp: eliminate abuse of ep->driver data

#1225237 — [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

#1225320 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

#1225333 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

#1225377 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

#1225379 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

#1225389 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

#1225403 — Re: [PATCH 04/26] usb: gadget: introduce 'enabled' flag in struct usb_ep

#1225238 — [PATCH 02/26] usb: gadget: f_ncm: obtain cdev from function instead of driver_data

#1225252 — [PATCH 01/26] usb: gadget: fix few outdated comments

#1225258 — [PATCH 07/26] usb: gadget: f_eem: eliminate abuse of ep->driver data