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 <[EMAIL PROTECTED]>
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: [EMAIL PROTECTED]
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
