[U-Boot] [PATCH v3 01/10] net: Remove volatile from net API

[Joe Hershberger]

Hi Wolfgang,

On Mon, May 21, 2012 at 2:05 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Joe Hershberger,
>
> In message <1337108353-28086-2-git-send-email-joe.hershberger at ni.com> you wrote:
>> Signed-off-by: Joe Hershberger <joe.hershberger at ni.com>
>> Cc: Joe Hershberger <joe.hershberger at gmail.com>
>> ---
>> Changes for v2:
>>   - Remove volatile from eth driver API
>>   - Not using global NetRxPacket (since not casting away volatile)
>> Changes for v3:
>>
>>  include/net.h |   25 +++++++++++++------------
>>  net/bootp.c   |    4 ++--
>>  net/eth.c     |   12 +++++-------
>>  net/net.c     |   34 +++++++++++++++++-----------------
>>  net/rarp.c    |    2 +-
>>  net/tftp.c    |    6 +++---
>>  6 files changed, 41 insertions(+), 42 deletions(-)
>
> While getting rid of volatile is a good thing in general, I am worried
> about the test scope of this patch.

I was too.  That's why I asked for guidance:

On Sun, Feb 26, 2012 at 7:51 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Joe,
>
> In message <CANr=Z=bA9gPv=OALJ2OR_drNM7-KuZn9H4EL7nEsdSMtMyn1Kw at mail.gmail.com> you wrote:
>>
>> >On Fri, Feb 3, 2012 at 5:44 AM, Mike Frysinger <vapier at gentoo.org> wrote:
>> >> in general, i like this.  my only concern would be the drivers that might
>> >> break due to incorrect cache management (which the volatile markers happen to
>> >> work around for them).
>> >>
>> >> having the API accept a volatile but then casting it away puts us in a worse
>> >> place i think.  on one hand, our API is saying "we treat it as volatile" when
>> >> we really don't, so any drivers that call it with a volatile don't get build
>> >> warnings.  i'd just drop it from all of the net API so that the drivers which
>> >> do call with a volatile pointer get a build warning -- now the driver
>> >> maintainer knows they have to at least look at something.
>> >
>> > I'm fine with that... it was my first approach, in fact.  However, I
>> > was concerned that if I submitted a patch that added warnings to every
>> > Ethernet-enabled board, that would be frowned upon.  I think it is the
>> > correct thing to do, but don't have a good feeling for how open people
>> > are to changing an interface like that.  I also did not want to
>> > attempt to revise every net driver in this series.
>> >
>> > Wolfgang,
>> >
>> > Would you approve of changing the net driver API to not use volatile
>> > buffer pointers and have the driver file warn until the driver
>> > maintainer addresses the warnings?
>
> I also think this is the right thing to do, and such cleanups
> sometimes have to spitout warnings for the not-yet-fixed boards.
> That's unavoidable.  Go for it...
>
>> Can I get guidance on this from you?  I'm about done preparing the
>> changes to the link-local patch-set and would like to know how you
>> prefer that I proceed.
>
> ACK for above.

This is why I went forward with warnings in the mainline for this
interface change.

> This commit is causing tons of compiler warnings because the
> respective changes have only be done in the header file and the net/
> code, but they are missing in _all_ network drivers.  I wonder which
> compile and run time testing has been done with this?  I mean, you
> decided to pull this into mainline - has it been tested at all ?!?

Yes... I tested this on MPC8313 (powerpc) and AM1808 (arm).

> When are you planning to provide fixes for all these warnings?  As is,
> the whole build is spoiled.  Or should we revert these patches?

I plan to submit patches for the boards I am able to test, but for
reason that Mike suggested, I'm not sure it is wise to fix the warning
unless it can be tested on hardware.

Apologies for not making this clear,
-Joe