[PATCH 1/4] ozwpan: staging: Fix crash for race condition.

[PATCH 1/4] ozwpan: staging: Fix crash for race condition.

[Rupesh Gujare]

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 <rupesh.gujare@atmel.com>
---
 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

[PATCH 2/4] staging: ozwpan: Check error condition before creating endpoint.

[Rupesh Gujare]

Check if interface number is correct before creating an end point.

Signed-off-by: Rupesh Gujare <rupesh.gujare@atmel.com>
---
 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

[PATCH 3/4] staging: ozwpan: Increment reference counter.

[Rupesh Gujare]

Increment PD reference counter, on every timer event so that
we do not loose PD object by mistake.

Signed-off-by: Rupesh Gujare <rupesh.gujare@atmel.com>
---
 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

[PATCH 4/4] staging: ozwpan: Create deferred work to destroy PD object.

[Rupesh Gujare]

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 <rupesh.gujare@atmel.com>
---
 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

Re: [PATCH 4/4] staging: ozwpan: Create deferred work to destroy PD object.

[Sergei Shtylyov]

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 <rupesh.gujare@atmel.com>
> ---
>   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

Re: [PATCH 4/4] staging: ozwpan: Create deferred work to destroy PD object.

[Rupesh Gujare]

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

Re: [PATCH 4/4] staging: ozwpan: Create deferred work to destroy PD object.

[Dan Carpenter]

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

Re: [PATCH 4/4] staging: ozwpan: Create deferred work to destroy PD object.

[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