diff mbox

[U-Boot,01/28] net: Remove volatile from all of net except the eth driver interface

Message ID 1327020811-1538-2-git-send-email-joe.hershberger@ni.com
State Superseded
Delegated to: Joe Hershberger
Headers show

Commit Message

Joe Hershberger Jan. 20, 2012, 12:53 a.m. UTC 
The mv_eth driver should not redefine the net function definition

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
Cc: Joe Hershberger <joe.hershberger@gmail.com>
Cc: Wolfgang Denk <wd@denx.de>
---
 board/Marvell/db64360/mv_eth.c |    2 --
 board/Marvell/db64460/mv_eth.c |    2 --
 board/esd/cpci750/mv_eth.c     |    2 --
 board/prodrive/p3mx/mv_eth.c   |    2 --
 include/net.h                  |   16 ++++++++--------
 net/bootp.c                    |    4 ++--
 net/net.c                      |   40 ++++++++++++++++++++--------------------
 net/rarp.c                     |    2 +-
 net/tftp.c                     |    6 +++---
 9 files changed, 34 insertions(+), 42 deletions(-)

Comments

Simon Glass Jan. 20, 2012, 4:22 p.m. UTC | #1 
Hi Joe,

On Thu, Jan 19, 2012 at 4:53 PM, Joe Hershberger <joe.hershberger@ni.com> wrote:
> The mv_eth driver should not redefine the net function definition
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Joe Hershberger <joe.hershberger@gmail.com>
> Cc: Wolfgang Denk <wd@denx.de>
> ---
>  board/Marvell/db64360/mv_eth.c |    2 --
>  board/Marvell/db64460/mv_eth.c |    2 --
>  board/esd/cpci750/mv_eth.c     |    2 --
>  board/prodrive/p3mx/mv_eth.c   |    2 --
>  include/net.h                  |   16 ++++++++--------
>  net/bootp.c                    |    4 ++--
>  net/net.c                      |   40 ++++++++++++++++++++--------------------
>  net/rarp.c                     |    2 +-
>  net/tftp.c                     |    6 +++---
>  9 files changed, 34 insertions(+), 42 deletions(-)
>
> diff --git a/board/Marvell/db64360/mv_eth.c b/board/Marvell/db64360/mv_eth.c
> index 6340585..550610e 100644
> --- a/board/Marvell/db64360/mv_eth.c
> +++ b/board/Marvell/db64360/mv_eth.c
> @@ -95,8 +95,6 @@ int mv64360_eth_xmit (struct eth_device *, volatile void *packet, int length);
>  #ifndef  UPDATE_STATS_BY_SOFTWARE
>  static void mv64360_eth_print_stat (struct eth_device *dev);
>  #endif
> -/* Processes a received packet */
> -extern void NetReceive (volatile uchar *, int);
>
>  extern unsigned int INTERNAL_REG_BASE_ADDR;
>
> diff --git a/board/Marvell/db64460/mv_eth.c b/board/Marvell/db64460/mv_eth.c
> index 4aefbaf..c447229 100644
> --- a/board/Marvell/db64460/mv_eth.c
> +++ b/board/Marvell/db64460/mv_eth.c
> @@ -95,8 +95,6 @@ int mv64460_eth_xmit (struct eth_device *, volatile void *packet, int length);
>  #ifndef  UPDATE_STATS_BY_SOFTWARE
>  static void mv64460_eth_print_stat (struct eth_device *dev);
>  #endif
> -/* Processes a received packet */
> -extern void NetReceive (volatile uchar *, int);
>
>  extern unsigned int INTERNAL_REG_BASE_ADDR;
>
> diff --git a/board/esd/cpci750/mv_eth.c b/board/esd/cpci750/mv_eth.c
> index 001c1ad..db2324b 100644
> --- a/board/esd/cpci750/mv_eth.c
> +++ b/board/esd/cpci750/mv_eth.c
> @@ -95,8 +95,6 @@ int mv64360_eth_xmit (struct eth_device *, volatile void *packet, int length);
>  #ifndef  UPDATE_STATS_BY_SOFTWARE
>  static void mv64360_eth_print_stat (struct eth_device *dev);
>  #endif
> -/* Processes a received packet */
> -extern void NetReceive (volatile uchar *, int);
>
>  extern unsigned int INTERNAL_REG_BASE_ADDR;
>
> diff --git a/board/prodrive/p3mx/mv_eth.c b/board/prodrive/p3mx/mv_eth.c
> index 15b3bfc..f26ded8 100644
> --- a/board/prodrive/p3mx/mv_eth.c
> +++ b/board/prodrive/p3mx/mv_eth.c
> @@ -109,8 +109,6 @@ int phy_setup_aneg (char *devname, unsigned char addr);
>  #ifndef         UPDATE_STATS_BY_SOFTWARE
>  static void mv64460_eth_print_stat (struct eth_device *dev);
>  #endif
> -/* Processes a received packet */
> -extern void NetReceive (volatile uchar *, int);
>
>  extern unsigned int INTERNAL_REG_BASE_ADDR;
>
> diff --git a/include/net.h b/include/net.h
> index e4d42c2..ccdb01d 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -342,9 +342,9 @@ extern uchar                NetOurEther[6];         /* Our ethernet address         */
>  extern uchar           NetServerEther[6];      /* Boot server enet address     */
>  extern IPaddr_t                NetOurIP;               /* Our    IP addr (0 = unknown) */
>  extern IPaddr_t                NetServerIP;            /* Server IP addr (0 = unknown) */
> -extern volatile uchar * NetTxPacket;           /* THE transmit packet          */
> -extern volatile uchar * NetRxPackets[PKTBUFSRX];/* Receive packets             */
> -extern volatile uchar * NetRxPacket;           /* Current receive packet       */
> +extern uchar           *NetTxPacket;           /* THE transmit packet */
> +extern uchar           *NetRxPackets[PKTBUFSRX];/* Receive packets */
> +extern uchar           *NetRxPacket;           /* Current receive packet */
>  extern int             NetRxPacketLen;         /* Current rx packet length     */
>  extern unsigned                NetIPID;                /* IP ID (counting)             */
>  extern uchar           NetBcastAddr[6];        /* Ethernet boardcast address   */
> @@ -408,10 +408,10 @@ extern void       NetStartAgain(void);
>  extern int     NetEthHdrSize(void);
>
>  /* Set ethernet header; returns the size of the header */
> -extern int     NetSetEther(volatile uchar *, uchar *, uint);
> +extern int NetSetEther(uchar *, uchar *, uint);
>
>  /* Set IP header */
> -extern void    NetSetIP(volatile uchar *, IPaddr_t, int, int, int);
> +extern void NetSetIP(uchar *, IPaddr_t, int, int, int);
>
>  /* Checksum */
>  extern int     NetCksumOk(uchar *, int);       /* Return true if cksum OK      */
> @@ -423,7 +423,7 @@ extern void net_set_icmp_handler(rxhand_icmp_f *f); /* Set ICMP RX handler */
>  extern void    NetSetTimeout(ulong, thand_f *);/* Set timeout handler          */
>
>  /* Transmit "NetTxPacket" */
> -extern void    NetSendPacket(volatile uchar *, int);
> +extern void NetSendPacket(uchar *, int);
>
>  /* Transmit UDP packet, performing ARP request if needed */
>  extern int     NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, int len);
> @@ -445,7 +445,7 @@ void net_auto_load(void);
>  * footprint in our tests.
>  */
>  /* return IP *in network byteorder* */
> -static inline IPaddr_t NetReadIP(volatile void *from)
> +static inline IPaddr_t NetReadIP(void *from)
>  {
>        IPaddr_t ip;
>        memcpy((void*)&ip, (void*)from, sizeof(ip));
> @@ -467,7 +467,7 @@ static inline void NetWriteIP(void *to, IPaddr_t ip)
>  }
>
>  /* copy IP */
> -static inline void NetCopyIP(volatile void *to, void *from)
> +static inline void NetCopyIP(void *to, void *from)
>  {
>        memcpy((void*)to, from, sizeof(IPaddr_t));
>  }
> diff --git a/net/bootp.c b/net/bootp.c
> index 34124b8..6d40608 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -585,7 +585,7 @@ static int BootpExtended (u8 * e)
>  void
>  BootpRequest (void)
>  {
> -       volatile uchar *pkt, *iphdr;
> +       uchar *pkt, *iphdr;
>        Bootp_t *bp;
>        int ext_len, pktlen, iplen;
>
> @@ -837,7 +837,7 @@ static int DhcpMessageType(unsigned char *popt)
>
>  static void DhcpSendRequestPkt(Bootp_t *bp_offer)
>  {
> -       volatile uchar *pkt, *iphdr;
> +       uchar *pkt, *iphdr;
>        Bootp_t *bp;
>        int pktlen, iplen, extlen;
>        IPaddr_t OfferedIP;
> diff --git a/net/net.c b/net/net.c
> index 045405b..29e018c 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -152,7 +152,7 @@ IPaddr_t    NetOurIP;
>  /* Server IP addr (0 = unknown) */
>  IPaddr_t       NetServerIP;
>  /* Current receive packet */
> -volatile uchar *NetRxPacket;
> +uchar *NetRxPacket;
>  /* Current rx packet length */
>  int            NetRxPacketLen;
>  /* IP packet ID */
> @@ -208,10 +208,10 @@ void NcStart(void);
>  int nc_input_packet(uchar *pkt, unsigned dest, unsigned src, unsigned len);
>  #endif
>
> -volatile uchar PktBuf[(PKTBUFSRX+1) * PKTSIZE_ALIGN + PKTALIGN];
> +uchar PktBuf[(PKTBUFSRX+1) * PKTSIZE_ALIGN + PKTALIGN];
>
>  /* Receive packet */
> -volatile uchar *NetRxPackets[PKTBUFSRX];
> +uchar *NetRxPackets[PKTBUFSRX];
>
>  /* Current RX packet handler */
>  static rxhand_f *packetHandler;
> @@ -225,7 +225,7 @@ static ulong        timeStart;
>  /* Current timeout value */
>  static ulong   timeDelta;
>  /* THE transmit packet */
> -volatile uchar *NetTxPacket;
> +uchar *NetTxPacket;
>
>  static int net_check_prereq(enum proto_t protocol);
>
> @@ -246,7 +246,7 @@ int         NetArpWaitTry;
>
>  void ArpRequest(void)
>  {
> -       volatile uchar *pkt;
> +       uchar *pkt;
>        ARP_t *arp;
>
>        debug("ARP broadcast %d\n", NetArpWaitTry);
> @@ -704,7 +704,7 @@ NetSetTimeout(ulong iv, thand_f *f)
>
>
>  void
> -NetSendPacket(volatile uchar *pkt, int len)
> +NetSendPacket(uchar *pkt, int len)
>  {
>        (void) eth_send(pkt, len);
>  }
> @@ -767,8 +767,8 @@ static ushort PingSeqNo;
>  int PingSend(void)
>  {
>        static uchar mac[6];
> -       volatile IP_t *ip;
> -       volatile ushort *s;
> +       IP_t *ip;
> +       ushort *s;
>        uchar *pkt;
>
>        /* XXX always send arp request */
> @@ -783,7 +783,7 @@ int PingSend(void)
>        pkt = NetArpWaitTxPacket;
>        pkt += NetSetEther(pkt, mac, PROT_IP);
>
> -       ip = (volatile IP_t *)pkt;
> +       ip = (IP_t *)pkt;
>
>        /*
>         * Construct an IP and ICMP header.
> @@ -935,9 +935,9 @@ static ushort CDP_compute_csum(const uchar *buff, ushort len)
>
>  int CDPSendTrigger(void)
>  {
> -       volatile uchar *pkt;
> -       volatile ushort *s;
> -       volatile ushort *cp;
> +       uchar *pkt;
> +       ushort *s;
> +       ushort *cp;
>        Ethernet_t *et;
>        int len;
>        ushort chksum;
> @@ -964,7 +964,7 @@ int CDPSendTrigger(void)
>        /* CDP header */
>        *pkt++ = 0x02;                          /* CDP version 2 */
>        *pkt++ = 180;                           /* TTL */
> -       s = (volatile ushort *)pkt;
> +       s = (ushort *)pkt;
>        cp = s;
>        /* checksum (0 for later calculation) */
>        *s++ = htons(0);
> @@ -1454,9 +1454,9 @@ NetReceive(volatile uchar *inpkt, int len)
>
>        debug("packet received\n");
>
> -       NetRxPacket = inpkt;
> +       NetRxPacket = (uchar *)inpkt;
>        NetRxPacketLen = len;
> -       et = (Ethernet_t *)inpkt;
> +       et = (Ethernet_t *)NetRxPacket;

Why change this?

>
>        /* too small packet? */
>        if (len < ETHER_HDR_SIZE)
> @@ -1491,11 +1491,11 @@ NetReceive(volatile uchar *inpkt, int len)
>                 */
>                x = ntohs(et->et_prot);
>
> -               ip = (IP_t *)(inpkt + E802_HDR_SIZE);
> +               ip = (IP_t *)(NetRxPacket + E802_HDR_SIZE);

and these? You are using a global instead of the passed-in local.

>                len -= E802_HDR_SIZE;
>
>        } else if (x != PROT_VLAN) {    /* normal packet */
> -               ip = (IP_t *)(inpkt + ETHER_HDR_SIZE);
> +               ip = (IP_t *)(NetRxPacket + ETHER_HDR_SIZE);


>                len -= ETHER_HDR_SIZE;
>
>        } else {                        /* VLAN packet */
> @@ -1519,7 +1519,7 @@ NetReceive(volatile uchar *inpkt, int len)
>                vlanid = cti & VLAN_IDMASK;
>                x = ntohs(vet->vet_type);
>
> -               ip = (IP_t *)(inpkt + VLAN_ETHER_HDR_SIZE);
> +               ip = (IP_t *)(NetRxPacket + VLAN_ETHER_HDR_SIZE);


>                len -= VLAN_ETHER_HDR_SIZE;
>        }
>
> @@ -1917,7 +1917,7 @@ NetEthHdrSize(void)
>  }
>
>  int
> -NetSetEther(volatile uchar *xet, uchar * addr, uint prot)
> +NetSetEther(uchar *xet, uchar * addr, uint prot)
>  {
>        Ethernet_t *et = (Ethernet_t *)xet;
>        ushort myvlanid;
> @@ -1942,7 +1942,7 @@ NetSetEther(volatile uchar *xet, uchar * addr, uint prot)
>  }
>
>  void
> -NetSetIP(volatile uchar *xip, IPaddr_t dest, int dport, int sport, int len)
> +NetSetIP(uchar *xip, IPaddr_t dest, int dport, int sport, int len)
>  {
>        IP_t *ip = (IP_t *)xip;
>
> diff --git a/net/rarp.c b/net/rarp.c
> index 097f970..77d63e8 100644
> --- a/net/rarp.c
> +++ b/net/rarp.c
> @@ -71,7 +71,7 @@ void
>  RarpRequest (void)
>  {
>        int i;
> -       volatile uchar *pkt;
> +       uchar *pkt;
>        ARP_t * rarp;
>
>        printf("RARP broadcast %d\n", ++RarpTry);
> diff --git a/net/tftp.c b/net/tftp.c
> index 7aa3e23..e62f229 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -310,9 +310,9 @@ static void
>  TftpSend(void)
>  {
>        uchar *pkt;
> -       volatile uchar *xp;
> -       int             len = 0;
> -       volatile ushort *s;
> +       uchar *xp;
> +       int len = 0;
> +       ushort *s;
>
>  #ifdef CONFIG_MCAST_TFTP
>        /* Multicast TFTP.. non-MasterClients do not ACK data. */
> --
> 1.6.0.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Regards,
Simon
Joe Hershberger Jan. 20, 2012, 8:15 p.m. UTC | #2 
Hi Simon,

On Fri, Jan 20, 2012 at 10:22 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Joe,
>
> On Thu, Jan 19, 2012 at 4:53 PM, Joe Hershberger <joe.hershberger@ni.com> wrote:
>> The mv_eth driver should not redefine the net function definition
>>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> Cc: Joe Hershberger <joe.hershberger@gmail.com>
>> Cc: Wolfgang Denk <wd@denx.de>
>> ---
>>  <snip>
>> @@ -1454,9 +1454,9 @@ NetReceive(volatile uchar *inpkt, int len)
>>
>>        debug("packet received\n");
>>
>> -       NetRxPacket = inpkt;
>> +       NetRxPacket = (uchar *)inpkt;
>>        NetRxPacketLen = len;
>> -       et = (Ethernet_t *)inpkt;
>> +       et = (Ethernet_t *)NetRxPacket;
>
> Why change this?
>
>>
>>        /* too small packet? */
>>        if (len < ETHER_HDR_SIZE)
>> @@ -1491,11 +1491,11 @@ NetReceive(volatile uchar *inpkt, int len)
>>                 */
>>                x = ntohs(et->et_prot);
>>
>> -               ip = (IP_t *)(inpkt + E802_HDR_SIZE);
>> +               ip = (IP_t *)(NetRxPacket + E802_HDR_SIZE);
>
> and these? You are using a global instead of the passed-in local.
>
>>                len -= E802_HDR_SIZE;
>>
>>        } else if (x != PROT_VLAN) {    /* normal packet */
>> -               ip = (IP_t *)(inpkt + ETHER_HDR_SIZE);
>> +               ip = (IP_t *)(NetRxPacket + ETHER_HDR_SIZE);
>
>
>>                len -= ETHER_HDR_SIZE;
>>
>>        } else {                        /* VLAN packet */
>> @@ -1519,7 +1519,7 @@ NetReceive(volatile uchar *inpkt, int len)
>>                vlanid = cti & VLAN_IDMASK;
>>                x = ntohs(vet->vet_type);
>>
>> -               ip = (IP_t *)(inpkt + VLAN_ETHER_HDR_SIZE);
>> +               ip = (IP_t *)(NetRxPacket + VLAN_ETHER_HDR_SIZE);
>
>
>>                len -= VLAN_ETHER_HDR_SIZE;
>>        }
>>

This patch removes volatile from the NetRxPacket and all but 1 of the
other places that inpkt was assigned.  You will notice that the first
assignment of inpkt to NetRxPacket casts away the volatile:

>> @@ -1454,9 +1454,9 @@ NetReceive(volatile uchar *inpkt, int len)
>> <snip>
>> -       NetRxPacket = inpkt;
>> +       NetRxPacket = (uchar *)inpkt;

All the assignments that need a non-volatile pointer now use
NetRxPacket instead of inpkt, since it is already assigned and and the
same type minus volatile.

Best regards,
-Joe
Simon Glass Jan. 24, 2012, 6:09 a.m. UTC | #3 
Hi Joe,

On Fri, Jan 20, 2012 at 12:15 PM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Simon,
>
> On Fri, Jan 20, 2012 at 10:22 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Joe,
>>
>> On Thu, Jan 19, 2012 at 4:53 PM, Joe Hershberger <joe.hershberger@ni.com> wrote:
>>> The mv_eth driver should not redefine the net function definition
>>>
>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>> Cc: Joe Hershberger <joe.hershberger@gmail.com>
>>> Cc: Wolfgang Denk <wd@denx.de>
>>> ---
>>>  <snip>
>>> @@ -1454,9 +1454,9 @@ NetReceive(volatile uchar *inpkt, int len)
>>>
>>>        debug("packet received\n");
>>>
>>> -       NetRxPacket = inpkt;
>>> +       NetRxPacket = (uchar *)inpkt;
>>>        NetRxPacketLen = len;
>>> -       et = (Ethernet_t *)inpkt;
>>> +       et = (Ethernet_t *)NetRxPacket;
>>
>> Why change this?
>>
>>>
>>>        /* too small packet? */
>>>        if (len < ETHER_HDR_SIZE)
>>> @@ -1491,11 +1491,11 @@ NetReceive(volatile uchar *inpkt, int len)
>>>                 */
>>>                x = ntohs(et->et_prot);
>>>
>>> -               ip = (IP_t *)(inpkt + E802_HDR_SIZE);
>>> +               ip = (IP_t *)(NetRxPacket + E802_HDR_SIZE);
>>
>> and these? You are using a global instead of the passed-in local.
>>
>>>                len -= E802_HDR_SIZE;
>>>
>>>        } else if (x != PROT_VLAN) {    /* normal packet */
>>> -               ip = (IP_t *)(inpkt + ETHER_HDR_SIZE);
>>> +               ip = (IP_t *)(NetRxPacket + ETHER_HDR_SIZE);
>>
>>
>>>                len -= ETHER_HDR_SIZE;
>>>
>>>        } else {                        /* VLAN packet */
>>> @@ -1519,7 +1519,7 @@ NetReceive(volatile uchar *inpkt, int len)
>>>                vlanid = cti & VLAN_IDMASK;
>>>                x = ntohs(vet->vet_type);
>>>
>>> -               ip = (IP_t *)(inpkt + VLAN_ETHER_HDR_SIZE);
>>> +               ip = (IP_t *)(NetRxPacket + VLAN_ETHER_HDR_SIZE);
>>
>>
>>>                len -= VLAN_ETHER_HDR_SIZE;
>>>        }
>>>
>
> This patch removes volatile from the NetRxPacket and all but 1 of the
> other places that inpkt was assigned.  You will notice that the first
> assignment of inpkt to NetRxPacket casts away the volatile:

Yes - I wonder why NetReceive needs to remain volatile?

>
>>> @@ -1454,9 +1454,9 @@ NetReceive(volatile uchar *inpkt, int len)
>>> <snip>
>>> -       NetRxPacket = inpkt;
>>> +       NetRxPacket = (uchar *)inpkt;
>
> All the assignments that need a non-volatile pointer now use
> NetRxPacket instead of inpkt, since it is already assigned and and the
> same type minus volatile.

Yes, I am only sensitive to this because it is a global and there is
enough use of globals in the net code already.

>
> Best regards,
> -Joe

Regards,
Simon
Joe Hershberger Jan. 24, 2012, 6:27 a.m. UTC | #4 
Hi Simon,

On Tue, Jan 24, 2012 at 12:09 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Joe,
>
> On Fri, Jan 20, 2012 at 12:15 PM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Simon,
>>
>> On Fri, Jan 20, 2012 at 10:22 AM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Joe,
>>>
>>> On Thu, Jan 19, 2012 at 4:53 PM, Joe Hershberger <joe.hershberger@ni.com> wrote:
>>>> The mv_eth driver should not redefine the net function definition
>>>>
>>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>>> Cc: Joe Hershberger <joe.hershberger@gmail.com>
>>>> Cc: Wolfgang Denk <wd@denx.de>
>>>> ---
>>>>  <snip>
>>>> @@ -1454,9 +1454,9 @@ NetReceive(volatile uchar *inpkt, int len)
>>>>
>>>>        debug("packet received\n");
>>>>
>>>> -       NetRxPacket = inpkt;
>>>> +       NetRxPacket = (uchar *)inpkt;
>>>>        NetRxPacketLen = len;
>>>> -       et = (Ethernet_t *)inpkt;
>>>> +       et = (Ethernet_t *)NetRxPacket;
>>>
>>> Why change this?
>>>
>>>>
>>>>        /* too small packet? */
>>>>        if (len < ETHER_HDR_SIZE)
>>>> @@ -1491,11 +1491,11 @@ NetReceive(volatile uchar *inpkt, int len)
>>>>                 */
>>>>                x = ntohs(et->et_prot);
>>>>
>>>> -               ip = (IP_t *)(inpkt + E802_HDR_SIZE);
>>>> +               ip = (IP_t *)(NetRxPacket + E802_HDR_SIZE);
>>>
>>> and these? You are using a global instead of the passed-in local.
>>>
>>>>                len -= E802_HDR_SIZE;
>>>>
>>>>        } else if (x != PROT_VLAN) {    /* normal packet */
>>>> -               ip = (IP_t *)(inpkt + ETHER_HDR_SIZE);
>>>> +               ip = (IP_t *)(NetRxPacket + ETHER_HDR_SIZE);
>>>
>>>
>>>>                len -= ETHER_HDR_SIZE;
>>>>
>>>>        } else {                        /* VLAN packet */
>>>> @@ -1519,7 +1519,7 @@ NetReceive(volatile uchar *inpkt, int len)
>>>>                vlanid = cti & VLAN_IDMASK;
>>>>                x = ntohs(vet->vet_type);
>>>>
>>>> -               ip = (IP_t *)(inpkt + VLAN_ETHER_HDR_SIZE);
>>>> +               ip = (IP_t *)(NetRxPacket + VLAN_ETHER_HDR_SIZE);
>>>
>>>
>>>>                len -= VLAN_ETHER_HDR_SIZE;
>>>>        }
>>>>
>>
>> This patch removes volatile from the NetRxPacket and all but 1 of the
>> other places that inpkt was assigned.  You will notice that the first
>> assignment of inpkt to NetRxPacket casts away the volatile:
>
> Yes - I wonder why NetReceive needs to remain volatile?

It is called by all the Ethernet drivers with volatile buffer
pointers.  At this point, I decided to limit the changes to not
include every Ethernet driver (have to split it somewhere).

>>>> @@ -1454,9 +1454,9 @@ NetReceive(volatile uchar *inpkt, int len)
>>>> <snip>
>>>> -       NetRxPacket = inpkt;
>>>> +       NetRxPacket = (uchar *)inpkt;
>>
>> All the assignments that need a non-volatile pointer now use
>> NetRxPacket instead of inpkt, since it is already assigned and and the
>> same type minus volatile.
>
> Yes, I am only sensitive to this because it is a global and there is
> enough use of globals in the net code already.

I chose to not add a local variable to hold the non-volatile pointer
since there was already a global.  Since u-boot is single threaded,
this should not be a problem and could be easily changed later when
the Ethernet driver interface is cleaned up.

Best regards,
-Joe
Mike Frysinger Feb. 3, 2012, 11:39 a.m. UTC | #5 
On Tuesday 24 January 2012 01:27:53 Joe Hershberger wrote:
> On Tue, Jan 24, 2012 at 12:09 AM, Simon Glass wrote:
> > On Fri, Jan 20, 2012 at 12:15 PM, Joe Hershberger wrote:
> >> On Fri, Jan 20, 2012 at 10:22 AM, Simon Glass wrote:
> >>> On Thu, Jan 19, 2012 at 4:53 PM, Joe Hershberger wrote:
> >>>> @@ -1454,9 +1454,9 @@ NetReceive(volatile uchar *inpkt, int len)
> >>>> <snip>
> >>>> -       NetRxPacket = inpkt;
> >>>> +       NetRxPacket = (uchar *)inpkt;
> >> 
> >> All the assignments that need a non-volatile pointer now use
> >> NetRxPacket instead of inpkt, since it is already assigned and and the
> >> same type minus volatile.
> > 
> > Yes, I am only sensitive to this because it is a global and there is
> > enough use of globals in the net code already.
> 
> I chose to not add a local variable to hold the non-volatile pointer
> since there was already a global.  Since u-boot is single threaded,
> this should not be a problem and could be easily changed later when
> the Ethernet driver interface is cleaned up.

funcs given a ptr should operate on that ptr.  relying on global variables is 
a step backwards imo.
-mike
Mike Frysinger Feb. 3, 2012, 11:44 a.m. UTC | #6 
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.
-mike
Joe Hershberger Feb. 7, 2012, 5:41 p.m. UTC | #7 
Hi Mike,

On Fri, Feb 3, 2012 at 5:44 AM, Mike Frysinger <vapier@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.
> -mike

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?

Thanks,
-Joe
diff mbox

Patch

diff --git a/board/Marvell/db64360/mv_eth.c b/board/Marvell/db64360/mv_eth.c
index 6340585..550610e 100644
--- a/board/Marvell/db64360/mv_eth.c
+++ b/board/Marvell/db64360/mv_eth.c
@@ -95,8 +95,6 @@  int mv64360_eth_xmit (struct eth_device *, volatile void *packet, int length);
 #ifndef  UPDATE_STATS_BY_SOFTWARE
 static void mv64360_eth_print_stat (struct eth_device *dev);
 #endif
-/* Processes a received packet */
-extern void NetReceive (volatile uchar *, int);
 
 extern unsigned int INTERNAL_REG_BASE_ADDR;
 
diff --git a/board/Marvell/db64460/mv_eth.c b/board/Marvell/db64460/mv_eth.c
index 4aefbaf..c447229 100644
--- a/board/Marvell/db64460/mv_eth.c
+++ b/board/Marvell/db64460/mv_eth.c
@@ -95,8 +95,6 @@  int mv64460_eth_xmit (struct eth_device *, volatile void *packet, int length);
 #ifndef  UPDATE_STATS_BY_SOFTWARE
 static void mv64460_eth_print_stat (struct eth_device *dev);
 #endif
-/* Processes a received packet */
-extern void NetReceive (volatile uchar *, int);
 
 extern unsigned int INTERNAL_REG_BASE_ADDR;
 
diff --git a/board/esd/cpci750/mv_eth.c b/board/esd/cpci750/mv_eth.c
index 001c1ad..db2324b 100644
--- a/board/esd/cpci750/mv_eth.c
+++ b/board/esd/cpci750/mv_eth.c
@@ -95,8 +95,6 @@  int mv64360_eth_xmit (struct eth_device *, volatile void *packet, int length);
 #ifndef  UPDATE_STATS_BY_SOFTWARE
 static void mv64360_eth_print_stat (struct eth_device *dev);
 #endif
-/* Processes a received packet */
-extern void NetReceive (volatile uchar *, int);
 
 extern unsigned int INTERNAL_REG_BASE_ADDR;
 
diff --git a/board/prodrive/p3mx/mv_eth.c b/board/prodrive/p3mx/mv_eth.c
index 15b3bfc..f26ded8 100644
--- a/board/prodrive/p3mx/mv_eth.c
+++ b/board/prodrive/p3mx/mv_eth.c
@@ -109,8 +109,6 @@  int phy_setup_aneg (char *devname, unsigned char addr);
 #ifndef	 UPDATE_STATS_BY_SOFTWARE
 static void mv64460_eth_print_stat (struct eth_device *dev);
 #endif
-/* Processes a received packet */
-extern void NetReceive (volatile uchar *, int);
 
 extern unsigned int INTERNAL_REG_BASE_ADDR;
 
diff --git a/include/net.h b/include/net.h
index e4d42c2..ccdb01d 100644
--- a/include/net.h
+++ b/include/net.h
@@ -342,9 +342,9 @@  extern uchar		NetOurEther[6];		/* Our ethernet address		*/
 extern uchar		NetServerEther[6];	/* Boot server enet address	*/
 extern IPaddr_t		NetOurIP;		/* Our    IP addr (0 = unknown)	*/
 extern IPaddr_t		NetServerIP;		/* Server IP addr (0 = unknown)	*/
-extern volatile uchar * NetTxPacket;		/* THE transmit packet		*/
-extern volatile uchar * NetRxPackets[PKTBUFSRX];/* Receive packets		*/
-extern volatile uchar * NetRxPacket;		/* Current receive packet	*/
+extern uchar		*NetTxPacket;		/* THE transmit packet */
+extern uchar		*NetRxPackets[PKTBUFSRX];/* Receive packets */
+extern uchar		*NetRxPacket;		/* Current receive packet */
 extern int		NetRxPacketLen;		/* Current rx packet length	*/
 extern unsigned		NetIPID;		/* IP ID (counting)		*/
 extern uchar		NetBcastAddr[6];	/* Ethernet boardcast address	*/
@@ -408,10 +408,10 @@  extern void	NetStartAgain(void);
 extern int	NetEthHdrSize(void);
 
 /* Set ethernet header; returns the size of the header */
-extern int	NetSetEther(volatile uchar *, uchar *, uint);
+extern int NetSetEther(uchar *, uchar *, uint);
 
 /* Set IP header */
-extern void	NetSetIP(volatile uchar *, IPaddr_t, int, int, int);
+extern void NetSetIP(uchar *, IPaddr_t, int, int, int);
 
 /* Checksum */
 extern int	NetCksumOk(uchar *, int);	/* Return true if cksum OK	*/
@@ -423,7 +423,7 @@  extern void net_set_icmp_handler(rxhand_icmp_f *f); /* Set ICMP RX handler */
 extern void	NetSetTimeout(ulong, thand_f *);/* Set timeout handler		*/
 
 /* Transmit "NetTxPacket" */
-extern void	NetSendPacket(volatile uchar *, int);
+extern void NetSendPacket(uchar *, int);
 
 /* Transmit UDP packet, performing ARP request if needed */
 extern int	NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, int len);
@@ -445,7 +445,7 @@  void net_auto_load(void);
  * footprint in our tests.
  */
 /* return IP *in network byteorder* */
-static inline IPaddr_t NetReadIP(volatile void *from)
+static inline IPaddr_t NetReadIP(void *from)
 {
 	IPaddr_t ip;
 	memcpy((void*)&ip, (void*)from, sizeof(ip));
@@ -467,7 +467,7 @@  static inline void NetWriteIP(void *to, IPaddr_t ip)
 }
 
 /* copy IP */
-static inline void NetCopyIP(volatile void *to, void *from)
+static inline void NetCopyIP(void *to, void *from)
 {
 	memcpy((void*)to, from, sizeof(IPaddr_t));
 }
diff --git a/net/bootp.c b/net/bootp.c
index 34124b8..6d40608 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -585,7 +585,7 @@  static int BootpExtended (u8 * e)
 void
 BootpRequest (void)
 {
-	volatile uchar *pkt, *iphdr;
+	uchar *pkt, *iphdr;
 	Bootp_t *bp;
 	int ext_len, pktlen, iplen;
 
@@ -837,7 +837,7 @@  static int DhcpMessageType(unsigned char *popt)
 
 static void DhcpSendRequestPkt(Bootp_t *bp_offer)
 {
-	volatile uchar *pkt, *iphdr;
+	uchar *pkt, *iphdr;
 	Bootp_t *bp;
 	int pktlen, iplen, extlen;
 	IPaddr_t OfferedIP;
diff --git a/net/net.c b/net/net.c
index 045405b..29e018c 100644
--- a/net/net.c
+++ b/net/net.c
@@ -152,7 +152,7 @@  IPaddr_t	NetOurIP;
 /* Server IP addr (0 = unknown) */
 IPaddr_t	NetServerIP;
 /* Current receive packet */
-volatile uchar *NetRxPacket;
+uchar *NetRxPacket;
 /* Current rx packet length */
 int		NetRxPacketLen;
 /* IP packet ID */
@@ -208,10 +208,10 @@  void NcStart(void);
 int nc_input_packet(uchar *pkt, unsigned dest, unsigned src, unsigned len);
 #endif
 
-volatile uchar	PktBuf[(PKTBUFSRX+1) * PKTSIZE_ALIGN + PKTALIGN];
+uchar PktBuf[(PKTBUFSRX+1) * PKTSIZE_ALIGN + PKTALIGN];
 
 /* Receive packet */
-volatile uchar *NetRxPackets[PKTBUFSRX];
+uchar *NetRxPackets[PKTBUFSRX];
 
 /* Current RX packet handler */
 static rxhand_f *packetHandler;
@@ -225,7 +225,7 @@  static ulong	timeStart;
 /* Current timeout value */
 static ulong	timeDelta;
 /* THE transmit packet */
-volatile uchar *NetTxPacket;
+uchar *NetTxPacket;
 
 static int net_check_prereq(enum proto_t protocol);
 
@@ -246,7 +246,7 @@  int		NetArpWaitTry;
 
 void ArpRequest(void)
 {
-	volatile uchar *pkt;
+	uchar *pkt;
 	ARP_t *arp;
 
 	debug("ARP broadcast %d\n", NetArpWaitTry);
@@ -704,7 +704,7 @@  NetSetTimeout(ulong iv, thand_f *f)
 
 
 void
-NetSendPacket(volatile uchar *pkt, int len)
+NetSendPacket(uchar *pkt, int len)
 {
 	(void) eth_send(pkt, len);
 }
@@ -767,8 +767,8 @@  static ushort PingSeqNo;
 int PingSend(void)
 {
 	static uchar mac[6];
-	volatile IP_t *ip;
-	volatile ushort *s;
+	IP_t *ip;
+	ushort *s;
 	uchar *pkt;
 
 	/* XXX always send arp request */
@@ -783,7 +783,7 @@  int PingSend(void)
 	pkt = NetArpWaitTxPacket;
 	pkt += NetSetEther(pkt, mac, PROT_IP);
 
-	ip = (volatile IP_t *)pkt;
+	ip = (IP_t *)pkt;
 
 	/*
 	 * Construct an IP and ICMP header.
@@ -935,9 +935,9 @@  static ushort CDP_compute_csum(const uchar *buff, ushort len)
 
 int CDPSendTrigger(void)
 {
-	volatile uchar *pkt;
-	volatile ushort *s;
-	volatile ushort *cp;
+	uchar *pkt;
+	ushort *s;
+	ushort *cp;
 	Ethernet_t *et;
 	int len;
 	ushort chksum;
@@ -964,7 +964,7 @@  int CDPSendTrigger(void)
 	/* CDP header */
 	*pkt++ = 0x02;				/* CDP version 2 */
 	*pkt++ = 180;				/* TTL */
-	s = (volatile ushort *)pkt;
+	s = (ushort *)pkt;
 	cp = s;
 	/* checksum (0 for later calculation) */
 	*s++ = htons(0);
@@ -1454,9 +1454,9 @@  NetReceive(volatile uchar *inpkt, int len)
 
 	debug("packet received\n");
 
-	NetRxPacket = inpkt;
+	NetRxPacket = (uchar *)inpkt;
 	NetRxPacketLen = len;
-	et = (Ethernet_t *)inpkt;
+	et = (Ethernet_t *)NetRxPacket;
 
 	/* too small packet? */
 	if (len < ETHER_HDR_SIZE)
@@ -1491,11 +1491,11 @@  NetReceive(volatile uchar *inpkt, int len)
 		 */
 		x = ntohs(et->et_prot);
 
-		ip = (IP_t *)(inpkt + E802_HDR_SIZE);
+		ip = (IP_t *)(NetRxPacket + E802_HDR_SIZE);
 		len -= E802_HDR_SIZE;
 
 	} else if (x != PROT_VLAN) {	/* normal packet */
-		ip = (IP_t *)(inpkt + ETHER_HDR_SIZE);
+		ip = (IP_t *)(NetRxPacket + ETHER_HDR_SIZE);
 		len -= ETHER_HDR_SIZE;
 
 	} else {			/* VLAN packet */
@@ -1519,7 +1519,7 @@  NetReceive(volatile uchar *inpkt, int len)
 		vlanid = cti & VLAN_IDMASK;
 		x = ntohs(vet->vet_type);
 
-		ip = (IP_t *)(inpkt + VLAN_ETHER_HDR_SIZE);
+		ip = (IP_t *)(NetRxPacket + VLAN_ETHER_HDR_SIZE);
 		len -= VLAN_ETHER_HDR_SIZE;
 	}
 
@@ -1917,7 +1917,7 @@  NetEthHdrSize(void)
 }
 
 int
-NetSetEther(volatile uchar *xet, uchar * addr, uint prot)
+NetSetEther(uchar *xet, uchar * addr, uint prot)
 {
 	Ethernet_t *et = (Ethernet_t *)xet;
 	ushort myvlanid;
@@ -1942,7 +1942,7 @@  NetSetEther(volatile uchar *xet, uchar * addr, uint prot)
 }
 
 void
-NetSetIP(volatile uchar *xip, IPaddr_t dest, int dport, int sport, int len)
+NetSetIP(uchar *xip, IPaddr_t dest, int dport, int sport, int len)
 {
 	IP_t *ip = (IP_t *)xip;
 
diff --git a/net/rarp.c b/net/rarp.c
index 097f970..77d63e8 100644
--- a/net/rarp.c
+++ b/net/rarp.c
@@ -71,7 +71,7 @@  void
 RarpRequest (void)
 {
 	int i;
-	volatile uchar *pkt;
+	uchar *pkt;
 	ARP_t *	rarp;
 
 	printf("RARP broadcast %d\n", ++RarpTry);
diff --git a/net/tftp.c b/net/tftp.c
index 7aa3e23..e62f229 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -310,9 +310,9 @@  static void
 TftpSend(void)
 {
 	uchar *pkt;
-	volatile uchar *xp;
-	int		len = 0;
-	volatile ushort *s;
+	uchar *xp;
+	int len = 0;
+	ushort *s;
 
 #ifdef CONFIG_MCAST_TFTP
 	/* Multicast TFTP.. non-MasterClients do not ACK data. */