James Kilts wrote:
> Here are a few patches that correct the memory alignment issues. The first
> file also contains a change from "break" to "return" because the variable
> "stage_1_cfg" is used after the loop and is really just "rtskb->data" which
> is freed using kfree_rtskb() before the break. The change to rtmac_proto.c
> was useful to me while tracking down an alignment issue, but if this last
> change was "accidentally" lost it wouldn't matter. :-)
>
> Syntactically I prefer dereferencing since it's easier to see what the code
> does at a glance (perhaps someone has a preference toward get_unaligned()
> and put_unaligned() instead). But consistency with the rest of the code
> seems to demand memcpy().
All good catches, thanks! I just have a few style remarks below, and I
would like to ask you to properly slit up the patches:
- unaligned access fixes
- break->return fix
- instrumentation enhancement
Moreover, when reposting, make sure that those patches are not
line-wrapped like this one.
>
> - James
>
>
>
> _____________________________
>
>
>
>
> Signed-off-by: James Kilts <jameskilts <at> google's mail service>
Your mail shows up in clear text when you go to public mailing lists, no
need to obfuscate, and I also prefer readable addresses in the git log.
> ---
>
>
> diff -ru rtnet-0.9.11_original/stack/rtcfg/rtcfg_client_event.c
> rtnet-0.9.11/stack/rtcfg/rtcfg_client_event.c
> --- rtnet-0.9.11_original/stack/rtcfg/rtcfg_client_event.c 2009-11-12
> 11:36:56.000000000 +0100
> +++ rtnet-0.9.11/stack/rtcfg/rtcfg_client_event.c 2009-12-10
> 16:07:16.000000000 +0100
> @@ -598,24 +622,24 @@
> if (rtskb->len < sizeof(struct rtcfg_frm_stage_1_cfg) +
> 2*RTCFG_ADDRSIZE_IP) {
> rtdm_mutex_unlock(&rtcfg_dev->dev_mutex);
> RTCFG_DEBUG(1, "RTcfg: received invalid stage_1_cfg "
> "frame\n");
> kfree_rtskb(rtskb);
> - break;
> + return;
> }
>
> rtdev = rtskb->rtdev;
>
> - daddr = *(u32*)stage_1_cfg->client_addr;
> + memcpy(&daddr, stage_1_cfg->client_addr, 4);
> stage_1_cfg = (struct rtcfg_frm_stage_1_cfg *)
> (((u8 *)stage_1_cfg) + RTCFG_ADDRSIZE_IP);
>
> - saddr = *(u32*)stage_1_cfg->server_addr;
> + memcpy(&saddr, stage_1_cfg->server_addr, 4);
> stage_1_cfg = (struct rtcfg_frm_stage_1_cfg *)
> (((u8 *)stage_1_cfg) + RTCFG_ADDRSIZE_IP);
>
> __rtskb_pull(rtskb, 2*RTCFG_ADDRSIZE_IP);
>
> /* Broadcast: IP is used to address client */
> if (rtskb->pkt_type == PACKET_BROADCAST) {
> /* directed to us? */
> @@ -748,6 +773,7 @@
> struct rtcfg_frm_announce *announce_frm;
> struct rtcfg_device *rtcfg_dev = &device[ifindex];
> u32 i;
> + u32 announce_from_addr;
> int result;
>
>
> @@ -771,8 +797,10 @@
> return -EINVAL;
> }
>
> + memcpy(&announce_from_addr, announce_frm->addr, 4);
> +
> /* update routing table */
> - rt_ip_route_add_host(*(u32 *)announce_frm->addr,
> + rt_ip_route_add_host(announce_from_addr,
> rtskb->mac.ethernet->h_source,
> rtskb->rtdev);
>
> announce_frm = (struct rtcfg_frm_announce *)
> @@ -1045,7 +1073,7 @@
> return;
> }
>
> - ip = *(u32 *)dead_station_frm->logical_addr;
> + memcpy(&ip, dead_station_frm->logical_addr, 4);
>
> /* only delete remote IPs from routing table */
> if (rtskb->rtdev->local_ip != ip)
> @@ -1132,11 +1160,11 @@
>
> rtdev = rtskb->rtdev;
>
> - daddr = *(u32*)stage_1_cfg->client_addr;
> + memcpy(&daddr, stage_1_cfg->client_addr, 4);
> stage_1_cfg = (struct rtcfg_frm_stage_1_cfg *)
> (((u8 *)stage_1_cfg) + RTCFG_ADDRSIZE_IP);
>
> - saddr = *(u32*)stage_1_cfg->server_addr;
> + memcpy(&saddr, stage_1_cfg->server_addr, 4);
> stage_1_cfg = (struct rtcfg_frm_stage_1_cfg *)
> (((u8 *)stage_1_cfg) + RTCFG_ADDRSIZE_IP);
>
> diff -ru rtnet-0.9.11_original/stack/rtcfg/rtcfg_event.c
> rtnet-0.9.11/stack/rtcfg/rtcfg_event.c
> --- rtnet-0.9.11_original/stack/rtcfg/rtcfg_event.c 2009-11-12
> 11:36:56.000000000 +0100
> +++ rtnet-0.9.11/stack/rtcfg/rtcfg_event.c 2009-12-10 16:22:28.000000000
> +0100
> @@ -523,7 +532,7 @@
> #ifdef CONFIG_RTNET_RTIPV4
> case RTCFG_ADDR_IP:
> if (((conn->addr_type & RTCFG_ADDR_MASK) == RTCFG_ADDR_IP)
> &&
> - (*(u32 *)announce->addr == conn->addr.ip_addr)) {
> + (memcmp(announce->addr, &(conn->addr.ip_addr), 4) ==
> 0)) {
Better go via a local variable here that should carry announce->addr for
the comparison.
> /* save MAC address - Ethernet-specific! */
> memcpy(conn->mac_addr, rtskb->mac.ethernet->h_source,
> ETH_ALEN);
>
> diff -ru rtnet-0.9.11_original/stack/rtcfg/rtcfg_frame.c
> rtnet-0.9.11/stack/rtcfg/rtcfg_frame.c
> --- rtnet-0.9.11_original/stack/rtcfg/rtcfg_frame.c 2009-11-12
> 11:36:56.000000000 +0100
> +++ rtnet-0.9.11/stack/rtcfg/rtcfg_frame.c 2009-12-10 16:13:04.000000000
> +0100
> @@ -170,12 +170,12 @@
> if (stage_1_frm->addr_type == RTCFG_ADDR_IP) {
> rtskb_put(rtskb, 2*RTCFG_ADDRSIZE_IP);
>
> - *(u32*)stage_1_frm->client_addr = conn->addr.ip_addr;
> + memcpy(stage_1_frm->client_addr, &(conn->addr.ip_addr), 4);
>
> stage_1_frm = (struct rtcfg_frm_stage_1_cfg *)
> (((u8 *)stage_1_frm) + RTCFG_ADDRSIZE_IP);
>
> - *(u32*)stage_1_frm->server_addr = rtdev->local_ip;
> + memcpy(stage_1_frm->server_addr, &(rtdev->local_ip), 4);
>
> stage_1_frm = (struct rtcfg_frm_stage_1_cfg *)
> (((u8 *)stage_1_frm) + RTCFG_ADDRSIZE_IP);
> @@ -332,7 +332,7 @@
> if (announce_new->addr_type == RTCFG_ADDR_IP) {
> rtskb_put(rtskb, RTCFG_ADDRSIZE_IP);
>
> - *(u32*)announce_new->addr = rtdev->local_ip;
> + memcpy(announce_new->addr, &(rtdev->local_ip), 4);
>
> announce_new = (struct rtcfg_frm_announce *)
> (((u8 *)announce_new) + RTCFG_ADDRSIZE_IP);
> @@ -388,7 +388,7 @@
> if (announce_rpl->addr_type == RTCFG_ADDR_IP) {
> rtskb_put(rtskb, RTCFG_ADDRSIZE_IP);
>
> - *(u32*)announce_rpl->addr = rtdev->local_ip;
> + memcpy(announce_rpl->addr, &(rtdev->local_ip), 4);
>
> announce_rpl = (struct rtcfg_frm_announce *)
> (((u8 *)announce_rpl) + RTCFG_ADDRSIZE_IP);
> @@ -512,7 +512,7 @@
> if (dead_station_frm->addr_type == RTCFG_ADDR_IP) {
> rtskb_put(rtskb, RTCFG_ADDRSIZE_IP);
>
> - *(u32*)dead_station_frm->logical_addr = conn->addr.ip_addr;
> + memcpy(dead_station_frm->logical_addr, &(conn->addr.ip_addr), 4);
>
> dead_station_frm = (struct rtcfg_frm_dead_station *)
> (((u8 *)dead_station_frm) + RTCFG_ADDRSIZE_IP);
>
> diff -ru rtnet-0.9.11_original/stack/rtmac/rtmac_proto.c
> rtnet-0.9.11/stack/rtmac/rtmac_proto.c
> --- rtnet-0.9.11_original/stack/rtmac/rtmac_proto.c 2009-11-12
> 11:36:56.000000000 +0100
> +++ rtnet-0.9.11/stack/rtmac/rtmac_proto.c 2009-12-10 12:31:44.000000000
> +0100
> @@ -50,7 +50,8 @@
>
> if (hdr->ver != RTMAC_VERSION) {
> rtdm_printk("RTmac: received unsupported RTmac protocol version on
> "
> - "device %s\n", skb->rtdev->name);
> + "device %s. Got 0x%02x but expected 0x%02x.\n",
> + skb->rtdev->name, hdr->ver, RTMAC_VERSION);
> goto error;
> }
>
>
>
Thanks again,
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
------------------------------------------------------------------------------
Return on Information:
Google Enterprise Search pays you back
Get the facts.
http://p.sf.net/sfu/google-dev2dev
_______________________________________________
RTnet-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/rtnet-developers
