Hi Peter,
good finding!
There is actually a comment in this regard in include/nuttx/net/netdev.h:
> The d_buf array must be aligned to two-byte, 16-bit address boundaries in
order to support aligned 16-bit accesses performed by the network.
W.r.t. to your proposed fix I think it would be more appropriate to make
the alignment requirement explicit by using the 'aligned_data' macro from
include/nuttx/compiler.h.
Thanks!
Michael
Am Mo., 17. Jan. 2022 um 09:01 Uhr schrieb Peter Kalbus
<p...@mailbox.org.invalid>:
> Hi all,
>
> during the bring-up of the composite-cdcecm a alignment issue result in
> hard faults (always on the used rp2040 based device).
>
> ANALYSIS:
>
> The root cause seems to be the way the pktbuf is defined in
> cdcecm_driver_s (driver/usbdev/cdcecm.c):
>
> struct cdcecm_driver_s
> {
> /* USB CDC-ECM device */
>
> struct usbdevclass_driver_s usbdev; /* USB device class vtable */
> struct usbdev_devinfo_s devinfo;
> FAR struct usbdev_req_s *ctrlreq; /* Allocated control request */
> FAR struct usbdev_ep_s *epint; /* Interrupt IN endpoint */
> FAR struct usbdev_ep_s *epbulkin; /* Bulk IN endpoint */
> FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
> uint8_t config; /* Selected configuration
> number */
>
> uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
> CONFIG_NET_GUARDSIZE];
>
>
> Due to 'config‘ element ‚ pktbuf‘ in the structure is not aligned in
> anyway from the struct itself.
>
> The driver structure is allocation during driver initialization in
> cdcecm_classobject():
>
> /* Initialize the driver structure */
>
> self = kmm_zalloc(sizeof(struct cdcecm_driver_s));
> if (!self)
> {
> nerr("Out of memory!\n");
> return -ENOMEM;
> }
>
> /* Network device initialization */
>
> self->dev.d_buf = self->pktbuf;
>
> As ’ self’ points to an aligned address, 'pktbuf' is always aligned at an
> odd address.
>
> Once the driver is initialized and processes received data in
> cdcecm_receive() the driver creates a hard fault at:
>
> #ifdef CONFIG_NET_IPv4
> if (BUF->type == HTONS(ETHTYPE_IP))
> {
>
> BUF/eth_hdr_s are defined as:
>
> #define BUF ((struct eth_hdr_s *)self->dev.d_buf)
>
>
> struct eth_hdr_s
> {
> uint8_t dest[6]; /* Ethernet destination address (6 bytes) */
> uint8_t src[6]; /* Ethernet source address (6 bytes) */
> uint16_t type; /* Type code (2 bytes) */
> };
>
>
> Element ’ type’ is always odd aligned and access to it results in all
> cases to a hard fault on the used rp2040 based board.
>
> PROPOSAL:
>
> Assuming the analysis is correct, there could be different options to fix
> there issue.
> Proposal would be to let the compiler do the job and not introduce any
> special code or elements in the structure to fix it programmatically.
>
> Analysing cdcdcm_driver_s in more details:
>
> FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
> uint8_t config; /* Selected configuration
> number */
>
> uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
> CONFIG_NET_GUARDSIZE];
>
>
> Element 'epbulkout' is a 32bit pointer and thus always aligned to 32bit.
> Element 'config‘ following is also aligned to 32bit —> as it’s only an
> uint8_t is doesn’t need to be aligned at all.
> Element ‚pktbuf‘ is odd aligned, but needs to be at least 16bit aligned.
>
> Exchanging 'config‘ and 'pktbuf‘ in the structure solves the issue:
>
> FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
>
> uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
> CONFIG_NET_GUARDSIZE];
>
> uint8_t config; /* Selected configuration
> number */
>
>
> Element ‚pktbuf‘ is now 32bit aligned as it’s following a 32bit pointer.
>
> The change has been tested on an rp2040 based device.
>
> The fix can be reviewed at:
> https://github.com/ptka/incubator-nuttx/commit/01732850ad345b5e2a9af24f788935d8f7928781
> <
> https://github.com/ptka/incubator-nuttx/commit/01732850ad345b5e2a9af24f788935d8f7928781
> >
>
> Please comment on the analysis and proposed fix.
>
> /Piet
>
>
