Understood … but I‘d like to separate them in two different PRs … mixing up things is rarely a good idea … I‘ll create PR for the alignment issue later today. The PR for the general CDCECM on RP2040 will follow as it’s complete.
/Piet > Am 17.01.2022 um 13:43 schrieb Alan Carvalho de Assis <acas...@gmail.com>: > > It is possible to send two type of modifications in a single PR when > they are relative to the same logic goal (that is the case here). > > But please separate CDCECM support to rp2040 in a commit and the > alignment in another commit, so in the PR description you can use > something like this: > Add support to CDCECM for RaspberryPi Pico and fix an alignment issue. > > BR, > > Alan > >> On 1/17/22, Peter Kalbus <p...@me.com.invalid> wrote: >> Hi Alan, >> >> sure … can I create a PR from a single chasnge? >> >> Background is, that I have two changes on the branch: >> >> - older one: get CDCECM enabled in RP2040 in general —> not yet fully done >> - newer one: the fix from alignment issue >> >> /Piet >> >> >>> Am 17.01.2022 um 13:08 schrieb Alan Carvalho de Assis >>> <acas...@gmail.com>: >>> >>> 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 >>>> >>>> >> >>
