This discussion has been locked.
You can no longer post new replies to this discussion. If you have a question you can start a new discussion

Question about StellarisWare, SW-RDK-S2E, stellarisif.c

stellarisif.c

static err_t
stellarisif_transmit(struct netif *netif, struct pbuf *p)
{
  int iBuf;
  unsigned char *pucBuf;
  unsigned long *pulBuf;
  struct pbuf *q;
  int iGather;
  unsigned long ulGather;
  unsigned char *pucGather;

........................... [omitted]

    /* Initialze a long pointer into the pbuf for 32-bit access. */
    pulBuf = (unsigned long *)&pucBuf[iBuf];

    /**
     * Copy words of pbuf data into the Tx FIFO, but don't go past
     * the end of the pbuf.
     *
     */
    while((iBuf + 4) <= q->len) {
      HWREG(ETH_BASE + MAC_O_DATA) = *pulBuf++;
      iBuf += 4;
    }

The above code is part of the latest version of the TI/Luminary StellarisWare.

I think it is a bug? And I hope to fix it with

  __packed unsigned long *pulBuf;


in the KEIL toolchain. (Tested.)

But due to my very limited C programming skill, I am afraid that, I might be doing something stupid. So I would like to learn something from our experts, please kindly give me some advices.

Is that, the __packed qualifier (in this case) leads to the lower performance?

  • SW-RDK-S2E-6459 (part of the StellarisWare) is the latest version, which can be download from:

    www.luminarymicro.com/index.php

    The mentioned code triggers a Usage Fault/UNALIGNED on my LM3S6432/LM3S6911 MCUs.

  • "TI/Luminary StellarisWare"

    It's TI/Luminary code - so why not post on the TI/Luminary forum?

    It's their code; they wrote the code, so they should know why they did this!

    You are right that it is not generally safe to arbitrarily cast a byte pointer to a word pointer - but maybe the rest of their code should ensure that, in this particular case, it is OK...?

    I hope to fix it with

     __packed unsigned long *pulBuf;
    

    You really need to check with TI/Luminary whether that is compatible with what they are trying to achieve here!

    Note: unlike Keil, TI/Luminary technical staff do monitor their forum and do respond to issues like this!

    So post this on the TI/Luminary forum: e2e.ti.com/.../default.aspx

    Being sure, of course, to include a link to this thread, and give a link here to the thread over there...

  • why are you posting here?

    Due to my very limited C programming skill, I am afraid that, I might be doing something stupid.

    You are right that it is not generally safe to arbitrarily cast a byte pointer to a word pointer - but maybe the rest of their code should ensure that, in this particular case, it is OK...?

    Many Thanks for your help, Andy.

  • "Due to my very limited C programming skill, I am afraid that, I might be doing something stupid."

    But that would apply equally to whichever forum - so why did you specifically choose this forum - rather than the TI forum?

    And, no - it wasn't a stupid question!

  • <quote>
    cast a byte pointer to a word pointer
    <end quote>

    According to the standards, doing this causes undefined behaviour, it is not permitted and cannot be done.

    If Hans notices, he will have a fit!

  • But that would apply equally to whichever forum - so why did you specifically choose this forum - rather than the TI forum?

    Hi Andy,

    Because You and Per.

    I understand that, the above answer is bad.
    But I do believe that, you will correct me and then teach me something, if I did something stupid.

  • John,

    As far as I am concerned, the only true stupid thing you can do is not to be sure about something and don't dare to ask! Remember: the one that asks the question (might) look stupid for 5 minutes; the one that does not dare asking remains stupid for eternity!

  • And I hope to fix it with ... __packed

    IIRC, the Cortex-M3 CPU in the Luminary MCU's supports unaligned accesses. That is, no need to use __packed, it will only lead to less efficient code.

    According to the standards, doing this causes undefined behaviour, it is not permitted and cannot be done.

    Unless the code is only run on a very specific CPU that supports this, in which case this is OK.

  • It's isn't the casting of a byte pointer to a word pointer in itself that causes undefined behaviour and is not permitted.

    It's the casting of a pointer with wrong alignment.

    A random byte pointer have 50% probability to point at an odd address, which may fail for processors that can't do 16-bit accesses to odd addresses. And it have 75% probability to not be aligned base 4, introducing problems for 32-bit accesses.

    But in existing code, the byte pointers are often not random. malloc() for example always returns the best required alignment for any object size. So if the code gets a pointer that must have come from malloc(), that pointer can be cast to any other access size without a problem. A pointer from malloc() that someone have done ++ with, will on the other hand get incorrect alignment depending on the type of pointer, i.e. how many char steps the ++ increments it.

    So if the code is valid or not will depend on two things:
    - if the processor supports unaligned access
    - if the omitted code does something that guarantees that the address is always correctly aligned.

  • <quote>
    It's the casting of a pointer with wrong alignment.
    <end quote>

    Actually, the casting operation itself does not cause a problem.

    It is the dereferencing of the pointer to the result of the cast that might throw a wobbler.

    But hey, let's wait for the legendary anger of Hans ;)

  • Many Thanks to All.

    The mentioned code:

    \third_party\lwip-1.3.2\ports\stellaris\netif\stellarisif.c

    /**
     * This function should do the actual transmission of the packet. The packet is
     * contained in the pbuf that is passed to the function. This pbuf might be
     * chained.
     *
     * @param netif the lwip network interface structure for this ethernetif
     * @param p the MAC packet to send (e.g. IP packet including MAC addresses and type)
     * @return ERR_OK if the packet could be sent
     *         an err_t value if the packet couldn't be sent
     * @note This function MUST be called with interrupts disabled or with the
     *       Stellaris Ethernet transmit fifo protected.
     */
    static err_t
    stellarisif_transmit(struct netif *netif, struct pbuf *p)
    {
      int iBuf;
      unsigned char *pucBuf;
      unsigned long *pulBuf;
      struct pbuf *q;
      int iGather;
      unsigned long ulGather;
      unsigned char *pucGather;
    
      /**
       * Fill in the first two bytes of the payload data (configured as padding
       * with ETH_PAD_SIZE = 2) with the total length of the payload data
       * (minus the Ethernet MAC layer header).
       *
       */
      *((unsigned short *)(p->payload)) = p->tot_len - 16;
    
      /* Initialize the gather register. */
      iGather = 0;
      pucGather = (unsigned char *)&ulGather;
      ulGather = 0;
    
      /* Copy data from the pbuf(s) into the TX Fifo. */
      for(q = p; q != NULL; q = q->next) {
        /* Intialize a char pointer and index to the pbuf payload data. */
        pucBuf = (unsigned char *)q->payload;
        iBuf = 0;
    
        /**
         * If the gather buffer has leftover data from a previous pbuf
         * in the chain, fill it up and write it to the Tx FIFO.
         *
         */
        while((iBuf < q->len) && (iGather != 0)) {
          /* Copy a byte from the pbuf into the gather buffer. */
          pucGather[iGather] = pucBuf[iBuf++];
    
          /* Increment the gather buffer index modulo 4. */
          iGather = ((iGather + 1) % 4);
        }
    
        /**
         * If the gather index is 0 and the pbuf index is non-zero,
         * we have a gather buffer to write into the Tx FIFO.
         *
         */
        if((iGather == 0) && (iBuf != 0)) {
          HWREG(ETH_BASE + MAC_O_DATA) = ulGather;
          ulGather = 0;
        }
    
        /* Initialze a long pointer into the pbuf for 32-bit access. */
        pulBuf = (unsigned long *)&pucBuf[iBuf];
    
        /**
         * Copy words of pbuf data into the Tx FIFO, but don't go past
         * the end of the pbuf.
         *
         */
        while((iBuf + 4) <= q->len) {
          HWREG(ETH_BASE + MAC_O_DATA) = *pulBuf++;
          iBuf += 4;
        }
    [Cut]
    

    \third_party\lwip-1.3.2\src\include\lwip\pbuf.h

    struct pbuf {
      /** next pbuf in singly linked pbuf chain */
      struct pbuf *next;
    
      /** pointer to the actual data in the buffer */
      void *payload;
    
      /**
       * total length of this buffer and all next buffers in chain
       * belonging to the same packet.
       *
       * For non-queue packet chains this is the invariant:
       * p->tot_len == p->len + (p->next? p->next->tot_len: 0)
       */
      u16_t tot_len;
    
      /** length of this buffer */
      u16_t len;
    
      /** pbuf_type as u8_t instead of enum to save space */
      u8_t /*pbuf_type*/ type;
    
      /** misc flags */
      u8_t flags;
    
      /**
       * the reference count always equals the number of pointers
       * that refer to this pbuf. This can be pointers from an application,
       * the stack itself, or pbuf->next pointers from a chain.
       */
      u16_t ref;
    
    #if LWIP_PTPD
      /* the time at which the packet was received, seconds component */
      u32_t time_s;
    
      /* the time at which the packet was received, nanoseconds component */
      u32_t time_ns;
    #endif /* #if LWIP_PTPD */
    };
    
    

  • Maybe I need to further check the pbuf,

    stellarisif_transmit(struct netif *netif, struct pbuf *p)
    


    but it doesn't look like the address is always correctly aligned.

    Yes, the LMI MCU has the unaligned data access feature, enables data to be efficiently packed into memory. But I am not sure if I properly enabled this feature, cause I created a completely new KEIL project to include the necessary software components only.

  • What you need to know, is where payload comes from. Maybe the payload is guaranteed to always be perfectly aligned.

    That while loop with iGather seems to indicate that they do try to keep track of data moduly 4 bytes, so it's quite likely that they make sure that the data is 4-byte-aligned.

  • Yes - that has been confirmed by TI on the TI forum.

    I say they should have made this abundantly clear in the function comments, though.