Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751868AbdHaN5z (ORCPT ); Thu, 31 Aug 2017 09:57:55 -0400 Received: from canardo.mork.no ([148.122.252.1]:39921 "EHLO canardo.mork.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751011AbdHaN5x (ORCPT ); Thu, 31 Aug 2017 09:57:53 -0400 From: =?utf-8?Q?Bj=C3=B8rn_Mork?= To: Anna-Maria Gleixner Cc: LKML , Peter Zijlstra , Ingo Molnar , Christoph Hellwig , keescook@chromium.org, John Stultz , Thomas Gleixner , Oliver Neukum , Greg Kroah-Hartman , linux-usb@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [PATCH 24/25] net/cdc_ncm: Replace tasklet with softirq hrtimer Organization: m References: <20170831105725.809317030@linutronix.de> <20170831105827.479650817@linutronix.de> Date: Thu, 31 Aug 2017 15:57:04 +0200 In-Reply-To: <20170831105827.479650817@linutronix.de> (Anna-Maria Gleixner's message of "Thu, 31 Aug 2017 12:23:46 -0000") Message-ID: <87mv6fzvpr.fsf@miraculix.mork.no> User-Agent: Gnus/5.130015 (Ma Gnus v0.15) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v7VDw0JY031915 Content-Length: 3974 Lines: 126 Anna-Maria Gleixner writes: > From: Thomas Gleixner > > The bh tasklet is used in invoke the hrtimer (cdc_ncm_tx_timer_cb) in > softirq context. This can be also achieved without the tasklet but with > CLOCK_MONOTONIC_SOFT as hrtimer base. > > Signed-off-by: Thomas Gleixner > Signed-off-by: Anna-Maria Gleixner > Cc: Oliver Neukum > Cc: Greg Kroah-Hartman > Cc: linux-usb@vger.kernel.org > Cc: netdev@vger.kernel.org > --- > drivers/net/usb/cdc_ncm.c | 37 ++++++++++++++++--------------------- > include/linux/usb/cdc_ncm.h | 2 +- > 2 files changed, 17 insertions(+), 22 deletions(-) > > --- a/drivers/net/usb/cdc_ncm.c > +++ b/drivers/net/usb/cdc_ncm.c > @@ -61,7 +61,6 @@ static bool prefer_mbim; > module_param(prefer_mbim, bool, S_IRUGO | S_IWUSR); > MODULE_PARM_DESC(prefer_mbim, "Prefer MBIM setting on dual NCM/MBIM functions"); > > -static void cdc_ncm_txpath_bh(unsigned long param); > static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx); > static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer); > static struct usb_driver cdc_ncm_driver; > @@ -777,10 +776,9 @@ int cdc_ncm_bind_common(struct usbnet *d > if (!ctx) > return -ENOMEM; > > - hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > + hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL); > ctx->tx_timer.function = &cdc_ncm_tx_timer_cb; > - ctx->bh.data = (unsigned long)dev; > - ctx->bh.func = cdc_ncm_txpath_bh; > + ctx->usbnet = dev; > atomic_set(&ctx->stop, 0); > spin_lock_init(&ctx->mtx); > > @@ -967,10 +965,7 @@ void cdc_ncm_unbind(struct usbnet *dev, > > atomic_set(&ctx->stop, 1); > > - if (hrtimer_active(&ctx->tx_timer)) > - hrtimer_cancel(&ctx->tx_timer); > - > - tasklet_kill(&ctx->bh); > + hrtimer_cancel(&ctx->tx_timer); > > /* handle devices with combined control and data interface */ > if (ctx->control == ctx->data) > @@ -1348,20 +1343,9 @@ static void cdc_ncm_tx_timeout_start(str > HRTIMER_MODE_REL); > } > > -static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer) > +static void cdc_ncm_txpath_bh(struct cdc_ncm_ctx *ctx) > { > - struct cdc_ncm_ctx *ctx = > - container_of(timer, struct cdc_ncm_ctx, tx_timer); > - > - if (!atomic_read(&ctx->stop)) > - tasklet_schedule(&ctx->bh); > - return HRTIMER_NORESTART; > -} > - > -static void cdc_ncm_txpath_bh(unsigned long param) > -{ > - struct usbnet *dev = (struct usbnet *)param; > - struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0]; > + struct usbnet *dev = ctx->usbnet; > > spin_lock_bh(&ctx->mtx); > if (ctx->tx_timer_pending != 0) { > @@ -1379,6 +1363,17 @@ static void cdc_ncm_txpath_bh(unsigned l > } > } > > +static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer) > +{ > + struct cdc_ncm_ctx *ctx = > + container_of(timer, struct cdc_ncm_ctx, tx_timer); > + > + if (!atomic_read(&ctx->stop)) > + cdc_ncm_txpath_bh(ctx); > + > + return HRTIMER_NORESTART; > +} > + > struct sk_buff * > cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags) > { > --- a/include/linux/usb/cdc_ncm.h > +++ b/include/linux/usb/cdc_ncm.h > @@ -92,7 +92,6 @@ > struct cdc_ncm_ctx { > struct usb_cdc_ncm_ntb_parameters ncm_parm; > struct hrtimer tx_timer; > - struct tasklet_struct bh; > > const struct usb_cdc_ncm_desc *func_desc; > const struct usb_cdc_mbim_desc *mbim_desc; > @@ -101,6 +100,7 @@ struct cdc_ncm_ctx { > > struct usb_interface *control; > struct usb_interface *data; > + struct usbnet *usbnet; > > struct sk_buff *tx_curr_skb; > struct sk_buff *tx_rem_skb; I believe the struct usbnet pointer is redundant. We already have lots of pointers back and forth here. This should work, but is not tested: struct usbnet *dev = usb_get_intfdata(ctx->control): Bjørn