On Mon, Mar 17, 2014 at 2:27 PM, Bjørn Mork <bjorn@xxxxxxx> 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 linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
