Messages in this thread | | | Subject | Re: [RFC PATCH] softirq: fix tasklet_kill() usage and users | From | Santosh Shilimkar <> | Date | Sat, 6 Aug 2016 11:07:14 -0700 |
| |
ping !!
On 8/1/2016 9:13 PM, Santosh Shilimkar wrote: > Semantically the expectation from the tasklet init/kill API > should be as below. > > tasklet_init() == Init and Enable scheduling > tasklet_kill() == Disable scheduling and Destroy > > tasklet_init() API exibit above behavior but not the > tasklet_kill(). The tasklet handler can still get scheduled > and run even after the tasklet_kill(). > > There are 2, 3 places where drivers are working around > this issue by calling tasklet_disable() which will add an > usecount and there by avoiding the handlers being called. > One of the example 'commit 1e1257860fd1 > ("tty/serial: at91: correct the usage of tasklet")' > > tasklet_enable/tasklet_disable is a pair API and expected > to be used together. Usage of tasklet_disable() *just* to > workround tasklet scheduling after kill is probably not the > correct and inteded use of the API as done the API. > We also happen to see similar issue where in shutdown path > the tasklet_handler was getting called even after the > tasklet_kill(). > > We can fix this be making sure tasklet_kill() does right > thing and there by ensuring tasklet handler won't run after > tasklet_kil() with very simple change. Patch fixes the tasklet > code and also few drivers hacks to workaround the issue. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Tadeusz Struk <tadeusz.struk@intel.com> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Paul Bolle <pebolle@tiscali.nl> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > > Signed-off-by: Santosh Shilimkar <ssantosh@kernel.org> > --- > drivers/crypto/qat/qat_common/adf_isr.c | 1 - > drivers/crypto/qat/qat_common/adf_sriov.c | 1 - > drivers/crypto/qat/qat_common/adf_vf_isr.c | 2 -- > drivers/isdn/gigaset/interface.c | 1 - > drivers/tty/serial/atmel_serial.c | 1 - > kernel/softirq.c | 7 ++++--- > 6 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/crypto/qat/qat_common/adf_isr.c b/drivers/crypto/qat/qat_common/adf_isr.c > index 06d4901..fd5e900 100644 > --- a/drivers/crypto/qat/qat_common/adf_isr.c > +++ b/drivers/crypto/qat/qat_common/adf_isr.c > @@ -296,7 +296,6 @@ static void adf_cleanup_bh(struct adf_accel_dev *accel_dev) > int i; > > for (i = 0; i < hw_data->num_banks; i++) { > - tasklet_disable(&priv_data->banks[i].resp_handler); > tasklet_kill(&priv_data->banks[i].resp_handler); > } > } > diff --git a/drivers/crypto/qat/qat_common/adf_sriov.c b/drivers/crypto/qat/qat_common/adf_sriov.c > index 4a526e2..9e65888 100644 > --- a/drivers/crypto/qat/qat_common/adf_sriov.c > +++ b/drivers/crypto/qat/qat_common/adf_sriov.c > @@ -204,7 +204,6 @@ void adf_disable_sriov(struct adf_accel_dev *accel_dev) > } > > for (i = 0, vf = accel_dev->pf.vf_info; i < totalvfs; i++, vf++) { > - tasklet_disable(&vf->vf2pf_bh_tasklet); > tasklet_kill(&vf->vf2pf_bh_tasklet); > mutex_destroy(&vf->pf2vf_lock); > } > diff --git a/drivers/crypto/qat/qat_common/adf_vf_isr.c b/drivers/crypto/qat/qat_common/adf_vf_isr.c > index aa689ca..81e63bf 100644 > --- a/drivers/crypto/qat/qat_common/adf_vf_isr.c > +++ b/drivers/crypto/qat/qat_common/adf_vf_isr.c > @@ -191,7 +191,6 @@ static int adf_setup_pf2vf_bh(struct adf_accel_dev *accel_dev) > > static void adf_cleanup_pf2vf_bh(struct adf_accel_dev *accel_dev) > { > - tasklet_disable(&accel_dev->vf.pf2vf_bh_tasklet); > tasklet_kill(&accel_dev->vf.pf2vf_bh_tasklet); > mutex_destroy(&accel_dev->vf.vf2pf_lock); > } > @@ -268,7 +267,6 @@ static void adf_cleanup_bh(struct adf_accel_dev *accel_dev) > { > struct adf_etr_data *priv_data = accel_dev->transport; > > - tasklet_disable(&priv_data->banks[0].resp_handler); > tasklet_kill(&priv_data->banks[0].resp_handler); > } > > diff --git a/drivers/isdn/gigaset/interface.c b/drivers/isdn/gigaset/interface.c > index 600c79b..2ce63b6 100644 > --- a/drivers/isdn/gigaset/interface.c > +++ b/drivers/isdn/gigaset/interface.c > @@ -524,7 +524,6 @@ void gigaset_if_free(struct cardstate *cs) > if (!drv->have_tty) > return; > > - tasklet_disable(&cs->if_wake_tasklet); > tasklet_kill(&cs->if_wake_tasklet); > cs->tty_dev = NULL; > tty_unregister_device(drv->tty, cs->minor_index); > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index 954941d..27e638e 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -1915,7 +1915,6 @@ static void atmel_shutdown(struct uart_port *port) > * Clear out any scheduled tasklets before > * we destroy the buffers > */ > - tasklet_disable(&atmel_port->tasklet); > tasklet_kill(&atmel_port->tasklet); > > /* > diff --git a/kernel/softirq.c b/kernel/softirq.c > index 17caf4b..21397eb 100644 > --- a/kernel/softirq.c > +++ b/kernel/softirq.c > @@ -498,7 +498,7 @@ static void tasklet_action(struct softirq_action *a) > list = list->next; > > if (tasklet_trylock(t)) { > - if (!atomic_read(&t->count)) { > + if (atomic_read(&t->count) == 1) { > if (!test_and_clear_bit(TASKLET_STATE_SCHED, > &t->state)) > BUG(); > @@ -534,7 +534,7 @@ static void tasklet_hi_action(struct softirq_action *a) > list = list->next; > > if (tasklet_trylock(t)) { > - if (!atomic_read(&t->count)) { > + if (atomic_read(&t->count) == 1) { > if (!test_and_clear_bit(TASKLET_STATE_SCHED, > &t->state)) > BUG(); > @@ -559,7 +559,7 @@ void tasklet_init(struct tasklet_struct *t, > { > t->next = NULL; > t->state = 0; > - atomic_set(&t->count, 0); > + atomic_set(&t->count, 1); > t->func = func; > t->data = data; > } > @@ -576,6 +576,7 @@ void tasklet_kill(struct tasklet_struct *t) > } while (test_bit(TASKLET_STATE_SCHED, &t->state)); > } > tasklet_unlock_wait(t); > + atomic_dec(&t->count); > clear_bit(TASKLET_STATE_SCHED, &t->state); > } > EXPORT_SYMBOL(tasklet_kill); >
| |