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: [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
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
