From owner-freebsd-hackers@FreeBSD.ORG Fri Aug 12 09:56:13 2011 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E6A031065673; Fri, 12 Aug 2011 09:56:12 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: from mail-fx0-f54.google.com (mail-fx0-f54.google.com [209.85.161.54]) by mx1.freebsd.org (Postfix) with ESMTP id 1CB768FC14; Fri, 12 Aug 2011 09:56:11 +0000 (UTC) Received: by fxe4 with SMTP id 4so2913477fxe.13 for ; Fri, 12 Aug 2011 02:56:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=sender:message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:x-enigmail-version:content-type :content-transfer-encoding; bh=b5P0SVPmWIbkE8Tj2XVtemZweOzAz5RygQF4jJCrv1M=; b=UBaETVo1S7zQPtAzkpKTxN5kuIarnBTVD1QyIlzwbOf35jeBRy+Kx+y5cdsOwcNRBm PMk+Yh9BAfb5Q+9MDeQa31fdM3UFFwUMGWQAH9gK6MmODZVTs5xAPAZbmApWOgVllKaF ZoNizVdMFiC3eHWTqccG6/BRFdVRPLPjhLSGE= Received: by 10.223.21.207 with SMTP id k15mr1028973fab.60.1313142971211; Fri, 12 Aug 2011 02:56:11 -0700 (PDT) Received: from mavbook2.mavhome.dp.ua (pc.mavhome.dp.ua [212.86.226.226]) by mx.google.com with ESMTPS id r12sm1070208fam.38.2011.08.12.02.56.08 (version=SSLv3 cipher=OTHER); Fri, 12 Aug 2011 02:56:09 -0700 (PDT) Sender: Alexander Motin Message-ID: <4E44F8AF.4010307@FreeBSD.org> Date: Fri, 12 Aug 2011 12:55:59 +0300 From: Alexander Motin User-Agent: Thunderbird 2.0.0.23 (X11/20091212) MIME-Version: 1.0 To: Eygene Ryabinkin References: <4CAD348034DD463E80C89DD5A0BDD71B@multiplay.co.uk> In-Reply-To: X-Enigmail-Version: 0.96.0 Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Cc: freebsd-hackers@freebsd.org, Steven Hartland Subject: Re: cam / ata timeout limited to 2147 due to overflow bug? X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Aug 2011 09:56:13 -0000 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