[Cbe-oss-dev] [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Geert Uytterhoeven]

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 at 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