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 <p...@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 <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
>>>
>>>
>
