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 <[email protected]>:
>
> 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 <[email protected]> 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 <[email protected]>:
>>>
>>> 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
>>> <[email protected] <mailto:[email protected]>>:
>>>
>>>> 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
>>
>>
