From: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Anna-Maria Gleixner <[email protected]>
Cc: Oliver Neukum <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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;
On Thu, Aug 31, 2017 at 12:23:46PM -0000, Anna-Maria Gleixner wrote:
> From: Thomas Gleixner <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Anna-Maria Gleixner <[email protected]>
> Cc: Oliver Neukum <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
Acked-by: Greg Kroah-Hartman <[email protected]>
Anna-Maria Gleixner <[email protected]> writes:
> From: Thomas Gleixner <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Anna-Maria Gleixner <[email protected]>
> Cc: Oliver Neukum <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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
From: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Anna-Maria Gleixner <[email protected]>
Cc: Oliver Neukum <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Cc: [email protected]
Acked-by: Greg Kroah-Hartman <[email protected]>
[bigeasy: using usb_get_intfdata() as suggested by Bjørn Mork]
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
On 2017-08-31 15:57:04 [+0200], Bjørn Mork wrote:
> 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):
I think so, too. Still untested as I don't have a working gadget around.
v1…v2: Updated as suggested by Bjørn and added Greg's Acked-by.
drivers/net/usb/cdc_ncm.c | 36 +++++++++++++++---------------------
include/linux/usb/cdc_ncm.h | 1 -
2 files changed, 15 insertions(+), 22 deletions(-)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8f572b9f3625..42f7bd90e6a4 100644
--- 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,8 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
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;
atomic_set(&ctx->stop, 0);
spin_lock_init(&ctx->mtx);
@@ -967,10 +964,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
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 +1342,9 @@ static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx)
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 = usb_get_intfdata(ctx->control);
spin_lock_bh(&ctx->mtx);
if (ctx->tx_timer_pending != 0) {
@@ -1379,6 +1362,17 @@ static void cdc_ncm_txpath_bh(unsigned long param)
}
}
+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)
{
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index 1a59699cf82a..62b506fddf8d 100644
--- 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;
--
2.14.1