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

Reply via email to