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