Bjørn Mork <bjorn@xxxxxxx> writes:
> [48880.048494] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
> [48880.048573] IP: [<ffffffffa06ba879>] cdc_ncm_tx_bundle+0x168/0x43b [cdc_ncm]
This one is because you removed the "if (skb == NULL)" from the for
loop, but left the "skb = NULL;" at the end:
@@ -719,28 +707,15 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
/* compute maximum buffer size */
rem = ctx->tx_max - offset;
- if (skb == NULL) {
- skb = ctx->tx_rem_skb;
- ctx->tx_rem_skb = NULL;
-
- /* check for end of skb */
- if (skb == NULL)
- break;
- }
-
if (skb->len > rem) {
if (n == 0) {
/* won't fit, MTU problem? */
dev_kfree_skb_any(skb);
skb = NULL;
ctx->netdev->stats.tx_dropped++;
+ error = 1;
} else {
- /* no room for skb - store for later */
- if (ctx->tx_rem_skb != NULL) {
- dev_kfree_skb_any(ctx->tx_rem_skb);
- ctx->netdev->stats.tx_dropped++;
- }
- ctx->tx_rem_skb = skb;
+
skb = NULL;
ready2send = 1;
}
@@ -768,13 +743,6 @@ cdc_ncm_fill_tx_frame(struct cdc_ncm_ctx *ctx, struct sk_buff *skb)
skb = NULL;
}
If I understand your intentions with this code, then the for loop should
probably just go away completely?
Anyway, after avoiding that one, I ended up with
[ 857.112692] cdc_ncm_fill_tx_frame: skb_out=ffff880218624cc0, length=2048
[ 857.112696] cdc_ncm_tx_fixup: returning (null)
[ 857.112731] BUG: unable to handle kernel NULL pointer dereference at 0000000000000068
[ 857.112807] IP: [<ffffffffa030e68b>] usbnet_start_xmit+0x128/0x4e9 [usbnet]
No need for the reset of the trace here. Removing the "goto not_drop"
makes usbnet_start_xmit continue after tx_fixup returns NULL:
- if (info->tx_fixup) {
+ if (transmit_now && info->tx_fixup) {
skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
if (!skb) {
if (netif_msg_tx_err(dev)) {
netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
goto drop;
- } else {
- /* cdc_ncm collected packet; waits for more */
- goto not_drop;
}
}
}
// some devices want funky USB-level framing, for
// win32 driver (usually) and/or hardware quirks
if (transmit_now && info->tx_fixup) {
skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
if (!skb) {
if (netif_msg_tx_err(dev)) {
netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
goto drop;
}
}
}
length = skb->len;
I stopped there. I am not exactly sure where you were going with this
anymore. tx_fixup will return the tx_curr_skb being currently built
every time it is called, without anything resetting it at that point.
Then when finally cdc_ncm_fill_tx_frame has been called enough times to
fill it completely, you end up just dropping it on the floor. The
result is that nothing is ever transmitted even after fixing the above
errors because the tx_curr_skb has an incomplete header every time
tx_fixup returns it.
I do like your idea, but if you don't mind I think we'll go ahead and
base the upcoming cdc_mbim driver on the current cdc_ncm *with* the
timer. The plan is to reuse as much of cdc_ncm as possible, so in
theory it should be easy to update both drivers when the timer goes away
(there is no need for cdc_mbim be aware of the timer at all). But
unfortunately it seems that cdc_ncm_fill_tx_frame must be changed a lot
to support the concept of multiple NDPs in a single NTB. This means
that the timer removal patches will conflict heavily with the
preparations for MBIM support. With some luck and acceptance from all
contributors, I hope to be able to post the first version of that as the
merge window closes.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
