| Message ID | 1394952550-9308-2-git-send-email-benchan@chromium.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Is this *really* driver material, or should we just leave the IP MTU
hint handling up to the userspace management application?
There are no less than 3(!) different ways for a device to specify the
MBIM MTU:
1) wMaxSegmentSize field of the "MBIM Control Model Functional
Descriptor"
(mandatory, pre-errata1, per-device, both IP and DSS)
2) IPv4Mtu/IPv6Mtu fields of the MBIM_IP_CONFIGURATION_INFO response to
MBIM_CID_IP_CONFIGURATION
(mandatory, pre-errata1, per-session, IP only)
3) wMTU field of the "MBIM Extended Functional Descriptor"
(optional, errata1, per-operator, IP only)
I see the optional extended MBIM descriptor as no more than a hint, and
given that MBIM_CID_IP_CONFIGURATION is mandatory I see no real use case
at all.
If the extended decriptor is there, and the application supports it,
then fine - let the application base the default IP MTU on this
value. But I cannot see anything possibly break for an application and
driver which does not support it. The fact that the USB-IF didn't
bother to update the MBIM version number when they added this descriptor
supports that claim IMHO. It tells us that it is perfectly OK for any
MBIM v1.0 compliant application/driver to not even parse this
descriptor, because the application/driver can predate Errata1. The
descriptor was added out of the blue in something called an "Errata",
and is IMHO best ignored until a proper new version of the standard is
published.
The driver currently set the MTU based on the wMaxSegmentSize from the
mandatory MBIM descriptor. AFAICS this is the only field the driver
*can* use. The MBIM management application may make use of the extended
descriptor, because it will know about "operator" and the relation to
"IP data streams". The driver does not have this knowledge, and it must
be capable of supporting non-IP streams (DSS) multiplexed over the same
network device.
AFAICS, the MTU information provided by the extended descriptor is
completely redundant for all cases where it has a meaning (the mandatory
MBIM_CID_IP_CONFIGURATION will provde the IPv4 and IPv6 per-session
MTUs), and possily completely wrong for any other case (wMaxSegmentSize
sets the maximum size for non-IP sessions)
My advice is to leave parsing of this descriptor to userspace. That is
also the only place where we can make use of the
bMaxOutstandingCommandMessages, which actually is useful information. I
just don't understand why they had to make that part of a descriptor
when they have defined a new dedicated and extensible management
protocol....
In case you disagree with the above (and do feel free to do so... I am
often wrong), some minor nits regarding the actual implementation:
Ben Chan <benchan@chromium.org> writes:
> According to "Universal Serial Bus Communications Class Subclass
> Specification for Mobile Broadband Interface Model, Revision 1.0,
> Errata-1" published by USB-IF, the wMTU field of the MBIM extended
> functional descriptor indicates the operator preferred MTU for IP data
> streams.
>
> This patch modifies cdc_ncm_setup to ensure that the MTU value set on
> the usbnet device does not exceed the operator preferred MTU indicated
> by wMTU if the MBIM device exposes a MBIM extended functional
> descriptor.
>
> Signed-off-by: Ben Chan <benchan@chromium.org>
> ---
> drivers/net/usb/cdc_ncm.c | 15 +++++++++++++++
> include/linux/usb/cdc_ncm.h | 1 +
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index dbff290..0b036ed 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -261,6 +261,13 @@ out:
> /* set MTU to max supported by the device if necessary */
> if (dev->net->mtu > ctx->max_datagram_size - eth_hlen)
> dev->net->mtu = ctx->max_datagram_size - eth_hlen;
> +
> + /* do not exceed operater preferred MTU */
> + if (ctx->mbim_extended_desc &&
> + ctx->mbim_extended_desc->wMTU != 0 &&
> + dev->net->mtu > ctx->mbim_extended_desc->wMTU)
> + dev->net->mtu = ctx->mbim_extended_desc->wMTU;
wMTU access needs le16_to_cpu.
> +
> return 0;
> }
>
> @@ -399,6 +406,14 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
> ctx->mbim_desc = (const struct usb_cdc_mbim_desc *)buf;
> break;
>
> + case USB_CDC_MBIM_EXTENDED_TYPE:
> + if (buf[0] < sizeof(*(ctx->mbim_extended_desc)))
> + break;
> +
> + ctx->mbim_extended_desc =
> + (const struct usb_cdc_mbim_extended_desc *)buf;
> + break;
> +
> default:
> break;
> }
> diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
> index c3fa807..bdf05fb 100644
> --- a/include/linux/usb/cdc_ncm.h
> +++ b/include/linux/usb/cdc_ncm.h
> @@ -93,6 +93,7 @@ struct cdc_ncm_ctx {
>
> const struct usb_cdc_ncm_desc *func_desc;
> const struct usb_cdc_mbim_desc *mbim_desc;
> + const struct usb_cdc_mbim_extended_desc *mbim_extended_desc;
> const struct usb_cdc_ether_desc *ether_desc;
>
> struct usb_interface *control;
Could we move this final MTU correction from cdc_ncm_setup to
cdc_ncm_bind_common to avoid bloating the device struct with another
descriptor pointer we donæt really need to keep around?
I know we look into descriptors in cdc_ncm_setup, because we have to,
but ideally I would have loved to see cdc_ncm_setup dealing with *just*
the NCM/MBIM specific control setup messages and cdc_ncm_bind_common
dealing with all the functional descriptors. That seems most logic to
me (but is of course only my personal opinion and nothing else - I do
not know what the original cdc_ncm author intended)
Bjørn
--
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
On Mon, Mar 17, 2014 at 2:42 AM, Bjørn Mork <bjorn@mork.no> wrote: > Is this *really* driver material, or should we just leave the IP MTU > hint handling up to the userspace management application? > > There are no less than 3(!) different ways for a device to specify the > MBIM MTU: > > 1) wMaxSegmentSize field of the "MBIM Control Model Functional > Descriptor" > (mandatory, pre-errata1, per-device, both IP and DSS) > > 2) IPv4Mtu/IPv6Mtu fields of the MBIM_IP_CONFIGURATION_INFO response to > MBIM_CID_IP_CONFIGURATION > (mandatory, pre-errata1, per-session, IP only) > > 3) wMTU field of the "MBIM Extended Functional Descriptor" > (optional, errata1, per-operator, IP only) > > I see the optional extended MBIM descriptor as no more than a hint, and > given that MBIM_CID_IP_CONFIGURATION is mandatory I see no real use case > at all. > > If the extended decriptor is there, and the application supports it, > then fine - let the application base the default IP MTU on this > value. But I cannot see anything possibly break for an application and > driver which does not support it. The fact that the USB-IF didn't > bother to update the MBIM version number when they added this descriptor > supports that claim IMHO. It tells us that it is perfectly OK for any > MBIM v1.0 compliant application/driver to not even parse this > descriptor, because the application/driver can predate Errata1. The > descriptor was added out of the blue in something called an "Errata", > and is IMHO best ignored until a proper new version of the standard is > published. > > The driver currently set the MTU based on the wMaxSegmentSize from the > mandatory MBIM descriptor. AFAICS this is the only field the driver > *can* use. The MBIM management application may make use of the extended > descriptor, because it will know about "operator" and the relation to > "IP data streams". The driver does not have this knowledge, and it must > be capable of supporting non-IP streams (DSS) multiplexed over the same > network device. > > AFAICS, the MTU information provided by the extended descriptor is > completely redundant for all cases where it has a meaning (the mandatory > MBIM_CID_IP_CONFIGURATION will provde the IPv4 and IPv6 per-session > MTUs), and possily completely wrong for any other case (wMaxSegmentSize > sets the maximum size for non-IP sessions) > > My advice is to leave parsing of this descriptor to userspace. That is > also the only place where we can make use of the > bMaxOutstandingCommandMessages, which actually is useful information. I > just don't understand why they had to make that part of a descriptor > when they have defined a new dedicated and extensible management > protocol.... > > In case you disagree with the above (and do feel free to do so... I am > often wrong), some minor nits regarding the actual implementation: > It's a bit messy how MTU is currently handled in MBIM. While wMTU may seem optional and redundant, it addresses some issues with wMaxSegmentSize and MBIM_CID_IP_CONFIGURATION, and hence why I suggest using wMTU when available: (1) wMaxSegmentSize The MBIM 1.0 errata-1 spec does suggest that wMaxSegmentSize must be at least 2048 and should not be used for determining IP MTU. However, some MBIM devices follow Microsoft's guideline, which suggests using wMaxSegmentSize to determine link MTU and its value should be between 1280 and 1500. The guideline may have been made before MBIM 1.0, but it clearly contradicts and violates the current spec. Unfortunately, it's followed by device vendors in practice. We could modify cdc_ncm not to have the lower bound (i.e. CDC_MBIM_MIN_DATAGRAM_SIZE = 2048) that it currently enforces. I don't feel like we should violate the spec in the driver if there are alternative solutions. (2) MBIM_CID_IP_CONFIGURATION MBIM_CID_IP_CONFIGURATION doesn't necessarily contain MTU information according to the spec. Bit 3 of IPv4ConfigurationAvailable / IPv6ConfigurationAvailable of MBIM_IP_CONFIGURATION_INFO indicates whether MTU information is available. As the Microsoft guideline also suggests that MBIM_CID_IP_CONFIGURATION wouldn't be used for MTU purpose, I wouldn't be too surprised that devices just don't bother to notify MTU via MBIM_CID_IP_CONFIGURATION. (3) wMTU The MBIM extended functional descriptor is optional, but device vendors do use it to indicate the MTU (mostly due to aforementioned confusion around wMaxSegmentSize). Using the wMTU field wouldn't add too much code or runtime overhead in kernel, so why don't we use it to set up the initial MTU when available? We could handle it in userspace, but I see the cdc_ncm driver is a better fit as it (like other net drivers) already sets up mtu and leaving the wMTU case would seem incomplete to me. While (1) and (2) are fixable, it's hard to convince device vendors to update their firmware just for that as carrier certifications impose a heavy cost of firmware changes. > wMTU access needs le16_to_cpu. Good catch. I will fix it in patch v2. > > Could we move this final MTU correction from cdc_ncm_setup to > cdc_ncm_bind_common to avoid bloating the device struct with another > descriptor pointer we donæt really need to keep around? > > I know we look into descriptors in cdc_ncm_setup, because we have to, > but ideally I would have loved to see cdc_ncm_setup dealing with *just* > the NCM/MBIM specific control setup messages and cdc_ncm_bind_common > dealing with all the functional descriptors. That seems most logic to > me (but is of course only my personal opinion and nothing else - I do > not know what the original cdc_ncm author intended) > I understand the argument against the extra descriptor pointer. But I think it's better to keep the mtu related code together so that one can easily see how MTU is determined when trying to change or refactor the code. I haven't looked into what cdc_ncm_setup was originally intended for. If we'd like to avoid adding an extra pointer in cdc_ncm_ctx, we could have cdc_ncm_bind passing a locally scoped context to cdc_ncm_setup. Thanks, Ben -- 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 Chan <benchan@chromium.org> writes: > It's a bit messy how MTU is currently handled in MBIM. While wMTU may > seem optional and redundant, it addresses some issues with > wMaxSegmentSize and MBIM_CID_IP_CONFIGURATION, and hence why I suggest > using wMTU when available: > > (1) wMaxSegmentSize > > The MBIM 1.0 errata-1 spec does suggest that wMaxSegmentSize must be > at least 2048 and should not be used for determining IP MTU. However, > some MBIM devices follow Microsoft's guideline, which suggests using > wMaxSegmentSize to determine link MTU and its value should be between > 1280 and 1500. The guideline may have been made before MBIM 1.0, but > it clearly contradicts and violates the current spec. Unfortunately, > it's followed by device vendors in practice. We could modify cdc_ncm > not to have the lower bound (i.e. CDC_MBIM_MIN_DATAGRAM_SIZE = 2048) > that it currently enforces. I don't feel like we should violate the > spec in the driver if there are alternative solutions. > > (2) MBIM_CID_IP_CONFIGURATION > > MBIM_CID_IP_CONFIGURATION doesn't necessarily contain MTU information > according to the spec. Bit 3 of IPv4ConfigurationAvailable / > IPv6ConfigurationAvailable of MBIM_IP_CONFIGURATION_INFO indicates > whether MTU information is available. As the Microsoft guideline also > suggests that MBIM_CID_IP_CONFIGURATION wouldn't be used for MTU > purpose, I wouldn't be too surprised that devices just don't bother to > notify MTU via MBIM_CID_IP_CONFIGURATION. > > (3) wMTU > > The MBIM extended functional descriptor is optional, but device > vendors do use it to indicate the MTU (mostly due to aforementioned > confusion around wMaxSegmentSize). Using the wMTU field wouldn't add > too much code or runtime overhead in kernel, so why don't we use it to > set up the initial MTU when available? We could handle it in > userspace, but I see the cdc_ncm driver is a better fit as it (like > other net drivers) already sets up mtu and leaving the wMTU case would > seem incomplete to me. > > While (1) and (2) are fixable, it's hard to convince device vendors to > update their firmware just for that as carrier certifications impose a > heavy cost of firmware changes. This sounds all reasonable to me. Thanks for taking the time to explain it in such detail. I did know that some vendors set wMaxSegmentSize too low, but had no idea that vendors were using the extended descriptor instead of MBIM_CID_IP_CONFIGURATION. If so, then yes, it does make sense for the driver to base the default MTU on this descriptor. >> wMTU access needs le16_to_cpu. > > Good catch. I will fix it in patch v2. Tip: I found this because my test script/makefile includes "C=1 CF=-D__CHECK_ENDIAN__" I find that very useful when dealing with USB on a little endian system, like most of us have. It's all too easy to miss a conversion otherwise. >> Could we move this final MTU correction from cdc_ncm_setup to >> cdc_ncm_bind_common to avoid bloating the device struct with another >> descriptor pointer we donæt really need to keep around? >> >> I know we look into descriptors in cdc_ncm_setup, because we have to, >> but ideally I would have loved to see cdc_ncm_setup dealing with *just* >> the NCM/MBIM specific control setup messages and cdc_ncm_bind_common >> dealing with all the functional descriptors. That seems most logic to >> me (but is of course only my personal opinion and nothing else - I do >> not know what the original cdc_ncm author intended) >> > > I understand the argument against the extra descriptor pointer. But I > think it's better to keep the mtu related code together so that one > can easily see how MTU is determined when trying to change or refactor > the code. I haven't looked into what cdc_ncm_setup was originally > intended for. If we'd like to avoid adding an extra pointer in > cdc_ncm_ctx, we could have cdc_ncm_bind passing a locally scoped > context to cdc_ncm_setup. No, the extra pointer doesn't matter much. Just keep it as it is. Bjørn -- 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
On Mon, Mar 17, 2014 at 2:27 PM, Bjørn Mork <bjorn@mork.no> wrote: > > This sounds all reasonable to me. Thanks for taking the time to explain > it in such detail. I did know that some vendors set wMaxSegmentSize too > low, but had no idea that vendors were using the extended descriptor > instead of MBIM_CID_IP_CONFIGURATION. > > If so, then yes, it does make sense for the driver to base the default > MTU on this descriptor. Hopefully, subsequent MBIM specifications would further clarify and simplify things a bit, but it's gonna be a slow process as we all know :-/ > >>> wMTU access needs le16_to_cpu. >> >> Good catch. I will fix it in patch v2. > > Tip: I found this because my test script/makefile includes > "C=1 CF=-D__CHECK_ENDIAN__" > > I find that very useful when dealing with USB on a little endian system, > like most of us have. It's all too easy to miss a conversion otherwise. > Thanks again for the review and tip. I've submitted patch v2 to address the le16_to_cpu conversion. >>> Could we move this final MTU correction from cdc_ncm_setup to >>> cdc_ncm_bind_common to avoid bloating the device struct with another >>> descriptor pointer we donæt really need to keep around? >>> >>> I know we look into descriptors in cdc_ncm_setup, because we have to, >>> but ideally I would have loved to see cdc_ncm_setup dealing with *just* >>> the NCM/MBIM specific control setup messages and cdc_ncm_bind_common >>> dealing with all the functional descriptors. That seems most logic to >>> me (but is of course only my personal opinion and nothing else - I do >>> not know what the original cdc_ncm author intended) >>> >> >> I understand the argument against the extra descriptor pointer. But I >> think it's better to keep the mtu related code together so that one >> can easily see how MTU is determined when trying to change or refactor >> the code. I haven't looked into what cdc_ncm_setup was originally >> intended for. If we'd like to avoid adding an extra pointer in >> cdc_ncm_ctx, we could have cdc_ncm_bind passing a locally scoped >> context to cdc_ncm_setup. > > No, the extra pointer doesn't matter much. Just keep it as it is. > > > Bjørn -- 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
From: Ben Chan <benchan@chromium.org> Date: Mon, 17 Mar 2014 17:46:27 -0700 > Thanks again for the review and tip. I've submitted patch v2 to > address the le16_to_cpu conversion. When you update a patch from a series, you should repost the entire patch set, rather than just the patch which changes. This avoids any and all ambiguity of which patches go with which others. -- 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
On Mon, Mar 17, 2014 at 6:41 PM, David Miller <davem@davemloft.net> wrote: > From: Ben Chan <benchan@chromium.org> > Date: Mon, 17 Mar 2014 17:46:27 -0700 > >> Thanks again for the review and tip. I've submitted patch v2 to >> address the le16_to_cpu conversion. > > When you update a patch from a series, you should repost the entire > patch set, rather than just the patch which changes. > > This avoids any and all ambiguity of which patches go with which > others. Thanks for pointing that out. I've submitted the whole patch set as v3 (bumped the revision to avoid confusion). -- 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
From: Ben Chan <benchan@chromium.org> Date: Mon, 17 Mar 2014 21:00:06 -0700 > On Mon, Mar 17, 2014 at 6:41 PM, David Miller <davem@davemloft.net> wrote: >> From: Ben Chan <benchan@chromium.org> >> Date: Mon, 17 Mar 2014 17:46:27 -0700 >> >>> Thanks again for the review and tip. I've submitted patch v2 to >>> address the le16_to_cpu conversion. >> >> When you update a patch from a series, you should repost the entire >> patch set, rather than just the patch which changes. >> >> This avoids any and all ambiguity of which patches go with which >> others. > > Thanks for pointing that out. I've submitted the whole patch set as v3 > (bumped the revision to avoid confusion). I hate to ask for a repost, but: 1) Please put Greg KH's ACK into patch #1, patchwork doesn't carry ACKs from one submission to the next, as it shouldn't. 2) Please give a leading "[PATCH 0/2] ..." posting explaining at a high level what this series is doing. Also, make it explicit what tree you want me to apply this too, and if the changes are relevant for -stable. -- 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 --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index dbff290..0b036ed 100644 --- a/drivers/net/usb/cdc_ncm.c +++ b/drivers/net/usb/cdc_ncm.c @@ -261,6 +261,13 @@ out: /* set MTU to max supported by the device if necessary */ if (dev->net->mtu > ctx->max_datagram_size - eth_hlen) dev->net->mtu = ctx->max_datagram_size - eth_hlen; + + /* do not exceed operater preferred MTU */ + if (ctx->mbim_extended_desc && + ctx->mbim_extended_desc->wMTU != 0 && + dev->net->mtu > ctx->mbim_extended_desc->wMTU) + dev->net->mtu = ctx->mbim_extended_desc->wMTU; + return 0; } @@ -399,6 +406,14 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_ ctx->mbim_desc = (const struct usb_cdc_mbim_desc *)buf; break; + case USB_CDC_MBIM_EXTENDED_TYPE: + if (buf[0] < sizeof(*(ctx->mbim_extended_desc))) + break; + + ctx->mbim_extended_desc = + (const struct usb_cdc_mbim_extended_desc *)buf; + break; + default: break; } diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h index c3fa807..bdf05fb 100644 --- a/include/linux/usb/cdc_ncm.h +++ b/include/linux/usb/cdc_ncm.h @@ -93,6 +93,7 @@ struct cdc_ncm_ctx { const struct usb_cdc_ncm_desc *func_desc; const struct usb_cdc_mbim_desc *mbim_desc; + const struct usb_cdc_mbim_extended_desc *mbim_extended_desc; const struct usb_cdc_ether_desc *ether_desc; struct usb_interface *control;
According to "Universal Serial Bus Communications Class Subclass Specification for Mobile Broadband Interface Model, Revision 1.0, Errata-1" published by USB-IF, the wMTU field of the MBIM extended functional descriptor indicates the operator preferred MTU for IP data streams. This patch modifies cdc_ncm_setup to ensure that the MTU value set on the usbnet device does not exceed the operator preferred MTU indicated by wMTU if the MBIM device exposes a MBIM extended functional descriptor. Signed-off-by: Ben Chan <benchan@chromium.org> --- drivers/net/usb/cdc_ncm.c | 15 +++++++++++++++ include/linux/usb/cdc_ncm.h | 1 + 2 files changed, 16 insertions(+)