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 >>> >>> > >
