cam / ata timeout limited to 2147 due to overflow bug?

[Alexander Motin]

Eygene Ryabinkin wrote:
> Fri, Aug 05, 2011 at 10:59:43AM +0100, Steven Hartland wrote:
>> I've tried the patch and it a few cut and paste errors, which I've fixed,
> 
> Thanks for spotting that!
> 
>> and confirmed it works as expected, so thanks for that :)
>>
>> There's also a load more drivers with the same issue so I've gone through
>> and fixed all the occurances I can find. Here's the updated patch:-
>> http://blog.multiplay.co.uk/dropzone/freebsd/ccb_timeout.patch
> 
> I had found a couple of missed drivers, fixed overlong lines and fixed
> the missing 10 in the sys/dev/hptrr/hptrr_os_bsd.c.  Also changed ciss
> to have u_int32_t timeouts instead of int ones: this should not harm
> anything, because all passed timeouts are explicit numbers that are
> not larger than 100000.  And I had also renamed
> CAM_HDR_TIMEOUT_TO_TICKS to the base CAM_TIMEOUT_TO_TICKS, because it
> seems that every CAM timeout is 32-bit long.  The new patch lives at
>   http://codelabs.ru/fbsd/patches/cam/CAM-properly-convert-timeout-to-ticks.diff
> 
> But there are some cases where the argument to the
> CAM_TIMEOUT_TO_TICKS is int and not u_int32_t.  It should be mostly
> harmless for now, since the values do not exceed 2^32, but my current
> feeling about timeouts that are counted in milliseconds that there
> should be an in-kernel type for this stuff.  Seems like 32-bit wide
> unsigned value is good for it: maximal value is around 46 days that
> should be fine for the millisecond-precision timeout.
> 
> Through my grep session for the kernel sources I had seen other
> (t * hz / 1000) constructs, so I feel that the fix should be
> extended to cover these cases as well.
> 
> I am interested in the other's opinions on this.

First of all, not so many people really need millisecond precision for
timeouts, combined with large timeout range. I would prefer to see
seconds in CAM. Same time 64bit mul/div pair for every call may worth
something, especially for low-end 32bit archs. We can't change existing
CAM API, but global mechanical replace through the kernel is not a good
idea IMHO.

Personally, I would not touch argument types in ciss -- it is almost
null change, but may break some patches applicability. While using
uint32_t in CAM structures could be a benefit from compatibility point,
it is not important from this point in function arguments.

-- 
Alexander Motin