Oliver Neukum <oneukum@xxxxxxx> writes:
>> 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.
Yes, I agree. Will change this. Let's try to go for b) then. 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 (max < min)
max = min;
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
