diff mbox

usbnet: drop unneeded check for NULL

Message ID 1346768514-19823-1-git-send-email-oliver@neukum.org
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Oliver Neukum Sept. 4, 2012, 2:21 p.m. UTC
usbnet_start_xmit() is always called with a valid skb

Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
 drivers/net/usb/usbnet.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

Comments

Richard Cochran Sept. 4, 2012, 3:53 p.m. UTC | #1
On Tue, Sep 04, 2012 at 04:21:54PM +0200, Oliver Neukum wrote:
> usbnet_start_xmit() is always called with a valid skb

So, has the problem that this test worked around been fixed?

Thanks,
Richard

 
> Signed-off-by: Oliver Neukum <oneukum@suse.de>
> ---
>  drivers/net/usb/usbnet.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 8531c1c..5234d20 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1092,8 +1092,7 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
>  	unsigned long		flags;
>  	int retval;
>  
> -	if (skb)
> -		skb_tx_timestamp(skb);
> +	skb_tx_timestamp(skb);
>  
>  	// some devices want funky USB-level framing, for
>  	// win32 driver (usually) and/or hardware quirks
> -- 
> 1.7.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum Sept. 4, 2012, 4:13 p.m. UTC | #2
On Tuesday 04 September 2012 17:53:43 Richard Cochran wrote:
> On Tue, Sep 04, 2012 at 04:21:54PM +0200, Oliver Neukum wrote:
> > usbnet_start_xmit() is always called with a valid skb
> 
> So, has the problem that this test worked around been fixed?

netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
                                     struct net_device *net)
{
        struct usbnet           *dev = netdev_priv(net);
        int                     length;
        struct urb              *urb = NULL;
        struct skb_data         *entry;
        struct driver_info      *info = dev->driver_info;
        unsigned long           flags;
        int retval;

        if (skb)
                skb_tx_timestamp(skb);

        // some devices want funky USB-level framing, for
        // win32 driver (usually) and/or hardware quirks
        if (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;
                        }
                }
        }
        length = skb->len;

If that check is ever needed and tx_fixup not needed, the driver will oops here.
The check is wrong in any case.

	Regards
		Oliver
 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Sept. 4, 2012, 4:37 p.m. UTC | #3
From: Oliver Neukum <oliver@neukum.org>
Date: Tue, 04 Sep 2012 18:13:18 +0200

> If that check is ever needed and tx_fixup not needed, the driver will oops here.
> The check is wrong in any case.

Right, we are dealing with two different things here.

Tree-wide there was a blind set of changes to protect skb_tx_timestamp()
calls with a NULL skb check.

But in this specific case, it's completely unnecessary.

So Oliver's change is definitely correct and I will add it to net-next

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Cochran Sept. 5, 2012, 4:47 a.m. UTC | #4
On Tue, Sep 04, 2012 at 12:37:40PM -0400, David Miller wrote:
> From: Oliver Neukum <oliver@neukum.org>
> Date: Tue, 04 Sep 2012 18:13:18 +0200
> 
> > If that check is ever needed and tx_fixup not needed, the driver will oops here.
> > The check is wrong in any case.
> 
> Right, we are dealing with two different things here.
> 
> Tree-wide there was a blind set of changes to protect skb_tx_timestamp()
> calls with a NULL skb check.
> 
> But in this specific case, it's completely unnecessary.
> 
> So Oliver's change is definitely correct and I will add it to net-next

Looking at git blame on usbnet.c we see ...

    23ba0799	if (skb)
    23ba0799		skb_tx_timestamp(skb);

and reading on ...

    commit 23ba07991dad5a96a024c1b45cb602eef5f83df8
    Author: Konstantin Khlebnikov <khlebnikov@openvz.org>
    Date:   Mon Nov 7 05:54:58 2011 +0000

    usbnet: fix oops in usbnet_start_xmit
    
    This patch fixes the bug added in commit v3.1-rc7-1055-gf9b491e
    SKB can be NULL at this point, at least for cdc-ncm.
    
    Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
    Acked-by: Richard Cochran <richardcochran@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

and finally cdc-ncm.c reveals

static void cdc_ncm_txpath_bh(unsigned long param)
{
	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)param;

	spin_lock_bh(&ctx->mtx);
	if (ctx->tx_timer_pending != 0) {
		ctx->tx_timer_pending--;
		cdc_ncm_tx_timeout_start(ctx);
		spin_unlock_bh(&ctx->mtx);
	} else if (ctx->netdev != NULL) {
		spin_unlock_bh(&ctx->mtx);
		netif_tx_lock_bh(ctx->netdev);
		usbnet_start_xmit(NULL, ctx->netdev);
----------------------------------^^^^
		netif_tx_unlock_bh(ctx->netdev);
	}
}

and so I think the problem that the test addresses is still present,
or am I missing something?

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum Sept. 5, 2012, 6:24 a.m. UTC | #5
On Wednesday 05 September 2012 06:47:12 Richard Cochran wrote:
> and so I think the problem that the test addresses is still present,
> or am I missing something?

No,

you are right. Thank you.

Dave, for now, please don't apply this patch. In the long run, this crap
in cdc-ncm needs to go. I am starting rewriting this driver right now.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Sept. 5, 2012, 4:50 p.m. UTC | #6
From: Oliver Neukum <oneukum@suse.de>
Date: Wed, 05 Sep 2012 08:24:25 +0200

> On Wednesday 05 September 2012 06:47:12 Richard Cochran wrote:
>> and so I think the problem that the test addresses is still present,
>> or am I missing something?
> 
> No,
> 
> you are right. Thank you.
> 
> Dave, for now, please don't apply this patch. In the long run, this crap
> in cdc-ncm needs to go. I am starting rewriting this driver right now.

I already applied it several days ago, someone send me a revert with a
verbose commit message explaining the situation.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings Sept. 5, 2012, 10:14 p.m. UTC | #7
On Wed, 2012-09-05 at 12:50 -0400, David Miller wrote:
> From: Oliver Neukum <oneukum@suse.de>
> Date: Wed, 05 Sep 2012 08:24:25 +0200
> 
> > On Wednesday 05 September 2012 06:47:12 Richard Cochran wrote:
> >> and so I think the problem that the test addresses is still present,
> >> or am I missing something?
> > 
> > No,
> > 
> > you are right. Thank you.
> > 
> > Dave, for now, please don't apply this patch. In the long run, this crap
> > in cdc-ncm needs to go. I am starting rewriting this driver right now.
> 
> I already applied it several days ago, someone send me a revert with a
> verbose commit message explaining the situation.

cdc-ncm is aggregating skbs into jumbo USB packets and then, because the
netdev is not signalled when the qdisc stops transmitting, flushing them
after one scheduler tick.  Flushing is done by calling
usbnet_start_xmit() with a null skb pointer and then substituting a real
skb in the tx_fixup callback.

Perhaps the skb_tx_timestamp() call should be moved below the
'if (info->tx_fixup)' block, at which point skb is definitely non-null.
It doesn't look like cdc-ncm will provide useful timestamps either way.

cdc-ncm's aggregation could be improved by either (1) implementing some
type of GSO with appropriate gso_max_size and gso_max_segs limits (2)
adding an explicit transmit flushing operation, similar to that
Alexander Duyck proposed.

Ben.
Richard Cochran Sept. 6, 2012, 7:52 a.m. UTC | #8
On Wed, Sep 05, 2012 at 11:14:35PM +0100, Ben Hutchings wrote:
> 
> Perhaps the skb_tx_timestamp() call should be moved below the
> 'if (info->tx_fixup)' block, at which point skb is definitely non-null.

No, that block shuffles the packet alignment all around, and spoils
any code which needs to match packets with time stamps.

> It doesn't look like cdc-ncm will provide useful timestamps either way.

But it is not the only user of this code. Furthermore, the PHY driver
will want a chance to provide a HW time stamp in any case.

Thanks,
Richard


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 8531c1c..5234d20 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1092,8 +1092,7 @@  netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 	unsigned long		flags;
 	int retval;
 
-	if (skb)
-		skb_tx_timestamp(skb);
+	skb_tx_timestamp(skb);
 
 	// some devices want funky USB-level framing, for
 	// win32 driver (usually) and/or hardware quirks