Hi Peter, Could you please send a Pull Request to mainline? Click on Pull Request tab and New Pull Request.
BR, Alan On 1/17/22, Peter Kalbus <p...@mailbox.org.invalid> wrote: > Hi Michael, > > understood … I changed the fix according to you proposal and tested it … > works fine,too: > > https://github.com/ptka/incubator-nuttx/commit/09be64bb1f2fff9f85a392e0fab6915f9083fe2d > <https://github.com/ptka/incubator-nuttx/commit/09be64bb1f2fff9f85a392e0fab6915f9083fe2d> > > 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 > */ > > aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE + > CONFIG_NET_GUARDSIZE]; > > struct usbdev_req_s *rdreq; /* Single read request */ > > > /Piet > > >> Am 17.01.2022 um 10:42 schrieb Michael Jung <mij...@gmx.net>: >> >> 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 <mailto: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 >>> <https://github.com/ptka/incubator-nuttx/commit/01732850ad345b5e2a9af24f788935d8f7928781> >>>> >>> >>> Please comment on the analysis and proposed fix. >>> >>> /Piet > >
