[U-Boot] RFC: getramsize() prototype and volatile qualifier

[U-Boot] RFC: getramsize() prototype and volatile qualifier

[Albert ARIBAUD]

Hi all,

Call it a detail, but I see that get_ram_size() calls sometime qualify 
their argument as volatile and sometimes not, and this makes checkpatch 
complain that volatiles are Bad(tm), which I would like to get fixed.

The prototype for get_ram_size() in is

	long	get_ram_size  (volatile long *, long);

While I understand that the way get_ram_size() works, it needs to 
perform volatile *accesses* to addresses computed from its arguments, I 
don't see why it requires one of the arguments themselves to be volatile.

Am I missing something here, particularly about some toolchain requiring 
the argument to be volatile? I see no reason it should, but better safe 
than sorry.

Amicalement,
-- 
Albert.

[U-Boot] RFC: getramsize() prototype and volatile qualifier

[Mike Frysinger]

On Thu, Apr 21, 2011 at 3:09 AM, Albert ARIBAUD wrote:
> Call it a detail, but I see that get_ram_size() calls sometime qualify
> their argument as volatile and sometimes not, and this makes checkpatch
> complain that volatiles are Bad(tm), which I would like to get fixed.
>
> The prototype for get_ram_size() in is
>
> ? ? ? ?long ? ?get_ram_size ?(volatile long *, long);
>
> While I understand that the way get_ram_size() works, it needs to
> perform volatile *accesses* to addresses computed from its arguments, I
> don't see why it requires one of the arguments themselves to be volatile.
>
> Am I missing something here, particularly about some toolchain requiring
> the argument to be volatile? I see no reason it should, but better safe
> than sorry.

the argument isnt volatile, the memory it points to is.  since
get_ram_size() itself doesnt dereference the argument (only goes
through a local "addr"), the volatile marking in the prototype could
be dropped, but personally i dont see the point.  the prototype doesnt
require callers to add volatile markings to their own code, and i
could see someone using "base" in the future after the volatile being
dropped and people not noticing right away.  the current code is safe
and causes no problems by itself, so i see no value in changing it.

this sounds like useless "gotta be checkpatch clean" work by people
blindly following its output.
-mike

[U-Boot] RFC: getramsize() prototype and volatile qualifier

[Albert ARIBAUD]

Le 21/04/2011 19:02, Mike Frysinger a ?crit :
> On Thu, Apr 21, 2011 at 3:09 AM, Albert ARIBAUD wrote:
>> Call it a detail, but I see that get_ram_size() calls sometime qualify
>> their argument as volatile and sometimes not, and this makes checkpatch
>> complain that volatiles are Bad(tm), which I would like to get fixed.
>>
>> The prototype for get_ram_size() in is
>>
>>         long    get_ram_size  (volatile long *, long);
>>
>> While I understand that the way get_ram_size() works, it needs to
>> perform volatile *accesses* to addresses computed from its arguments, I
>> don't see why it requires one of the arguments themselves to be volatile.
>>
>> Am I missing something here, particularly about some toolchain requiring
>> the argument to be volatile? I see no reason it should, but better safe
>> than sorry.
>
> the argument isnt volatile, the memory it points to is.  since
> get_ram_size() itself doesnt dereference the argument (only goes
> through a local "addr"), the volatile marking in the prototype could
> be dropped, but personally i dont see the point.  the prototype doesnt
> require callers to add volatile markings to their own code, and i
> could see someone using "base" in the future after the volatile being
> dropped and people not noticing right away.  the current code is safe
> and causes no problems by itself, so i see no value in changing it.
>
> this sounds like useless "gotta be checkpatch clean" work by people
> blindly following its output.
> -mike

Just because checkpatch complains about something does not mean that 
particular complaint is without merit. :)

Yes, the code works as it is; but it'll work just as well if volatile is 
removed from the prototype and calls, and that'll make the code simpler 
and more homogeneous overall, plus it'll give checkpatch one less cause 
for complaint. We gain something there, if not a functional plus, an 
overal quality plus.

As for people fiddling with get_ram_size() *body* and removing the 
volatile 'addr' and use the non-volatile 'base' directly, why would 
they? They will plainly see that 'addr' is volatile and 'base' is not 
(all the more because removing the volatile' attribute from 'base' will 
require casting it to 'base+int' where it belongs), and the very role of 
the function makes it clear to them that the volatile attribute is 
required for 'addr', so they'll keep 'addr'.

Plus, get_ram_size() is pretty stable -- actually, it has changed only 
once in 2006 since its creation in 2004.

Amicalement,
-- 
Albert.

[U-Boot] RFC: getramsize() prototype and volatile qualifier

[Mike Frysinger]

On Thu, Apr 21, 2011 at 1:50 PM, Albert ARIBAUD wrote:
> Le 21/04/2011 19:02, Mike Frysinger a ?crit :
>> On Thu, Apr 21, 2011 at 3:09 AM, Albert ARIBAUD wrote:
>>> Call it a detail, but I see that get_ram_size() calls sometime qualify
>>> their argument as volatile and sometimes not, and this makes checkpatch
>>> complain that volatiles are Bad(tm), which I would like to get fixed.
>>>
>>> The prototype for get_ram_size() in is
>>>
>>> ? ? ? ?long ? ?get_ram_size ?(volatile long *, long);
>>>
>>> While I understand that the way get_ram_size() works, it needs to
>>> perform volatile *accesses* to addresses computed from its arguments, I
>>> don't see why it requires one of the arguments themselves to be volatile.
>>>
>>> Am I missing something here, particularly about some toolchain requiring
>>> the argument to be volatile? I see no reason it should, but better safe
>>> than sorry.
>>
>> the argument isnt volatile, the memory it points to is. ?since
>> get_ram_size() itself doesnt dereference the argument (only goes
>> through a local "addr"), the volatile marking in the prototype could
>> be dropped, but personally i dont see the point. ?the prototype doesnt
>> require callers to add volatile markings to their own code, and i
>> could see someone using "base" in the future after the volatile being
>> dropped and people not noticing right away. ?the current code is safe
>> and causes no problems by itself, so i see no value in changing it.
>>
>> this sounds like useless "gotta be checkpatch clean" work by people
>> blindly following its output.
>
> Just because checkpatch complains about something does not mean that
> particular complaint is without merit. :)

like ive said many times, "checkpatch clean" is not a hard rule.
review the output to see when it makes sense.  the general "volatile
is bad" check is there because people often screw it up.  this is not
one of those cases ... we absolutely want volatile accesses.

> Yes, the code works as it is; but it'll work just as well if volatile is
> removed from the prototype and calls

uhh, there is no "volatile in calls".  there is no requirement that
the call sites also use volatile markings.

>, and that'll make the code simpler and more homogeneous overall

i think you missed a fairly important qualifier.  this is your
opinion, and one i dont share in this particular instance.

> plus it'll give checkpatch one less cause for complaint. We gain
> something there, if not a functional plus, an overal quality plus.

there is absolutely no quality difference

> As for people fiddling with get_ram_size() *body* and removing the volatile
> 'addr' and use the non-volatile 'base' directly, why would they?

i have no idea.  i leave the door open to things that i dont happen to
think of rather than assuming absolutes.

> They will plainly see that 'addr' is volatile and 'base' is not

this sort of blind assumption is dangerous.  things that are plain to
you doesnt mean it's plain to everyone, and people make mistakes all
the time.  keeping the volatile markings on base protects from that
sort of screw up.

> (all the more because removing the volatile' attribute from 'base' will
> require casting it to 'base+int' where it belongs)

i dont know what you're talking about here ... there is no need for
casting regardless of the volatile markings of base.  casting is need
to ignore warnings when casting "down" from volatile to non-volatile,
not the other way around.  which is not the case here.
-mike

[U-Boot] RFC: getramsize() prototype and volatile qualifier

[Albert ARIBAUD]

Le 22/04/2011 00:28, Mike Frysinger a ?crit :

> On Thu, Apr 21, 2011 at 1:50 PM, Albert ARIBAUD wrote:
>> Le 21/04/2011 19:02, Mike Frysinger a ?crit :
>>> On Thu, Apr 21, 2011 at 3:09 AM, Albert ARIBAUD wrote:
>>>> Call it a detail, but I see that get_ram_size() calls sometime qualify
>>>> their argument as volatile and sometimes not, and this makes checkpatch
>>>> complain that volatiles are Bad(tm), which I would like to get fixed.
>>>>
>>>> The prototype for get_ram_size() in is
>>>>
>>>>         long    get_ram_size  (volatile long *, long);
>>>>
>>>> While I understand that the way get_ram_size() works, it needs to
>>>> perform volatile *accesses* to addresses computed from its arguments, I
>>>> don't see why it requires one of the arguments themselves to be volatile.
>>>>
>>>> Am I missing something here, particularly about some toolchain requiring
>>>> the argument to be volatile? I see no reason it should, but better safe
>>>> than sorry.
>>>
>>> the argument isnt volatile, the memory it points to is.  since
>>> get_ram_size() itself doesnt dereference the argument (only goes
>>> through a local "addr"), the volatile marking in the prototype could
>>> be dropped, but personally i dont see the point.  the prototype doesnt
>>> require callers to add volatile markings to their own code, and i
>>> could see someone using "base" in the future after the volatile being
>>> dropped and people not noticing right away.  the current code is safe
>>> and causes no problems by itself, so i see no value in changing it.
>>>
>>> this sounds like useless "gotta be checkpatch clean" work by people
>>> blindly following its output.
>>
>> Just because checkpatch complains about something does not mean that
>> particular complaint is without merit. :)
>
> like ive said many times, "checkpatch clean" is not a hard rule.
> review the output to see when it makes sense.  the general "volatile
> is bad" check is there because people often screw it up.  this is not
> one of those cases ... we absolutely want volatile accesses.

Volatile accesses, yes, we want them, I said so. But that does not mean 
we need volatile *paramters* to the function or *arguments* to the calls.

>> Yes, the code works as it is; but it'll work just as well if volatile is
>> removed from the prototype and calls
>
> uhh, there is no "volatile in calls".  there is no requirement that
> the call sites also use volatile markings.

There *are* volatiles in the calls; just check the source code.

>> , and that'll make the code simpler and more homogeneous overall
>
> i think you missed a fairly important qualifier.  this is your
> opinion, and one i dont share in this particular instance.

Maybe you missed the fact that there are calls to get_ram_size() that 
don't have the volatile qualifier to 'base' and there are other who have 
it -- that's heterogeneity (I suspect it stems from various strictnesses 
in toolchains) and removing all volatiles objectively does remove that 
heterogeneity.

>> plus it'll give checkpatch one less cause for complaint. We gain
>> something there, if not a functional plus, an overal quality plus.
>
> there is absolutely no quality difference

Less error / warning messages from a tool we use counts as better 
quality for me.

>> As for people fiddling with get_ram_size() *body* and removing the volatile
>> 'addr' and use the non-volatile 'base' directly, why would they?
>
> i have no idea.  i leave the door open to things that i dont happen to
> think of rather than assuming absolutes.

I don't assume absolutes here I think. My assumtion is that 'people' 
will not blindly remove a volatile qualifier in a memory size test 
function which obviously requires its memory accesses to not be 
optimized in any way.

>> They will plainly see that 'addr' is volatile and 'base' is not
>
> this sort of blind assumption is dangerous.  things that are plain to
> you doesnt mean it's plain to everyone, and people make mistakes all
> the time.  keeping the volatile markings on base protects from that
> sort of screw up.

If things are not plain, then let's make them plain where they need to 
be. We do agree that the qualifier in the prototype and calls is useless 
from a functional viewpoint; we do agree that the volatile *access* in 
the function body is crucial; if you think people who want to modify the 
body of get_ram_size() need an explicit warning about keeping the 
accesses volatile then that warning should be given right near the 
accesses, for instance in the form of a Big Fat Comment (tm) right in 
the function's code, not elsewhere in calls.

>> (all the more because removing the volatile' attribute from 'base' will
>> require casting it to 'base+int' where it belongs)
>
> i dont know what you're talking about here ... there is no need for
> casting regardless of the volatile markings of base.  casting is need
> to ignore warnings when casting "down" from volatile to non-volatile,
> not the other way around.  which is not the case here.
> -mike

Correct: the cast to volatile is not needed. Still, removing the 'base' 
variable will remove a 'volatile' qualifier. However, I still think 
people who want to modify get_ram_size() know about volatiles and how 
the function works and won't make the mistake of removing the qualifier. 
And if they do, the change will be reviewed and NAKed anyway.

Amicalement,
-- 
Albert.