Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756918AbXHUJwx (ORCPT ); Tue, 21 Aug 2007 05:52:53 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754187AbXHUJwp (ORCPT ); Tue, 21 Aug 2007 05:52:45 -0400 Received: from vervifontaine.sonytel.be ([80.88.33.193]:52245 "EHLO vervifontaine.sonycom.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754159AbXHUJwo (ORCPT ); Tue, 21 Aug 2007 05:52:44 -0400 Date: Tue, 21 Aug 2007 11:52:42 +0200 (CEST) From: Geert Uytterhoeven To: Satyam Sharma cc: NeilBrown , Jan Engelhardt , Jens Axboe , Linux Kernel Development , Tejun Heo , Cell Broadband Engine OSS Development Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio In-Reply-To: Message-ID: References: <20070816211551.11839.patches@notabene> <1070816112100.12182@suse.de> MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="-584337861-2078517362-1187689962=:6982" Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5351 Lines: 127 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. ---584337861-2078517362-1187689962=:6982 Content-Type: TEXT/PLAIN; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Tue, 21 Aug 2007, Satyam Sharma wrote: > On Tue, 21 Aug 2007, Geert Uytterhoeven wrote: > > On Tue, 21 Aug 2007, Satyam Sharma wrote: > > > > > > This is turning rather funny :-) > > > > > > * Why the printk() conversion specifier must be "%llu"? > > > > > > When reusing parts of this code (such as this debug message) for another > > > 32-bit driver (we certainly seem to care about this, as per your reply), > > > the largest size of sector_t can be "unsigned long long", thereby causing > > > truncated output, and the following warning: > > > > > > warning: format ???lu???expects type ???ong unsigned int??? but argument 2 has > > > type ???ector_t? > > > > You will get a warning, so you will know. > > Ah. So you'd much rather prefer that code in drivers/ make assumptions: > sizeof(long) == sizeof(long long), and, sizeof(long) == sizeof(sector_t), > hmmm? If it's code for a PS3-specific driver that cannot work in 32-bit mode, yes. > > > Therefore, let us not depend on the fact that this driver is being used > > > only for 64-bit platforms as of now (especially since this is in drivers/ > > > and not in arch/) and rather make the printk() specifier as "%llu". > > > > > > [ Sadly, I had to repeat most of my previous mail, which Jan Engelhardt > > > appears to have snipped. ] > > > > [ and you dropped the cell mailing list from the CC list ] > > I don't enjoy getting "this is a subscriber-only list" notifications, > thank you. IC. > > > * Why we require an explicit (unsigned long long) cast? > > > > > > Having made the above change (and say we don't have an explicit cast > > > there), that line now becomes: > > > > > > printk("... %llu\n", ..., iter.bio->bi_sector); > > > > > > Now if we build this code with CONFIG_LBD=n, sector_t becomes just an > > > "unsigned long" (whereas printk specifier is the larger "%llu") thereby > > > causing: > > > > > > warning: format ???llu???expects type ???ong long unsigned int??? but argument > > > 2 has type ???ector_t? > > > > > > Therefore, let us shut this up with an explicit (unsigned long long) cast, > > > as is done in most other existing code in the kernel where we want to get > > > bi_sector printed out, which would correctly convert the value to the > > > larger integer type (even negative ones, due to sign-extension) and print > > > it out. > > > > > > > On the other hand, adding a cast may hide bugs: > > > > - cast to unsigned long long: When sector_t is changed to an even larger > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > type, it will be silently truncated as well. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > > IMHO, this is not a valid enough reason, given the above (BTW if/when that > > > happens, we'll have to update the printk conversion specifer as well). > > > > > > Personally, I'd say code that assumes "sizeof(unsigned long long) == > > > sizeof(unsigned long)", and also that assumes "sizeof(unsigned long) == > > > sizeof(sector_t)" -- both assumptions _are_ made by the above code -- > > > is not good taste, even if both may be true for PS3 platform. So unless > > > we decide "nobody except that platform would ever build this driver > > > anyway", I'd suggest to make the printk specifier as "%llu" and use an > > > explicit (unsigned long long) cast. Therefore (on top of this series): > > > > Adding such a cast makes it impossible for the compiler to detect (and warn us) > > if something has changed. > > Change as in increase in size, right? Did you even read the mail? The only Yes. Yes. > argument you had given against the cast to (unsigned long long) was what > has been underlined above, which was bogus, considering we would have to > change the "%lu" specifier in the printk() also, when that happens > (otherwise suffer silent truncation). Not silent truncation. Without a cast, the compiler will warn. > But looks like you /are/ deciding "nobody except PS3 platform would ever > build this driver anyway" ... so okay, the current code, will all it's > non-portable assumptions, is okay. OK, thx. The driver uses the PS3-specific hypervisor interface, which is 64-bit. With kind regards, Geert Uytterhoeven Software Architect Sony Network and Software Technology Center Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone: +32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: Geert.Uytterhoeven@sonycom.com Internet: http://www.sony-europe.com/ Sony Network and Software Technology Center Europe A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619 ---584337861-2078517362-1187689962=:6982-- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/