Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758548AbXHUHJp (ORCPT ); Tue, 21 Aug 2007 03:09:45 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752599AbXHUHJh (ORCPT ); Tue, 21 Aug 2007 03:09:37 -0400 Received: from vervifontaine.sonytel.be ([80.88.33.193]:49565 "EHLO vervifontaine.sonycom.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751115AbXHUHJg (ORCPT ); Tue, 21 Aug 2007 03:09:36 -0400 Date: Tue, 21 Aug 2007 09:09:33 +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-1808098190-1187680108=:11703" Content-ID: Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6825 Lines: 159 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-1808098190-1187680108=:11703 Content-Type: TEXT/PLAIN; CHARSET=UTF-8 Content-Transfer-Encoding: 8BIT Content-ID: On Tue, 21 Aug 2007, Satyam Sharma wrote: > On Mon, 20 Aug 2007, Geert Uytterhoeven wrote: > > On Sat, 18 Aug 2007, Satyam Sharma wrote: > > > On Sat, 18 Aug 2007, Jan Engelhardt wrote: > > > > On Aug 18 2007 20:07, Satyam Sharma wrote: > > > > >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote: > > > > >> On Thu, 16 Aug 2007, NeilBrown wrote: > > > > >> [...] > > > > >> > dev_dbg(&dev->sbd.core, > > > > >> > "%s:%u: bio %u: %u segs %u sectors from %lu\n", > ^^^ > > > > > >> > - __func__, __LINE__, i, bio_segments(bio), > > > > >> > - bio_sectors(bio), sector); > > > > >> > - bio_for_each_segment(bvec, bio, j) { > > > > >> > + __func__, __LINE__, i, bio_segments(iter.bio), > > > > >> > + bio_sectors(iter.bio), > > > > >> > + (unsigned long)iter.bio->bi_sector); > ^^^^^^^^^^^^^^^ ^^^^^^^^^ > > > > > >> Superfluous cast: PS3 is 64-bit only, and casts are evil. > > > > > > > > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully > > > > so since sector_t may just change its shape underneath. > > > > > > Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way. > > > I was mentioning why the cast itself should be (unsigned long long) otoh. > > > > On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending > > on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's > > no compiler warning to be silenced by adding the cast. > > 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 ??long unsigned int??, but argument 2 has > type ??sector_t?? You will get a warning, so you will know. > 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 ] > * 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 ??long long unsigned int??, but argument > 2 has type ??sector_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. > Signed-off-by: Satyam Sharma NAK. Do not add casts that are not needed. Casts are evil. > diff -ruNp a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c > --- a/drivers/block/ps3disk.c 2007-08-21 06:52:34.000000000 +0530 > +++ b/drivers/block/ps3disk.c 2007-08-21 06:56:07.000000000 +0530 > @@ -100,10 +100,10 @@ static void ps3disk_scatter_gather(struc > rq_for_each_segment(bvec, req, iter) { > unsigned long flags; > dev_dbg(&dev->sbd.core, > - "%s:%u: bio %u: %u segs %u sectors from %lu\n", > + "%s:%u: bio %u: %u segs %u sectors from %llu\n", > __func__, __LINE__, i, bio_segments(iter.bio), > bio_sectors(iter.bio), > - (unsigned long)iter.bio->bi_sector); > + (unsigned long long)iter.bio->bi_sector); > > size = bvec->bv_len; > buf = bvec_kmap_irq(bvec, flags); > @@ -142,8 +142,9 @@ static int ps3disk_submit_request_sg(str > > start_sector = req->sector * priv->blocking_factor; > sectors = req->nr_sectors * priv->blocking_factor; > - dev_dbg(&dev->sbd.core, "%s:%u: %s %lu sectors starting at %lu\n", > - __func__, __LINE__, op, sectors, start_sector); > + dev_dbg(&dev->sbd.core, "%s:%u: %s %llu sectors starting at %llu\n", > + __func__, __LINE__, op, (unsigned long long)sectors, > + (unsigned long long)start_sector); > > if (write) { > ps3disk_scatter_gather(dev, req, 1); 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-1808098190-1187680108=:11703-- - 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/