Hi,
I examined the proposed fix and the root cause of the issue. I found out
that all network related types definitions are missing the "packed"
attribute hence the crash on certain platforms may happen.
Actually we should have
begin_packed_struct 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) */
} end_packed_struct;
And the same for all network related types, so we can eliminate problems
like this.
As an alternative hotfix I can propose move pktbuf[CONFIG_NET_ETH_PKTSIZE +
CONFIG_NET_GUARDSIZE]; to be heap allocated (actually currently it already
is), so we should remove "pktbuf" from "struct cdcecm_driver_s" and replace
"self->dev.d_buf = self->pktbuf;" with "self->dev.d_buf =
kmm_zalloc(CONFIG_NET_ETH_PKTSIZE + CONFIG_NET_GUARDSIZE);". The
"kmm_free(self->dev.d_buf);" should be added in prior to any
"kmm_free(self);". In this case alignment will be handled by memory
allocator.
Best regards,
Petro
пн, 17 січ. 2022 р. о 14:53 Kalbus, Peter <p...@mailbox.org.invalid> пише:
> 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
> >>>>
> >>>>
> >>
> >>
>
