You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/17 13:36:20 UTC
[GitHub] [incubator-nuttx] ptka opened a new pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
ptka opened a new pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249
## Summary
aligment is needed to avoid hard fault in driver/usbdev/usbdev.c in cdcecm_receive() at:
#ifdef CONFIG_NET_IPv4
if (BUF->type == HTONS(ETHTYPE_IP))
{
Details: https://www.mail-archive.com/dev@nuttx.apache.org/msg07413.html
## Impact
configs using cdcecm driver
## Testing
tested on target (Tiny2040) connected to a LinuxVM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786112280
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
Maybe it is reasonable to change type from `uint8_t` to `uint32_t` and allocate in 32 bit words? This will eliminate dependencies to compiler.h.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786245870
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
I will check your proposal adding 'begin_packed_struct' ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786318309
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
@ptka I believe you and unfortunately I do not have tiny2040-composite board to try it. Is it possible for you to generate disassembly for `cdcecm_receive()`? I would like to examine what instructions are generated for `if (BUF->type == HTONS(ETHTYPE_IP))` so it leads for hard fault.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786295371
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
Your test program is correctly, for what you want to show, but it's not reflecting the problem. I extended the program to added the missing parts:
#include <stdint.h>
#include <stdlib.h>
#include <stdio.h>
#define uint8_t unsigned char
#define CONFIG_NET_ETH_PKTSIZE 590
#define CONFIG_NET_GUARDSIZE 2
struct net_driver_s
{
uint8_t *d_buf;
};
struct cdcecm_driver_s
{
uint8_t config; /* Selected configuration number */
uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
CONFIG_NET_GUARDSIZE];
struct net_driver_s dev;
};
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) */
};
struct packed_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) */
} __attribute__ ((packed));
uint8_t pkt[128];
#define BUF ((struct eth_hdr_s *)pkt)
#define BUF_PACKED ((struct packed_eth_hdr_s *)pkt)
int f(void)
{
if (BUF->type == 0x0008)
{
return 0;
}
return 1;
}
int f_packed(void)
{
if (BUF_PACKED->type == 0x0008)
{
return 0;
}
return 1;
}
#define BUF_ORIG ((struct eth_hdr_s *)self->dev.d_buf)
#define BUF_ORIG_PACKED ((struct packed_eth_hdr_s *)self->dev.d_buf)
int main(void)
{
struct cdcecm_driver_s *self = malloc(sizeof(struct cdcecm_driver_s));
printf("self = 0x%08lx\n", (unsigned long)self);
self->dev.d_buf = self->pktbuf;
printf("self->ptkbuf = 0x%08lx\n", (unsigned long)self->pktbuf);
printf("self->dev.d_buf = 0x%08lx\n", (unsigned long)self->dev.d_buf);
printf("&(BUF_ORIG->type) = 0x%08lx\n", (unsigned long)&BUF_ORIG->type);
printf("&(BUF_ORIG_PACKED->type) = 0x%08lx\n", (unsigned long)&BUF_ORIG_PACKED->type);
}
---------
The interesting part is:
struct cdcecm_driver_s
{
uint8_t config; /* Selected configuration number */
uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
CONFIG_NET_GUARDSIZE];
struct net_driver_s dev;
};
Due to 'config', pktbuf get's an odd address. I we use pktbuf for an eth_hdr_s structure, there's no way that the definition of eth_hdr_s can prevent, that 'type' doesn't get an odd address, too.
Please output of the program:
self = 0x136e04290 ---> malloc() return a well aligned address for cdcecm_driver_s structure
self->ptkbuf = 0x136e04291 --> pktbuf gets automatically an odd address due to 'config' element in front
self->dev.d_buf = 0x136e04291 --> that's obvious as d_buf is assigned from pktbus
&(BUF_ORIG->type) = 0x136e0429d --> address of type also gets an odd address --> CRASH
&(BUF_ORIG_PACKED->type) = 0x136e0429d --> packed attribute doesn't change anything
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786318309
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
@ptka I believe you and unfortunately I do not have tiny2040-composite board to try it. Is it possible for you to generate disassembly for `cdcecm_receive()`? I would like to examine what instructions are generated for `if (BUF->type == HTONS(ETHTYPE_IP))` so it leads for hard fault. I'm expecting something like
```
ldr r2, .L4
ldrb r1, [r2, #12]
ldrb r3, [r2, #13]
lsls r3, r3, #8
orrs r3, r1
```
but if it is not, then probably if will explain everything
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786254615
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
> Could be fixed by moving pktbuf before config?
Correct, that was my original idea --> take a look at the referenced mailing list information. There are several ways to fix the issue ... not sure what the best solution is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786264331
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
It is very strange because begin_packed_struct / end_packed_struct are exactly designed to handle the issues that you are meeting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786245302
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
That's right. But the issue is not with the eth_hdr_s structure, but the fact, that BUF macro casts the start of the structure to the odd aligned uint8 ptkbuf in cdcecm_driver_s which is the result on the uint8 config element.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786252367
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
... just tested on my targets ... begin_packed_struct / end_packed_struct doesn't fix the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#issuecomment-1015220744
Thanks for merging … you‘re welcome!
/Piet
> Am 18.01.2022 um 09:09 schrieb Petro Karashchenko ***@***.***>:
>
>
> @ptka thank you very much for time spent on the issue fix and the review!
>
> —
> Reply to this email directly, view it on GitHub, or unsubscribe.
> Triage notifications on the go with GitHub Mobile for iOS or Android.
> You are receiving this because you were mentioned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786169975
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
I changed using uint16_t to align to 16bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786120096
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
Than the calculation of the pktbuf size would need a modification (16bit alignment would be enough):
pktbuf[CONFIG_NET_ETH_PKTSIZE + CONFIG_NET_GUARDSIZE];
-->
pktbuf[(CONFIG_NET_ETH_PKTSIZE + CONFIG_NET_GUARDSIZE + 1) / 2];
Is the dependency to compiler.h less well supported on the different architecures?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786326970
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
Let's proceed with this change. Probably will need to add few `DEBUGASSERT`s to catch situations when memory pointed by `d_buf` is not 16-bit aligned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka removed a comment on pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka removed a comment on pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#issuecomment-1014958870
I did some further investigations and the issue is now at the next place in arp_table.c:
void arp_hdr_update(FAR struct net_driver_s *dev, FAR uint16_t *pipaddr,
FAR uint8_t *ethaddr)
{
in_addr_t ipaddr = net_ip4addr_conv32(pipaddr);
/* Update the ARP table */
Here it’s not an element of a struct, but a pointer to an uint16_t, which gets now the problem.
So the solution with begin_packed_struct/end_packed_struct but it’s not catching all issues with the unaligned d_buf.
Disassembling is not needed.
> Am 17.01.2022 um 23:39 schrieb Petro Karashchenko ***@***.***>:
>
>
> @pkarashchenko commented on this pull request.
>
> In drivers/usbdev/cdcecm.c <https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786318309>:
>
> > @@ -120,7 +120,7 @@ struct cdcecm_driver_s
> FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
> uint8_t config; /* Selected configuration number */
>
> - uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
> + aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
> @ptka <https://github.com/ptka> I believe you and unfortunately I do not have tiny2040-composite board to try it. Is it possible for you to generate disassembly for cdcecm_receive()? I would like to examine what instructions are generated for if (BUF->type == HTONS(ETHTYPE_IP)) so it leads for hard fault.
>
> —
> Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786318309>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGUYCXWHHQO67C4MCTTPLLUWSLBFANCNFSM5MEWZEKA>.
> Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
> You are receiving this because you were mentioned.
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#issuecomment-1014982463
Hi,
Yes. That seems to be true. I was confused about "if (BUF->type ==
HTONS(ETHTYPE_IP))" and I didn't expect it to fail anymore with a packed
definition. CI build
https://github.com/apache/incubator-nuttx/runs/4846754947?check_suite_focus=true
highlighted all the things that you are talking about. That is why I closed
https://github.com/apache/incubator-nuttx/pull/5251. This approach leads to
a massive reworking and I think that there value of benefits is less than
efforts that need to be spent.
Best regards,
Petro
вт, 18 січ. 2022 р. о 01:48 Peter Kalbus ***@***.***> пише:
> I did some further investigations and the issue is now at the next place
> in arp_table.c:
>
> void arp_hdr_update(FAR struct net_driver_s *dev, FAR uint16_t *pipaddr,
> FAR uint8_t *ethaddr)
> {
> in_addr_t ipaddr = net_ip4addr_conv32(pipaddr);
>
> /* Update the ARP table */
>
> Here it’s not an element of a struct, but a pointer to an uint16_t, which
> gets now the problem.
>
> So the solution with begin_packed_struct/end_packed_struct but it’s not
> catching all issues with the unaligned d_buf.
>
> Disassembling is not needed.
>
>
> > Am 17.01.2022 um 23:39 schrieb Petro Karashchenko ***@***.***>:
> >
> >
> > @pkarashchenko commented on this pull request.
> >
> > In drivers/usbdev/cdcecm.c <
> https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786318309
> >:
> >
> > > @@ -120,7 +120,7 @@ struct cdcecm_driver_s
> > FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
> > uint8_t config; /* Selected configuration number */
> >
> > - uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
> > + aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
> > @ptka <https://github.com/ptka> I believe you and unfortunately I do
> not have tiny2040-composite board to try it. Is it possible for you to
> generate disassembly for cdcecm_receive()? I would like to examine what
> instructions are generated for if (BUF->type == HTONS(ETHTYPE_IP)) so it
> leads for hard fault.
> >
> > —
> > Reply to this email directly, view it on GitHub <
> https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786318309>,
> or unsubscribe <
> https://github.com/notifications/unsubscribe-auth/AAGUYCXWHHQO67C4MCTTPLLUWSLBFANCNFSM5MEWZEKA
> >.
> > Triage notifications on the go with GitHub Mobile for iOS <
> https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android <
> https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> > You are receiving this because you were mentioned.
> >
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/incubator-nuttx/pull/5249#issuecomment-1014958870>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEHCOCPOU3ZNXG2QUNLZF4TUWSTF3ANCNFSM5MEWZEKA>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786301316
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
I double checked it on my target ...
/1/ original code --> crash due to odd address of 'type' element
NuttX 10.2.0 09be64bb1f-dirty Jan 17 2022 22:30:03 arm pimoroni-tiny2040
nsh> conn
self =0x200091f0 --> aligned address for allocated memory
self-dev.d_buf =0x20009231 --> odd address for 'pktbuf'
&(BUF->type) =0x2000923d --> odd address for 'type'
conn_main: Exiting
/2/ modified version using uint16_t ptkbuf --> working as 'type' is now 16bit aligned
NuttX 10.2.0 09be64bb1f-dirty Jan 17 2022 22:35:28 arm pimoroni-tiny2040
nsh> conn
conn_main: Performing architecture-specific initialization
self =0x200091f0 --> aligned address for allocated memory
self-dev.d_buf =0x20009232 --> even address for 'pktbuf'
&(BUF->type) =0x2000923e --> even address for 'type'
conn_main: Exiting
/3/ your proposal --> crash
NuttX 10.2.0 09be64bb1f-dirty Jan 17 2022 22:40:23 arm pimoroni-tiny2040
nsh> conn
conn_main: Performing architecture-specific initialization
self =0x200091f0
self-dev.d_buf =0x20009231
&(BUF->type) =0x2000923d
conn_main: Exiting
Command used from 'make -j8' to compile cdcecm.c:
CC: usbdev/cdcecm.c
arm-none-eabi-gcc -c -fno-builtin -Wall -Wstrict-prototypes -Wshadow -Wundef -g -mcpu=cortex-m0 -mthumb -mfloat-abi=soft -isystem "/Users/piet/Projects/NuttX/tiny2040-composite/nuttx/include" -D__NuttX__ -D__KERNEL__ -pipe -I "/Users/piet/Projects/NuttX/tiny2040-composite/nuttx/drivers/bch" -I "/Users/piet/Projects/NuttX/tiny2040-composite/nuttx/drivers/crypto" -I "/Users/piet/Projects/NuttX/tiny2040-composite/nuttx/drivers/motor" -I "/Users/piet/Projects/NuttX/tiny2040-composite/nuttx/drivers/loop" -I "/Users/piet/Projects/NuttX/tiny2040-composite/nuttx/drivers/mmcsd" -I "/Users/piet/Projects/NuttX/tiny2040-composite/nuttx/drivers/spi" -I "/Users/piet/Projects/NuttX/tiny2040-composite/nuttx/drivers/usbdev" -I "/Users/piet/Projects/NuttX/tiny2040-composite/nuttx/drivers/usbhost" usbdev/cdcecm.c -o cdcecm.o
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786224832
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
I already replied in an e-mail thread. The root cause is that we are having
```
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) */
};
```
instead of
```
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;
```
As a temporary solution I propose eliminate `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);`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786235577
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
I'm not sure, if that that's the root cause. This eth_hdr_s is okay for my best knowledge.
I see the problem located in driver/usbdev/cdcecm.c in the structure cdcecm_driver_s:
uint8_t config; /* Selected configuration number */
uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
CONFIG_NET_GUARDSIZE];
The uint8_t config following uint8_t pktbuf gets the buffer alligned at an odd address. Changing the eth_hdr_s would not change anything here as there's no reference at all.
Later on pktbuf is assign to an uint8_t pointer: self->dev.d_buf = self->pktbuf;
The first place where eth_hdr_s come into the game is, when it crashed in cdcecm_receive using BUF macro:
#define BUF ((struct eth_hdr_s *)self->dev.d_buf)
There's no way the compiler can get any relation between ptkbuf in cdcecm_driver_s and the way it's used later.
Your proposal: self->dev.d_buf = kmm_zalloc(CONFIG_NET_ETH_PKTSIZE + CONFIG_NET_GUARDSIZE)
seems to be a clean solution, but adds extra effort in code and runtime (error handling) without any value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#issuecomment-1014958870
I did some further investigations and the issue is now at the next place in arp_table.c:
void arp_hdr_update(FAR struct net_driver_s *dev, FAR uint16_t *pipaddr,
FAR uint8_t *ethaddr)
{
in_addr_t ipaddr = net_ip4addr_conv32(pipaddr);
/* Update the ARP table */
Here it’s not an element of a struct, but a pointer to an uint16_t, which gets now the problem.
So the solution with begin_packed_struct/end_packed_struct but it’s not catching all issues with the unaligned d_buf.
Disassembling is not needed.
> Am 17.01.2022 um 23:39 schrieb Petro Karashchenko ***@***.***>:
>
>
> @pkarashchenko commented on this pull request.
>
> In drivers/usbdev/cdcecm.c <https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786318309>:
>
> > @@ -120,7 +120,7 @@ struct cdcecm_driver_s
> FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
> uint8_t config; /* Selected configuration number */
>
> - uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
> + aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
> @ptka <https://github.com/ptka> I believe you and unfortunately I do not have tiny2040-composite board to try it. Is it possible for you to generate disassembly for cdcecm_receive()? I would like to examine what instructions are generated for if (BUF->type == HTONS(ETHTYPE_IP)) so it leads for hard fault.
>
> —
> Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786318309>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGUYCXWHHQO67C4MCTTPLLUWSLBFANCNFSM5MEWZEKA>.
> Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
> You are receiving this because you were mentioned.
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#issuecomment-1014777715
Please fix
```
Error: usbdev/cdcecm.c:2098:23: error: assignment to 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint16_t *' {aka 'short unsigned int *'} [-Werror=incompatible-pointer-types]
2098 | self->dev.d_buf = self->pktbuf;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786222703
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
Could be fixed by moving pktbuf before config?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786298235
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
> Your test program is correctly, for what you want to show, but it's not reflecting the problem. I extended the program to added the missing parts:
>
> #include <stdint.h> #include <stdlib.h> #include <stdio.h>
>
> #define uint8_t unsigned char
>
> #define CONFIG_NET_ETH_PKTSIZE 590 #define CONFIG_NET_GUARDSIZE 2
>
> struct net_driver_s { uint8_t *d_buf; };
>
> struct cdcecm_driver_s { uint8_t config; /* Selected configuration number */
>
> uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE + CONFIG_NET_GUARDSIZE];
>
> struct net_driver_s dev; };
>
> 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) */ };
>
> struct packed_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) */ } **attribute** ((packed));
>
> uint8_t pkt[128];
>
> #define BUF ((struct eth_hdr_s *)pkt) #define BUF_PACKED ((struct packed_eth_hdr_s *)pkt)
>
> int f(void) { if (BUF->type == 0x0008) { return 0; }
>
> return 1; }
>
> int f_packed(void) { if (BUF_PACKED->type == 0x0008) { return 0; }
>
> return 1; }
>
> #define BUF_ORIG ((struct eth_hdr_s *)self->dev.d_buf) #define BUF_ORIG_PACKED ((struct packed_eth_hdr_s *)self->dev.d_buf)
>
> int main(void) { struct cdcecm_driver_s *self = malloc(sizeof(struct cdcecm_driver_s)); printf("self = 0x%08lx\n", (unsigned long)self);
>
> ```
> self->dev.d_buf = self->pktbuf;
>
> printf("self->ptkbuf = 0x%08lx\n", (unsigned long)self->pktbuf);
> printf("self->dev.d_buf = 0x%08lx\n", (unsigned long)self->dev.d_buf);
>
> printf("&(BUF_ORIG->type) = 0x%08lx\n", (unsigned long)&BUF_ORIG->type);
> printf("&(BUF_ORIG_PACKED->type) = 0x%08lx\n", (unsigned long)&BUF_ORIG_PACKED->type);
> ```
>
> }
>
> The interesting part is:
>
> struct cdcecm_driver_s { uint8_t config; /* Selected configuration number */
>
> uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE + CONFIG_NET_GUARDSIZE];
>
> struct net_driver_s dev; };
>
> Due to 'config', pktbuf get's an odd address. I we use pktbuf for an eth_hdr_s structure, there's no way that the definition of eth_hdr_s can prevent, that 'type' doesn't get an odd address, too.
>
> Please output of the program:
>
> self = 0x136e04290 ---> malloc() return a well aligned address for cdcecm_driver_s structure
>
> self->ptkbuf = 0x136e04291 --> pktbuf gets automatically an odd address due to 'config' element in front
>
> self->dev.d_buf = 0x136e04291 --> that's obvious as d_buf is assigned from pktbus
>
> &(BUF_ORIG->type) = 0x136e0429d --> address of type also gets an odd address --> CRASH
>
> &(BUF_ORIG_PACKED->type) = 0x136e0429d --> packed attribute doesn't change anything
Yes. I understand that `pktbuf` get's an odd address and what I want to say that it is ok, because `packed` attribute force byte access instructions to be generated `ldrb` and the `uint16_t type;` value is reassembled from bytes instead of using word instruction `ldrh` that actually cause a crash when odd address is used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786241904
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
@ptka in the original report was stated that problem occurs at `if (BUF->type == HTONS(ETHTYPE_IP))` because Element `type` is always odd aligned and access to it results in all cases to a hard fault on the used rp2040 based board.
Maybe I misunderstood original issue reporting? If so please clarify a place and conditions of the issue that you are trying to fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786260549
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
arm-none-eabi-gcc (GNU Arm Embedded Toolchain 10.3-2021.10) 10.3.1 20210824 (release)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786296146
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
Please see https://github.com/apache/incubator-nuttx/pull/5251
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786257267
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
> ... just tested on my targets ... begin_packed_struct / end_packed_struct doesn't fix the issue.
Are you sure? What compiler do you use?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
ptka commented on pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#issuecomment-1014837041
> Please fix
>
> ```
> Error: usbdev/cdcecm.c:2098:23: error: assignment to 'uint8_t *' {aka 'unsigned char *'} from incompatible pointer type 'uint16_t *' {aka 'short unsigned int *'} [-Werror=incompatible-pointer-types]
> 2098 | self->dev.d_buf = self->pktbuf;
> ```
Fixed ... I'm wondering why this is not reported on the rp2040 builds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#discussion_r786274863
##########
File path: drivers/usbdev/cdcecm.c
##########
@@ -120,7 +120,7 @@ struct cdcecm_driver_s
FAR struct usbdev_ep_s *epbulkout; /* Bulk OUT endpoint */
uint8_t config; /* Selected configuration number */
- uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
+ aligned_data(2) uint8_t pktbuf[CONFIG_NET_ETH_PKTSIZE +
Review comment:
I just created a small example test1.c
```
#include <stdint.h>
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) */
};
struct packed_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) */
} __attribute__ ((packed));
uint8_t pkt[128];
#define BUF ((struct eth_hdr_s *)pkt)
#define BUF_PACKED ((struct packed_eth_hdr_s *)pkt)
int f(void)
{
if (BUF->type == 0x0008)
{
return 0;
}
return 1;
}
int f_packed(void)
{
if (BUF_PACKED->type == 0x0008)
{
return 0;
}
return 1;
}
```
And compiled with same GCC that you are using with `arm-none-eabi-gcc -save-temps -Wall -Wextra -c -mcpu=cortex-m0 -mthumb -mfloat-abi=soft test1.c`. Here is what I got at the end:
```
.cpu cortex-m0
.arch armv6s-m
.fpu softvfp
.eabi_attribute 20, 1
.eabi_attribute 21, 1
.eabi_attribute 23, 3
.eabi_attribute 24, 1
.eabi_attribute 25, 1
.eabi_attribute 26, 1
.eabi_attribute 30, 6
.eabi_attribute 34, 0
.eabi_attribute 18, 4
.file "test1.c"
.text
.global pkt
.bss
.align 2
.type pkt, %object
.size pkt, 128
pkt:
.space 128
.text
.align 1
.global f
.syntax unified
.code 16
.thumb_func
.type f, %function
f:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 1, uses_anonymous_args = 0
push {r7, lr}
add r7, sp, #0
ldr r3, .L4
ldrh r3, [r3, #12]
cmp r3, #8
bne .L2
movs r3, #0
b .L3
.L2:
movs r3, #1
.L3:
movs r0, r3
mov sp, r7
@ sp needed
pop {r7, pc}
.L5:
.align 2
.L4:
.word pkt
.size f, .-f
.align 1
.global f_packed
.syntax unified
.code 16
.thumb_func
.type f_packed, %function
f_packed:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 1, uses_anonymous_args = 0
push {r7, lr}
add r7, sp, #0
ldr r3, .L9
ldrb r2, [r3, #12]
ldrb r3, [r3, #13]
lsls r3, r3, #8
orrs r3, r2
uxth r3, r3
cmp r3, #8
bne .L7
movs r3, #0
b .L8
.L7:
movs r3, #1
.L8:
movs r0, r3
mov sp, r7
@ sp needed
pop {r7, pc}
.L10:
.align 2
.L9:
.word pkt
.size f_packed, .-f_packed
.ident "GCC: (GNU Arm Embedded Toolchain 10.3-2021.10) 10.3.1 20210824 (release)"
```
It's clearly visible that is case of `f()` the `ldrh r3, [r3, #12]` instruction is generated and in case of `f_packed()` we are having `ldrb r2, [r3, #12]` + `ldrb r3, [r3, #13]` + `lsls r3, r3, #8` + `orrs r3, r2`+ `uxth r3, r3` before `cmp r3, #8`. That is exactly the issue that you are having. So if you tried with adding `begin_packed_struct` / `end_packed_struct` you either didn't recompile the code or recompiled, but didn't flash the new version. Please double check and reply.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#issuecomment-1015139932
@xiaoxiang781216 please take a loop. If you approve I think we can merge this change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5249: Fix aligment issue: pktbuf needs to be 16bit aligned
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5249:
URL: https://github.com/apache/incubator-nuttx/pull/5249#issuecomment-1015161690
@ptka thank you very much for time spent on the issue fix and the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org