Re: [PATCH 4/4] staging: ozwpan: Create deferred work to destroy PD object.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] staging: ozwpan: Create deferred work to destroy PD object.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] staging: ozwpan: Create deferred work to destroy PD object.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] staging: ozwpan: Create deferred work to destroy PD object.
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] staging: ozwpan: Create deferred work to destroy PD object.
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html