You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by Peter Kalbus <pt...@mailbox.org.INVALID> on 2022/01/17 08:01:17 UTC
rp2040: composite-cdcecm: hardfault due to alignment issue in driver/usbdev/usbdev.c
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
Re: rp2040: composite-cdcecm: hardfault due to alignment issue in driver/usbdev/usbdev.c
Posted by Peter Kalbus <pt...@mailbox.org.INVALID>.
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:19 schrieb Peter Kalbus <pt...@me.com>:
>
> 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 <ac...@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 <pt...@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 <mi...@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
>>>> <ptka@mailbox.org.invalid <ma...@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
>>>
>>>
>
Re: rp2040: composite-cdcecm: hardfault due to alignment issue in driver/usbdev/usbdev.c
Posted by Petro Karashchenko <pe...@gmail.com>.
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 <pt...@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 <acassis@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 <pt...@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
> >>> <ac...@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 <pt...@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 <mi...@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
> >>>>> <ptka@mailbox.org.invalid <ma...@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
> >>>>
> >>>>
> >>
> >>
>
Re: rp2040: composite-cdcecm: hardfault due to alignment issue in driver/usbdev/usbdev.c
Posted by "Kalbus, Peter" <pt...@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 <ac...@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 <pt...@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
>>> <ac...@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 <pt...@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 <mi...@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
>>>>> <ptka@mailbox.org.invalid <ma...@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
>>>>
>>>>
>>
>>
Re: rp2040: composite-cdcecm: hardfault due to alignment issue in driver/usbdev/usbdev.c
Posted by Alan Carvalho de Assis <ac...@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 <pt...@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
>> <ac...@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 <pt...@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 <mi...@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
>>>> <ptka@mailbox.org.invalid <ma...@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
>>>
>>>
>
>
Re: rp2040: composite-cdcecm: hardfault due to alignment issue in driver/usbdev/usbdev.c
Posted by Peter Kalbus <pt...@me.com.INVALID>.
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 <ac...@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 <pt...@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 <mi...@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
>>> <ptka@mailbox.org.invalid <ma...@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
>>
>>
Re: rp2040: composite-cdcecm: hardfault due to alignment issue in driver/usbdev/usbdev.c
Posted by Alan Carvalho de Assis <ac...@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 <pt...@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 <mi...@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
>> <ptka@mailbox.org.invalid <ma...@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
>
>
Re: rp2040: composite-cdcecm: hardfault due to alignment issue in driver/usbdev/usbdev.c
Posted by Peter Kalbus <pt...@mailbox.org.INVALID>.
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 <mi...@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
> <ptka@mailbox.org.invalid <ma...@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
Re: rp2040: composite-cdcecm: hardfault due to alignment issue in driver/usbdev/usbdev.c
Posted by Michael Jung <mi...@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
<pt...@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
>
>