* [PATCH 1/2] usb: dwc3: allocate gadget structure dynamically
@ 2020-08-06 9:07 Peter Chen
2020-08-06 9:07 ` [PATCH 2/2] Revert "usb: udc: allow adding and removing the same gadget device" Peter Chen
2020-08-06 14:54 ` [PATCH 1/2] usb: dwc3: allocate gadget structure dynamically Alan Stern
0 siblings, 2 replies; 4+ messages in thread
From: Peter Chen @ 2020-08-06 9:07 UTC (permalink / raw)
To: balbi; +Cc: linux-usb, linux-imx, Peter Chen, Greg Kroah-Hartman, Alan Stern
The current code uses commit fac323471df6 ("usb: udc: allow adding
and removing the same gadget device") as the workaround to let
the gadget device is re-used, but it is not allowed from driver
core point. In this commit, we allocate gadget structure dynamically,
and free it at its release function. Since the gadget device's
driver_data has already occupied by usb_composite_dev structure, we have
to use gadget device's platform data to store dwc3 structure.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
drivers/usb/dwc3/core.h | 2 +-
drivers/usb/dwc3/ep0.c | 26 +++++-----
drivers/usb/dwc3/gadget.c | 102 ++++++++++++++++++++++----------------
drivers/usb/dwc3/gadget.h | 2 +-
4 files changed, 74 insertions(+), 58 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 18c7a9907679..48301d950068 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1102,7 +1102,7 @@ struct dwc3 {
struct dwc3_event_buffer *ev_buf;
struct dwc3_ep *eps[DWC3_ENDPOINTS_NUM];
- struct usb_gadget gadget;
+ struct usb_gadget *gadget;
struct usb_gadget_driver *gadget_driver;
struct clk_bulk_data *clks;
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 8dd69728add3..1799bd19bf6f 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -131,7 +131,7 @@ static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
direction = !dwc->ep0_expect_in;
dwc->delayed_status = false;
- usb_gadget_set_state(&dwc->gadget, USB_STATE_CONFIGURED);
+ usb_gadget_set_state(dwc->gadget, USB_STATE_CONFIGURED);
if (dwc->ep0state == EP0_STATUS_PHASE)
__dwc3_ep0_do_control_status(dwc, dwc->eps[direction]);
@@ -325,7 +325,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
/*
* LTM will be set once we know how to set this in HW.
*/
- usb_status |= dwc->gadget.is_selfpowered;
+ usb_status |= dwc->gadget->is_selfpowered;
if ((dwc->speed == DWC3_DSTS_SUPERSPEED) ||
(dwc->speed == DWC3_DSTS_SUPERSPEED_PLUS)) {
@@ -450,7 +450,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
wValue = le16_to_cpu(ctrl->wValue);
wIndex = le16_to_cpu(ctrl->wIndex);
- state = dwc->gadget.state;
+ state = dwc->gadget->state;
switch (wValue) {
case USB_DEVICE_REMOTE_WAKEUP:
@@ -559,7 +559,7 @@ static int dwc3_ep0_handle_feature(struct dwc3 *dwc,
static int dwc3_ep0_set_address(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
{
- enum usb_device_state state = dwc->gadget.state;
+ enum usb_device_state state = dwc->gadget->state;
u32 addr;
u32 reg;
@@ -580,9 +580,9 @@ static int dwc3_ep0_set_address(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
dwc3_writel(dwc->regs, DWC3_DCFG, reg);
if (addr)
- usb_gadget_set_state(&dwc->gadget, USB_STATE_ADDRESS);
+ usb_gadget_set_state(dwc->gadget, USB_STATE_ADDRESS);
else
- usb_gadget_set_state(&dwc->gadget, USB_STATE_DEFAULT);
+ usb_gadget_set_state(dwc->gadget, USB_STATE_DEFAULT);
return 0;
}
@@ -592,14 +592,14 @@ static int dwc3_ep0_delegate_req(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
int ret;
spin_unlock(&dwc->lock);
- ret = dwc->gadget_driver->setup(&dwc->gadget, ctrl);
+ ret = dwc->gadget_driver->setup(dwc->gadget, ctrl);
spin_lock(&dwc->lock);
return ret;
}
static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
{
- enum usb_device_state state = dwc->gadget.state;
+ enum usb_device_state state = dwc->gadget->state;
u32 cfg;
int ret;
u32 reg;
@@ -622,7 +622,7 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
* to change the state on the next usb_ep_queue()
*/
if (ret == 0)
- usb_gadget_set_state(&dwc->gadget,
+ usb_gadget_set_state(dwc->gadget,
USB_STATE_CONFIGURED);
/*
@@ -641,7 +641,7 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
case USB_STATE_CONFIGURED:
ret = dwc3_ep0_delegate_req(dwc, ctrl);
if (!cfg && !ret)
- usb_gadget_set_state(&dwc->gadget,
+ usb_gadget_set_state(dwc->gadget,
USB_STATE_ADDRESS);
break;
default:
@@ -697,7 +697,7 @@ static void dwc3_ep0_set_sel_cmpl(struct usb_ep *ep, struct usb_request *req)
static int dwc3_ep0_set_sel(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
{
struct dwc3_ep *dep;
- enum usb_device_state state = dwc->gadget.state;
+ enum usb_device_state state = dwc->gadget->state;
u16 wLength;
if (state == USB_STATE_DEFAULT)
@@ -741,7 +741,7 @@ static int dwc3_ep0_set_isoch_delay(struct dwc3 *dwc, struct usb_ctrlrequest *ct
if (wIndex || wLength)
return -EINVAL;
- dwc->gadget.isoch_delay = wValue;
+ dwc->gadget->isoch_delay = wValue;
return 0;
}
@@ -1102,7 +1102,7 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
*/
if (!list_empty(&dep->pending_list)) {
dwc->delayed_status = false;
- usb_gadget_set_state(&dwc->gadget,
+ usb_gadget_set_state(dwc->gadget,
USB_STATE_CONFIGURED);
dwc3_ep0_do_control_status(dwc, event);
}
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0b59b2f1cf26..db531390af0f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -290,7 +290,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
*
* DWC_usb3 3.30a and DWC_usb31 1.90a programming guide section 3.2.2
*/
- if (dwc->gadget.speed <= USB_SPEED_HIGH) {
+ if (dwc->gadget->speed <= USB_SPEED_HIGH) {
reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
saved_config |= DWC3_GUSB2PHYCFG_SUSPHY;
@@ -422,7 +422,7 @@ static int dwc3_send_clear_stall_ep_cmd(struct dwc3_ep *dep)
*/
if (dep->direction &&
!DWC3_VER_IS_PRIOR(DWC3, 260A) &&
- (dwc->gadget.speed >= USB_SPEED_SUPER))
+ (dwc->gadget->speed >= USB_SPEED_SUPER))
cmd |= DWC3_DEPCMD_CLEARPENDIN;
memset(¶ms, 0, sizeof(params));
@@ -562,7 +562,7 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action)
| DWC3_DEPCFG_MAX_PACKET_SIZE(usb_endpoint_maxp(desc));
/* Burst size is only needed in SuperSpeed mode */
- if (dwc->gadget.speed >= USB_SPEED_SUPER) {
+ if (dwc->gadget->speed >= USB_SPEED_SUPER) {
u32 burst = dep->endpoint.maxburst;
params.param0 |= DWC3_DEPCFG_BURST_SIZE(burst - 1);
}
@@ -947,7 +947,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
unsigned no_interrupt, unsigned is_last)
{
struct dwc3 *dwc = dep->dwc;
- struct usb_gadget *gadget = &dwc->gadget;
+ struct usb_gadget *gadget = dwc->gadget;
enum usb_device_speed speed = gadget->speed;
trb->size = DWC3_TRB_SIZE_LENGTH(length);
@@ -1476,7 +1476,7 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
if (!dwc->dis_start_transfer_quirk &&
(DWC3_VER_IS_PRIOR(DWC31, 170A) ||
DWC3_VER_TYPE_IS_WITHIN(DWC31, 170A, EA01, EA06))) {
- if (dwc->gadget.speed <= USB_SPEED_HIGH && dep->direction)
+ if (dwc->gadget->speed <= USB_SPEED_HIGH && dep->direction)
return dwc3_gadget_start_isoc_quirk(dep);
}
@@ -2160,7 +2160,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
spin_lock_irqsave(&dwc->lock, flags);
if (dwc->gadget_driver) {
dev_err(dwc->dev, "%s is already bound to %s\n",
- dwc->gadget.name,
+ dwc->gadget->name,
dwc->gadget_driver->driver.name);
ret = -EBUSY;
goto err1;
@@ -2332,7 +2332,7 @@ static int dwc3_gadget_init_control_endpoint(struct dwc3_ep *dep)
dep->endpoint.maxburst = 1;
dep->endpoint.ops = &dwc3_gadget_ep0_ops;
if (!dep->direction)
- dwc->gadget.ep0 = &dep->endpoint;
+ dwc->gadget->ep0 = &dep->endpoint;
dep->endpoint.caps.type_control = true;
@@ -2378,7 +2378,7 @@ static int dwc3_gadget_init_in_endpoint(struct dwc3_ep *dep)
dep->endpoint.max_streams = 15;
dep->endpoint.ops = &dwc3_gadget_ep_ops;
list_add_tail(&dep->endpoint.ep_list,
- &dwc->gadget.ep_list);
+ &dwc->gadget->ep_list);
dep->endpoint.caps.type_iso = true;
dep->endpoint.caps.type_bulk = true;
dep->endpoint.caps.type_int = true;
@@ -2427,7 +2427,7 @@ static int dwc3_gadget_init_out_endpoint(struct dwc3_ep *dep)
dep->endpoint.max_streams = 15;
dep->endpoint.ops = &dwc3_gadget_ep_ops;
list_add_tail(&dep->endpoint.ep_list,
- &dwc->gadget.ep_list);
+ &dwc->gadget->ep_list);
dep->endpoint.caps.type_iso = true;
dep->endpoint.caps.type_bulk = true;
dep->endpoint.caps.type_int = true;
@@ -2488,7 +2488,7 @@ static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 total)
{
u8 epnum;
- INIT_LIST_HEAD(&dwc->gadget.ep_list);
+ INIT_LIST_HEAD(&dwc->gadget->ep_list);
for (epnum = 0; epnum < total; epnum++) {
int ret;
@@ -2924,7 +2924,7 @@ static void dwc3_disconnect_gadget(struct dwc3 *dwc)
{
if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
spin_unlock(&dwc->lock);
- dwc->gadget_driver->disconnect(&dwc->gadget);
+ dwc->gadget_driver->disconnect(dwc->gadget);
spin_lock(&dwc->lock);
}
}
@@ -2933,7 +2933,7 @@ static void dwc3_suspend_gadget(struct dwc3 *dwc)
{
if (dwc->gadget_driver && dwc->gadget_driver->suspend) {
spin_unlock(&dwc->lock);
- dwc->gadget_driver->suspend(&dwc->gadget);
+ dwc->gadget_driver->suspend(dwc->gadget);
spin_lock(&dwc->lock);
}
}
@@ -2942,7 +2942,7 @@ static void dwc3_resume_gadget(struct dwc3 *dwc)
{
if (dwc->gadget_driver && dwc->gadget_driver->resume) {
spin_unlock(&dwc->lock);
- dwc->gadget_driver->resume(&dwc->gadget);
+ dwc->gadget_driver->resume(dwc->gadget);
spin_lock(&dwc->lock);
}
}
@@ -2952,9 +2952,9 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
if (!dwc->gadget_driver)
return;
- if (dwc->gadget.speed != USB_SPEED_UNKNOWN) {
+ if (dwc->gadget->speed != USB_SPEED_UNKNOWN) {
spin_unlock(&dwc->lock);
- usb_gadget_udc_reset(&dwc->gadget, dwc->gadget_driver);
+ usb_gadget_udc_reset(dwc->gadget, dwc->gadget_driver);
spin_lock(&dwc->lock);
}
}
@@ -3055,9 +3055,9 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
dwc3_disconnect_gadget(dwc);
- dwc->gadget.speed = USB_SPEED_UNKNOWN;
+ dwc->gadget->speed = USB_SPEED_UNKNOWN;
dwc->setup_packet_pending = false;
- usb_gadget_set_state(&dwc->gadget, USB_STATE_NOTATTACHED);
+ usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
dwc->connected = false;
}
@@ -3136,8 +3136,8 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
switch (speed) {
case DWC3_DSTS_SUPERSPEED_PLUS:
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
- dwc->gadget.ep0->maxpacket = 512;
- dwc->gadget.speed = USB_SPEED_SUPER_PLUS;
+ dwc->gadget->ep0->maxpacket = 512;
+ dwc->gadget->speed = USB_SPEED_SUPER_PLUS;
break;
case DWC3_DSTS_SUPERSPEED:
/*
@@ -3157,27 +3157,27 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
dwc3_gadget_reset_interrupt(dwc);
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
- dwc->gadget.ep0->maxpacket = 512;
- dwc->gadget.speed = USB_SPEED_SUPER;
+ dwc->gadget->ep0->maxpacket = 512;
+ dwc->gadget->speed = USB_SPEED_SUPER;
break;
case DWC3_DSTS_HIGHSPEED:
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(64);
- dwc->gadget.ep0->maxpacket = 64;
- dwc->gadget.speed = USB_SPEED_HIGH;
+ dwc->gadget->ep0->maxpacket = 64;
+ dwc->gadget->speed = USB_SPEED_HIGH;
break;
case DWC3_DSTS_FULLSPEED:
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(64);
- dwc->gadget.ep0->maxpacket = 64;
- dwc->gadget.speed = USB_SPEED_FULL;
+ dwc->gadget->ep0->maxpacket = 64;
+ dwc->gadget->speed = USB_SPEED_FULL;
break;
case DWC3_DSTS_LOWSPEED:
dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(8);
- dwc->gadget.ep0->maxpacket = 8;
- dwc->gadget.speed = USB_SPEED_LOW;
+ dwc->gadget->ep0->maxpacket = 8;
+ dwc->gadget->speed = USB_SPEED_LOW;
break;
}
- dwc->eps[1]->endpoint.maxpacket = dwc->gadget.ep0->maxpacket;
+ dwc->eps[1]->endpoint.maxpacket = dwc->gadget->ep0->maxpacket;
/* Enable USB2 LPM Capability */
@@ -3245,7 +3245,7 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
if (dwc->gadget_driver && dwc->gadget_driver->resume) {
spin_unlock(&dwc->lock);
- dwc->gadget_driver->resume(&dwc->gadget);
+ dwc->gadget_driver->resume(dwc->gadget);
spin_lock(&dwc->lock);
}
}
@@ -3416,7 +3416,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
* Ignore suspend event until the gadget enters into
* USB_STATE_CONFIGURED state.
*/
- if (dwc->gadget.state >= USB_STATE_CONFIGURED)
+ if (dwc->gadget->state >= USB_STATE_CONFIGURED)
dwc3_gadget_suspend_interrupt(dwc,
event->event_info);
}
@@ -3590,6 +3590,12 @@ static int dwc3_gadget_get_irq(struct dwc3 *dwc)
out:
return irq;
}
+static void dwc_gadget_release(struct device *dev)
+{
+ struct usb_gadget *gadget = container_of(dev, struct usb_gadget, dev);
+
+ kfree(gadget);
+}
/**
* dwc3_gadget_init - initializes gadget related registers
@@ -3601,6 +3607,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
{
int ret;
int irq;
+ struct device *dev;
irq = dwc3_gadget_get_irq(dwc);
if (irq < 0) {
@@ -3633,12 +3640,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)
}
init_completion(&dwc->ep0_in_setup);
+ dwc->gadget = kzalloc(sizeof(struct usb_gadget), GFP_KERNEL);
+ if (!dwc->gadget) {
+ ret = -ENOMEM;
+ goto err3;
+ }
+
- dwc->gadget.ops = &dwc3_gadget_ops;
- dwc->gadget.speed = USB_SPEED_UNKNOWN;
- dwc->gadget.sg_supported = true;
- dwc->gadget.name = "dwc3-gadget";
- dwc->gadget.lpm_capable = true;
+ dev = &dwc->gadget->dev;
+ dev->platform_data = dwc;
+ dwc->gadget->ops = &dwc3_gadget_ops;
+ dwc->gadget->speed = USB_SPEED_UNKNOWN;
+ dwc->gadget->sg_supported = true;
+ dwc->gadget->name = "dwc3-gadget";
+ dwc->gadget->lpm_capable = true;
/*
* FIXME We might be setting max_speed to <SUPER, however versions
@@ -3661,7 +3676,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
dev_info(dwc->dev, "changing max_speed on rev %08x\n",
dwc->revision);
- dwc->gadget.max_speed = dwc->maximum_speed;
+ dwc->gadget->max_speed = dwc->maximum_speed;
/*
* REVISIT: Here we should clear all pending IRQs to be
@@ -3670,21 +3685,22 @@ int dwc3_gadget_init(struct dwc3 *dwc)
ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
if (ret)
- goto err3;
+ goto err4;
- ret = usb_add_gadget_udc(dwc->dev, &dwc->gadget);
+ ret = usb_add_gadget_udc_release(dwc->dev, dwc->gadget, dwc_gadget_release);
if (ret) {
dev_err(dwc->dev, "failed to register udc\n");
- goto err4;
+ goto err5;
}
- dwc3_gadget_set_speed(&dwc->gadget, dwc->maximum_speed);
+ dwc3_gadget_set_speed(dwc->gadget, dwc->maximum_speed);
return 0;
-err4:
+err5:
dwc3_gadget_free_endpoints(dwc);
-
+err4:
+ kfree(dwc->gadget);
err3:
dma_free_coherent(dwc->sysdev, DWC3_BOUNCE_SIZE, dwc->bounce,
dwc->bounce_addr);
@@ -3704,7 +3720,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
void dwc3_gadget_exit(struct dwc3 *dwc)
{
- usb_del_gadget_udc(&dwc->gadget);
+ usb_del_gadget_udc(dwc->gadget);
dwc3_gadget_free_endpoints(dwc);
dma_free_coherent(dwc->sysdev, DWC3_BOUNCE_SIZE, dwc->bounce,
dwc->bounce_addr);
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 24dca3872022..b547db228f1e 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -17,7 +17,7 @@
struct dwc3;
#define to_dwc3_ep(ep) (container_of(ep, struct dwc3_ep, endpoint))
-#define gadget_to_dwc(g) (container_of(g, struct dwc3, gadget))
+#define gadget_to_dwc(g) (dev_get_platdata(&g->dev))
/* DEPCFG parameter 1 */
#define DWC3_DEPCFG_INT_NUM(n) (((n) & 0x1f) << 0)
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] Revert "usb: udc: allow adding and removing the same gadget device"
2020-08-06 9:07 [PATCH 1/2] usb: dwc3: allocate gadget structure dynamically Peter Chen
@ 2020-08-06 9:07 ` Peter Chen
2020-08-06 14:54 ` [PATCH 1/2] usb: dwc3: allocate gadget structure dynamically Alan Stern
1 sibling, 0 replies; 4+ messages in thread
From: Peter Chen @ 2020-08-06 9:07 UTC (permalink / raw)
To: balbi; +Cc: linux-usb, linux-imx, Peter Chen, Greg Kroah-Hartman, Alan Stern
We have already allocated gadget structure dynamically at UDC (dwc3)
driver, so commit fac323471df6 ("usb: udc: allow adding and removing
the same gadget device")could be reverted.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
drivers/usb/gadget/udc/core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index ee226ad802a4..2170bb67cc9e 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1338,7 +1338,6 @@ 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.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] usb: dwc3: allocate gadget structure dynamically
2020-08-06 9:07 [PATCH 1/2] usb: dwc3: allocate gadget structure dynamically Peter Chen
2020-08-06 9:07 ` [PATCH 2/2] Revert "usb: udc: allow adding and removing the same gadget device" Peter Chen
@ 2020-08-06 14:54 ` Alan Stern
2020-08-06 19:18 ` Alan Stern
1 sibling, 1 reply; 4+ messages in thread
From: Alan Stern @ 2020-08-06 14:54 UTC (permalink / raw)
To: Peter Chen; +Cc: balbi, linux-usb, linux-imx, Greg Kroah-Hartman
On Thu, Aug 06, 2020 at 05:07:16PM +0800, Peter Chen wrote:
> The current code uses commit fac323471df6 ("usb: udc: allow adding
> and removing the same gadget device") as the workaround to let
> the gadget device is re-used, but it is not allowed from driver
> core point. In this commit, we allocate gadget structure dynamically,
> and free it at its release function. Since the gadget device's
> driver_data has already occupied by usb_composite_dev structure, we have
> to use gadget device's platform data to store dwc3 structure.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
There's one problem with this patch...
> @@ -3670,21 +3685,22 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>
> ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
> if (ret)
> - goto err3;
> + goto err4;
>
> - ret = usb_add_gadget_udc(dwc->dev, &dwc->gadget);
> + ret = usb_add_gadget_udc_release(dwc->dev, dwc->gadget, dwc_gadget_release);
> if (ret) {
> dev_err(dwc->dev, "failed to register udc\n");
> - goto err4;
> + goto err5;
> }
>
> - dwc3_gadget_set_speed(&dwc->gadget, dwc->maximum_speed);
> + dwc3_gadget_set_speed(dwc->gadget, dwc->maximum_speed);
>
> return 0;
>
> -err4:
> +err5:
> dwc3_gadget_free_endpoints(dwc);
> -
> +err4:
> + kfree(dwc->gadget);
Once you call usb_add_gadget_udc_release() -- even if the call fails --
you must not call kfree() directly. Instead you have to do
put_device(&dwc->gadget.dev). That's a little awkward given the code
arrangement here.
One of the advantages of the patch set I posted recently was that it
straightens out this whole situation. Immediately after allocating
the gadget structure you can call usb_initialize_gadget(), and after
that you call usb_put_gadget() to decrement the refcount -- just like
everything else in the device model.
Perhaps you should rewrite this part of your patch to rely on my work.
Alan Stern
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] usb: dwc3: allocate gadget structure dynamically
2020-08-06 14:54 ` [PATCH 1/2] usb: dwc3: allocate gadget structure dynamically Alan Stern
@ 2020-08-06 19:18 ` Alan Stern
0 siblings, 0 replies; 4+ messages in thread
From: Alan Stern @ 2020-08-06 19:18 UTC (permalink / raw)
To: Peter Chen; +Cc: balbi, linux-usb, linux-imx, Greg Kroah-Hartman
On Thu, Aug 06, 2020 at 10:54:14AM -0400, Alan Stern wrote:
> On Thu, Aug 06, 2020 at 05:07:16PM +0800, Peter Chen wrote:
> > The current code uses commit fac323471df6 ("usb: udc: allow adding
> > and removing the same gadget device") as the workaround to let
> > the gadget device is re-used, but it is not allowed from driver
> > core point. In this commit, we allocate gadget structure dynamically,
> > and free it at its release function. Since the gadget device's
> > driver_data has already occupied by usb_composite_dev structure, we have
> > to use gadget device's platform data to store dwc3 structure.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
>
> There's one problem with this patch...
>
> > @@ -3670,21 +3685,22 @@ int dwc3_gadget_init(struct dwc3 *dwc)
> >
> > ret = dwc3_gadget_init_endpoints(dwc, dwc->num_eps);
> > if (ret)
> > - goto err3;
> > + goto err4;
> >
> > - ret = usb_add_gadget_udc(dwc->dev, &dwc->gadget);
> > + ret = usb_add_gadget_udc_release(dwc->dev, dwc->gadget, dwc_gadget_release);
> > if (ret) {
> > dev_err(dwc->dev, "failed to register udc\n");
> > - goto err4;
> > + goto err5;
> > }
> >
> > - dwc3_gadget_set_speed(&dwc->gadget, dwc->maximum_speed);
> > + dwc3_gadget_set_speed(dwc->gadget, dwc->maximum_speed);
> >
> > return 0;
> >
> > -err4:
> > +err5:
> > dwc3_gadget_free_endpoints(dwc);
> > -
> > +err4:
> > + kfree(dwc->gadget);
>
> Once you call usb_add_gadget_udc_release() -- even if the call fails --
> you must not call kfree() directly. Instead you have to do
> put_device(&dwc->gadget.dev). That's a little awkward given the code
> arrangement here.
Sorry, I got that wrong. If usb_add_gadget_udc_release() succeeds then
you must call put_device() in order to destroy the gadget. But if it
fails then it calls put_device() for you, so that dwc->gadget is a stale
pointer. In this case you must not call either kfree() or put_device().
Alan Stern
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-06 19:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-06 9:07 [PATCH 1/2] usb: dwc3: allocate gadget structure dynamically Peter Chen
2020-08-06 9:07 ` [PATCH 2/2] Revert "usb: udc: allow adding and removing the same gadget device" Peter Chen
2020-08-06 14:54 ` [PATCH 1/2] usb: dwc3: allocate gadget structure dynamically Alan Stern
2020-08-06 19:18 ` Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).