Do not allocate a port to new device or process URB when its status is
yet to be read. This avoids race condition when USB core read hub
status a bit late, while new device tries to acquire port.
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 2b67107..4682d78 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -685,7 +685,7 @@ struct oz_port *oz_hcd_pd_arrived(void *hpd)
struct oz_port *port = &ozhcd->ports[i];
spin_lock(&port->port_lock);
- if (!(port->flags & OZ_PORT_F_PRESENT)) {
+ if (!(port->flags & (OZ_PORT_F_PRESENT | OZ_PORT_F_CHANGED))) {
oz_acquire_port(port, hpd);
spin_unlock(&port->port_lock);
break;
@@ -1818,7 +1818,8 @@ static int oz_hcd_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
port = &ozhcd->ports[port_ix];
if (port == NULL)
return -EPIPE;
- if ((port->flags & OZ_PORT_F_PRESENT) == 0) {
+ if (!(port->flags & OZ_PORT_F_PRESENT) ||
+ (port->flags & OZ_PORT_F_CHANGED)) {
oz_dbg(ON, "Refusing URB port_ix = %d devnum = %d\n",
port_ix, urb->dev->devnum);
return -EPIPE;
--
1.7.9.5
Check if interface number is correct before creating an end point.
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozhcd.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/ozwpan/ozhcd.c b/drivers/staging/ozwpan/ozhcd.c
index 4682d78..d5a3900 100644
--- a/drivers/staging/ozwpan/ozhcd.c
+++ b/drivers/staging/ozwpan/ozhcd.c
@@ -1247,6 +1247,8 @@ static int oz_build_endpoints_for_interface(struct usb_hcd *hcd,
int request_heartbeat = 0;
oz_dbg(ON, "interface[%d] = %p\n", if_ix, intf);
+ if (if_ix >= port->num_iface || port->iface == NULL)
+ return -ENOMEM;
for (i = 0; i < intf->desc.bNumEndpoints; i++) {
struct usb_host_endpoint *hep = &intf->endpoint[i];
u8 ep_addr = hep->desc.bEndpointAddress;
--
1.7.9.5
Increment PD reference counter, on every timer event so that
we do not loose PD object by mistake.
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozproto.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/ozwpan/ozproto.c b/drivers/staging/ozwpan/ozproto.c
index 8c2200b..d8ac823 100644
--- a/drivers/staging/ozwpan/ozproto.c
+++ b/drivers/staging/ozwpan/ozproto.c
@@ -497,7 +497,7 @@ void oz_pd_heartbeat_handler(unsigned long data)
spin_unlock_bh(&g_polling_lock);
if (apps)
oz_pd_heartbeat(pd, apps);
-
+ oz_pd_put(pd);
}
/*------------------------------------------------------------------------------
@@ -519,6 +519,7 @@ void oz_pd_timeout_handler(unsigned long data)
oz_pd_stop(pd);
break;
}
+ oz_pd_put(pd);
}
/*------------------------------------------------------------------------------
@@ -531,6 +532,7 @@ enum hrtimer_restart oz_pd_heartbeat_event(struct hrtimer *timer)
pd = container_of(timer, struct oz_pd, heartbeat);
hrtimer_forward_now(timer, ktime_set(pd->pulse_period /
MSEC_PER_SEC, (pd->pulse_period % MSEC_PER_SEC) * NSEC_PER_MSEC));
+ oz_pd_get(pd);
tasklet_schedule(&pd->heartbeat_tasklet);
return HRTIMER_RESTART;
}
@@ -543,6 +545,7 @@ enum hrtimer_restart oz_pd_timeout_event(struct hrtimer *timer)
struct oz_pd *pd;
pd = container_of(timer, struct oz_pd, timeout);
+ oz_pd_get(pd);
tasklet_schedule(&pd->timeout_tasklet);
return HRTIMER_NORESTART;
}
--
1.7.9.5
Currently we call oz_pd_destroy() from softirq context, where we
try to destroy relevant data structures, as well we kill a tasklet
which always result in following kernel warning.
[12279.262194] Attempt to kill tasklet from interrupt
[12279.262202] Attempt to kill tasklet from interrupt
This patch defers deallocation of data structures to work queue.
Signed-off-by: Rupesh Gujare <[email protected]>
---
drivers/staging/ozwpan/ozpd.c | 28 +++++++++++++++++++++++-----
drivers/staging/ozwpan/ozpd.h | 1 +
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/ozwpan/ozpd.c b/drivers/staging/ozwpan/ozpd.c
index 2514d79..06004c8 100644
--- a/drivers/staging/ozwpan/ozpd.c
+++ b/drivers/staging/ozwpan/ozpd.c
@@ -204,18 +204,16 @@ struct oz_pd *oz_pd_alloc(const u8 *mac_addr)
/*------------------------------------------------------------------------------
* Context: softirq or process
*/
-void oz_pd_destroy(struct oz_pd *pd)
+void oz_pd_free(struct work_struct *work)
{
struct list_head *e;
struct oz_tx_frame *f;
struct oz_isoc_stream *st;
struct oz_farewell *fwell;
+ struct oz_pd *pd;
oz_pd_dbg(pd, ON, "Destroying PD\n");
- if (hrtimer_active(&pd->timeout))
- hrtimer_cancel(&pd->timeout);
- if (hrtimer_active(&pd->heartbeat))
- hrtimer_cancel(&pd->heartbeat);
+ pd = container_of(work, struct oz_pd, workitem);
/*Disable timer tasklets*/
tasklet_kill(&pd->heartbeat_tasklet);
tasklet_kill(&pd->timeout_tasklet);
@@ -259,6 +257,26 @@ void oz_pd_destroy(struct oz_pd *pd)
}
/*------------------------------------------------------------------------------
+ * Context: softirq or Process
+ */
+void oz_pd_destroy(struct oz_pd *pd)
+{
+ int ret;
+
+ if (hrtimer_active(&pd->timeout))
+ hrtimer_cancel(&pd->timeout);
+ if (hrtimer_active(&pd->heartbeat))
+ hrtimer_cancel(&pd->heartbeat);
+
+ memset(&pd->workitem, 0, sizeof(pd->workitem));
+ INIT_WORK(&pd->workitem, oz_pd_free);
+ ret = schedule_work(&pd->workitem);
+
+ if (ret)
+ oz_pd_dbg(pd, ON, "failed to schedule workitem\n");
+}
+
+/*------------------------------------------------------------------------------
* Context: softirq-serialized
*/
int oz_services_start(struct oz_pd *pd, u16 apps, int resume)
diff --git a/drivers/staging/ozwpan/ozpd.h b/drivers/staging/ozwpan/ozpd.h
index 996ef65..12c7129 100644
--- a/drivers/staging/ozwpan/ozpd.h
+++ b/drivers/staging/ozwpan/ozpd.h
@@ -99,6 +99,7 @@ struct oz_pd {
u8 timeout_type;
struct tasklet_struct heartbeat_tasklet;
struct tasklet_struct timeout_tasklet;
+ struct work_struct workitem;
};
#define OZ_MAX_QUEUED_FRAMES 4
--
1.7.9.5
Hello.
On 08/22/2013 08:38 PM, Rupesh Gujare wrote:
> Currently we call oz_pd_destroy() from softirq context, where we
> try to destroy relevant data structures, as well we kill a tasklet
> which always result in following kernel warning.
> [12279.262194] Attempt to kill tasklet from interrupt
> [12279.262202] Attempt to kill tasklet from interrupt
> This patch defers deallocation of data structures to work queue.
> Signed-off-by: Rupesh Gujare <[email protected]>
> ---
> drivers/staging/ozwpan/ozpd.c | 28 +++++++++++++++++++++++-----
> drivers/staging/ozwpan/ozpd.h | 1 +
> 2 files changed, 24 insertions(+), 5 deletions(-)
> diff --git a/drivers/staging/ozwpan/ozpd.c b/drivers/staging/ozwpan/ozpd.c
> index 2514d79..06004c8 100644
> --- a/drivers/staging/ozwpan/ozpd.c
> +++ b/drivers/staging/ozwpan/ozpd.c
[...]
> @@ -259,6 +257,26 @@ void oz_pd_destroy(struct oz_pd *pd)
> }
>
> /*------------------------------------------------------------------------------
> + * Context: softirq or Process
> + */
> +void oz_pd_destroy(struct oz_pd *pd)
> +{
> + int ret;
> +
> + if (hrtimer_active(&pd->timeout))
> + hrtimer_cancel(&pd->timeout);
> + if (hrtimer_active(&pd->heartbeat))
> + hrtimer_cancel(&pd->heartbeat);
> +
> + memset(&pd->workitem, 0, sizeof(pd->workitem));
> + INIT_WORK(&pd->workitem, oz_pd_free);
Hm, memset(), then INIT_WORK()? Is memset() necessary?
> + ret = schedule_work(&pd->workitem);
> +
Don't think empty line is needed here.
> + if (ret)
> + oz_pd_dbg(pd, ON, "failed to schedule workitem\n");
> +}
> +
> +/*------------------------------------------------------------------------------
> * Context: softirq-serialized
> */
> int oz_services_start(struct oz_pd *pd, u16 apps, int resume)
> diff --git a/drivers/staging/ozwpan/ozpd.h b/drivers/staging/ozwpan/ozpd.h
> index 996ef65..12c7129 100644
> --- a/drivers/staging/ozwpan/ozpd.h
> +++ b/drivers/staging/ozwpan/ozpd.h
> @@ -99,6 +99,7 @@ struct oz_pd {
> u8 timeout_type;
> struct tasklet_struct heartbeat_tasklet;
> struct tasklet_struct timeout_tasklet;
> + struct work_struct workitem;
Er, other field names seem aligned, what about this one?
> };
WBR, Sergei
On Thu, Aug 22, 2013 at 05:38:51PM +0100, Rupesh Gujare wrote:
> +/*------------------------------------------------------------------------------
> * Context: softirq-serialized
> */
Don't resend the patch, but these comments are not in kernel style.
It's explained in Documentation/kernel-doc-nano-HOWTO.txt
The main thing is that could you just delete all the --------
lines?
regards,
dan carpenter
On 22/08/13 18:45, Sergei Shtylyov wrote:
>> +void oz_pd_destroy(struct oz_pd *pd)
>> +{
>> + int ret;
>> +
>> + if (hrtimer_active(&pd->timeout))
>> + hrtimer_cancel(&pd->timeout);
>> + if (hrtimer_active(&pd->heartbeat))
>> + hrtimer_cancel(&pd->heartbeat);
>> +
>> + memset(&pd->workitem, 0, sizeof(pd->workitem));
>> + INIT_WORK(&pd->workitem, oz_pd_free);
>
> Hm, memset(), then INIT_WORK()? Is memset() necessary?
Opps.. you are right, I think we don't need memset() here.
>
>> + ret = schedule_work(&pd->workitem);
>> +
>
> Don't think empty line is needed here.
Yes, I agree. I will send follow on patches to fix this, as original
patches had already been applied by Greg.
>
>> + if (ret)
>> + oz_pd_dbg(pd, ON, "failed to schedule workitem\n");
>> +}
>> +
>> +/*------------------------------------------------------------------------------
>>
>> * Context: softirq-serialized
>> */
>> int oz_services_start(struct oz_pd *pd, u16 apps, int resume)
>> diff --git a/drivers/staging/ozwpan/ozpd.h
>> b/drivers/staging/ozwpan/ozpd.h
>> index 996ef65..12c7129 100644
>> --- a/drivers/staging/ozwpan/ozpd.h
>> +++ b/drivers/staging/ozwpan/ozpd.h
>> @@ -99,6 +99,7 @@ struct oz_pd {
>> u8 timeout_type;
>> struct tasklet_struct heartbeat_tasklet;
>> struct tasklet_struct timeout_tasklet;
>> + struct work_struct workitem;
>
> Er, other field names seem aligned, what about this one?
>
After applying patch, it looks all right to me.
--
Regards,
Rupesh Gujare
On 23/08/13 10:05, Dan Carpenter wrote:
> On Thu, Aug 22, 2013 at 05:38:51PM +0100, Rupesh Gujare wrote:
>> +/*------------------------------------------------------------------------------
>> * Context: softirq-serialized
>> */
> Don't resend the patch, but these comments are not in kernel style.
> It's explained in Documentation/kernel-doc-nano-HOWTO.txt
>
> The main thing is that could you just delete all the --------
> lines?
OK Dan, looks reasonable.
--
Regards,
Rupesh Gujare