On Tue, 2014-05-13 at 10:49 +0200, Bjørn Mork wrote: > Oliver Neukum <oneukum@xxxxxxx> writes: > > > On Sat, 2014-05-10 at 17:41 +0200, Bjørn Mork wrote: > >> + /* clamp new_rx to sane values */ > >> + min = min_t(u32, USB_CDC_NCM_NTB_MIN_IN_SIZE, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > >> + max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > > > > Are you sure this makes sense? min_t both times? > > Yes, I am sure. At least it made sense when I wrote it. I am more in > doubt now. I actually suspected a copy n' paste error; thence the formulation. > I guess you don't question the max calculation, but just so everyone > else gets the idea: dwNtbInMaxSize is the buffer size suggested by the > device. Some devices just specify an insanely large value (132kB has > been observed). So we need to cap that to CDC_NCM_NTB_MAX_SIZE_RX, which > is the absolutely largest buffer size we are prepared to support. Good > USB_CDC_NCM_NTB_MIN_IN_SIZE is the minimum acceptable buffer size > according to the spec. dwNtbInMaxSize is not allowed to be smaller than > this. So if we assume that no device violates the spec, then the above > should simple be > > min = USB_CDC_NCM_NTB_MIN_IN_SIZE; > max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > > which is the result for all spec conforming devices. > > The reason I put that min_t() there instead was an attempt to deal with > the (not unlikely) event that some buggy device set dwNtbInMaxSize lower > than this required minimum value. We then have the choices: > > a) fail to support the buggy device > b) attempt to set a larger buffer size than the device supports > c) accept the lower size My preference would be b) > a) > c) It seems to me that would should respect the spec and if the spec sets a lower limit then we don't go lower. > So I chose c) in an attempt to be as gentle as possible. But I am open > to go for a) instead if you think that is better. After all > USB_CDC_NCM_NTB_MIN_IN_SIZE is as low as 2048, so it doesn't fit much > more than the headers and a single full size ethernet frame. And I see > now that we fail to do further sanity checking after this. What if > dwNtbInMaxSize is 0? Or smaller than the necessary headers? Exactly. Some fool may simply overlook setting it at all. > Should I rewrite the above to do a) instead? I.e. > > min = USB_CDC_NCM_NTB_MIN_IN_SIZE; > max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_RX, le32_to_cpu(ctx->ncm_parm.dwNtbInMaxSize)); > if (min > max) > fail; > > I don't think b) is a good idea. It might work, but it might also fail > in surprising ways making it hard to debug. Users may prefer working devices to clean failures, but I primarily care about conforming to spec. We just shouldn't do such violations in a general case. Regards Oliver -- 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
