Skip to content

Commit

Permalink
net: cdc_ncm: avoid changing RX/TX buffers on MTU changes
Browse files Browse the repository at this point in the history
NCM buffer sizes are negotiated with the device independently of
the network device MTU.  The RX buffers are allocated by the
usbnet framework based on the rx_urb_size value set by cdc_ncm. A
single RX buffer can hold a number of MTU sized packets.

The default usbnet change_mtu ndo only modifies rx_urb_size if it
is equal to hard_mtu.  And the cdc_ncm driver will set rx_urb_size
and hard_mtu independently of each other, based on dwNtbInMaxSize
and dwNtbOutMaxSize respectively. It was therefore assumed that
usbnet_change_mtu() would never touch rx_urb_size.  This failed to
consider the case where dwNtbInMaxSize and dwNtbOutMaxSize happens
to be equal.

Fix by implementing an NCM specific change_mtu ndo, modifying the
netdev MTU without touching the buffer size settings.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Bjørn Mork authored and David S. Miller committed Dec 24, 2015
1 parent 184fc8b commit 1dfddff
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 1 deletion.
2 changes: 1 addition & 1 deletion drivers/net/usb/cdc_mbim.c
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ static const struct net_device_ops cdc_mbim_netdev_ops = {
.ndo_stop = usbnet_stop,
.ndo_start_xmit = usbnet_start_xmit,
.ndo_tx_timeout = usbnet_tx_timeout,
.ndo_change_mtu = usbnet_change_mtu,
.ndo_change_mtu = cdc_ncm_change_mtu,
.ndo_set_mac_address = eth_mac_addr,
.ndo_validate_addr = eth_validate_addr,
.ndo_vlan_rx_add_vid = cdc_mbim_rx_add_vid,
Expand Down
31 changes: 31 additions & 0 deletions drivers/net/usb/cdc_ncm.c
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include <linux/module.h>
#include <linux/netdevice.h>
#include <linux/ctype.h>
#include <linux/etherdevice.h>
#include <linux/ethtool.h>
#include <linux/workqueue.h>
#include <linux/mii.h>
Expand Down Expand Up @@ -689,6 +690,33 @@ static void cdc_ncm_free(struct cdc_ncm_ctx *ctx)
kfree(ctx);
}

/* we need to override the usbnet change_mtu ndo for two reasons:
* - respect the negotiated maximum datagram size
* - avoid unwanted changes to rx and tx buffers
*/
int cdc_ncm_change_mtu(struct net_device *net, int new_mtu)
{
struct usbnet *dev = netdev_priv(net);
struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
int maxmtu = ctx->max_datagram_size - cdc_ncm_eth_hlen(dev);

if (new_mtu <= 0 || new_mtu > maxmtu)
return -EINVAL;
net->mtu = new_mtu;
return 0;
}
EXPORT_SYMBOL_GPL(cdc_ncm_change_mtu);

static const struct net_device_ops cdc_ncm_netdev_ops = {
.ndo_open = usbnet_open,
.ndo_stop = usbnet_stop,
.ndo_start_xmit = usbnet_start_xmit,
.ndo_tx_timeout = usbnet_tx_timeout,
.ndo_change_mtu = cdc_ncm_change_mtu,
.ndo_set_mac_address = eth_mac_addr,
.ndo_validate_addr = eth_validate_addr,
};

int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting, int drvflags)
{
struct cdc_ncm_ctx *ctx;
Expand Down Expand Up @@ -823,6 +851,9 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
/* add our sysfs attrs */
dev->net->sysfs_groups[0] = &cdc_ncm_sysfs_attr_group;

/* must handle MTU changes */
dev->net->netdev_ops = &cdc_ncm_netdev_ops;

return 0;

error2:
Expand Down
1 change: 1 addition & 0 deletions include/linux/usb/cdc_ncm.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ struct cdc_ncm_ctx {
};

u8 cdc_ncm_select_altsetting(struct usb_interface *intf);
int cdc_ncm_change_mtu(struct net_device *net, int new_mtu);
int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_altsetting, int drvflags);
void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf);
struct sk_buff *cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign);
Expand Down

0 comments on commit 1dfddff

Please sign in to comment.