| Message ID | 20230825153111.228768-1-lukma@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hsr : Provide fix for HSRv1 supervisor frames decoding | expand |
> - /* And leave the HSR tag. */ > + * And leave the HSR tag. > + * > + * The HSRv1 supervisory frame encapsulates the v0 frame > + * with EtherType of 0x88FB > + */ > if (ethhdr->h_proto == htons(ETH_P_HSR)) { > - pull_size = sizeof(struct ethhdr); > + if (hsr->prot_version == HSR_V1) > + /* In the above step the DA, SA and EtherType > + * (0x892F - HSRv1) bytes has been removed. > + * > + * As the HSRv1 has the HSR header added, one need > + * to remove path_and_LSDU_size and sequence_nr fields. > + * > + */ > + pull_size = 4; > + else > + pull_size = sizeof(struct hsr_tag); > + > skb_pull(skb, pull_size); > total_pull_size += pull_size; > } > @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame) > total_pull_size += pull_size; > > /* get HSR sup payload */ > + if (hsr->prot_version == HSR_V1) { > + /* In the HSRv1 supervisor frame, when > + * one with EtherType = 0x88FB is extracted, the Node A > + * MAC address is preceded with type and length elements of TLV > + * data field. > + * > + * It needs to be removed to get the remote peer MAC address. > + */ > + pull_size = sizeof(struct hsr_sup_tlv); > + skb_pull(skb, pull_size); > + total_pull_size += pull_size; > + } > + > hsr_sp = (struct hsr_sup_payload *)skb->data; I thought the fix is simply this: if (ethhdr->h_proto == htons(ETH_P_HSR)) { - pull_size = sizeof(struct ethhdr); + pull_size = sizeof(struct hsr_tag); skb_pull(skb, pull_size); total_pull_size += pull_size; } - pull_size = sizeof(struct hsr_tag); + pull_size = sizeof(struct hsr_sup_tag); Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes. The code in 5.15 before this refactored code uses those structures. When using v0 the EtherType uses the PRP tag instead of the HSR tag so the HSR related code is not executed.
Hi Lukasz, kernel test robot noticed the following build errors: [auto build test ERROR on net-next/main] [also build test ERROR on net/main linus/master v6.5-rc7 next-20230825] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Lukasz-Majewski/net-hsr-Provide-fix-for-HSRv1-supervisor-frames-decoding/20230825-233423 base: net-next/main patch link: https://lore.kernel.org/r/20230825153111.228768-1-lukma%40denx.de patch subject: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding config: powerpc64-randconfig-r022-20230826 (https://download.01.org/0day-ci/archive/20230826/202308260733.G7tU8UHx-lkp@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce: (https://download.01.org/0day-ci/archive/20230826/202308260733.G7tU8UHx-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308260733.G7tU8UHx-lkp@intel.com/ All errors (new ones prefixed by >>): >> net/hsr/hsr_framereg.c:289:8: error: expected ';' after expression 289 | * And leave the HSR tag. | ^ | ; >> net/hsr/hsr_framereg.c:289:5: error: use of undeclared identifier 'And' 289 | * And leave the HSR tag. | ^ >> net/hsr/hsr_framereg.c:289:9: error: use of undeclared identifier 'leave' 289 | * And leave the HSR tag. | ^ 3 errors generated. vim +289 net/hsr/hsr_framereg.c 249 250 /* Use the Supervision frame's info about an eventual macaddress_B for merging 251 * nodes that has previously had their macaddress_B registered as a separate 252 * node. 253 */ 254 void hsr_handle_sup_frame(struct hsr_frame_info *frame) 255 { 256 struct hsr_node *node_curr = frame->node_src; 257 struct hsr_port *port_rcv = frame->port_rcv; 258 struct hsr_priv *hsr = port_rcv->hsr; 259 struct hsr_sup_payload *hsr_sp; 260 struct hsr_sup_tlv *hsr_sup_tlv; 261 struct hsr_node *node_real; 262 struct sk_buff *skb = NULL; 263 struct list_head *node_db; 264 struct ethhdr *ethhdr; 265 int i; 266 unsigned int pull_size = 0; 267 unsigned int total_pull_size = 0; 268 269 /* Here either frame->skb_hsr or frame->skb_prp should be 270 * valid as supervision frame always will have protocol 271 * header info. 272 */ 273 if (frame->skb_hsr) 274 skb = frame->skb_hsr; 275 else if (frame->skb_prp) 276 skb = frame->skb_prp; 277 else if (frame->skb_std) 278 skb = frame->skb_std; 279 if (!skb) 280 return; 281 282 /* Leave the ethernet header. */ 283 pull_size = sizeof(struct ethhdr); 284 skb_pull(skb, pull_size); 285 total_pull_size += pull_size; 286 287 ethhdr = (struct ethhdr *)skb_mac_header(skb); 288 > 289 * And leave the HSR tag. 290 * 291 * The HSRv1 supervisory frame encapsulates the v0 frame 292 * with EtherType of 0x88FB 293 */ 294 if (ethhdr->h_proto == htons(ETH_P_HSR)) { 295 if (hsr->prot_version == HSR_V1) 296 /* In the above step the DA, SA and EtherType 297 * (0x892F - HSRv1) bytes has been removed. 298 * 299 * As the HSRv1 has the HSR header added, one need 300 * to remove path_and_LSDU_size and sequence_nr fields. 301 * 302 */ 303 pull_size = 4; 304 else 305 pull_size = sizeof(struct hsr_tag); 306 307 skb_pull(skb, pull_size); 308 total_pull_size += pull_size; 309 } 310 311 /* And leave the HSR sup tag. */ 312 pull_size = sizeof(struct hsr_tag); 313 skb_pull(skb, pull_size); 314 total_pull_size += pull_size; 315 316 /* get HSR sup payload */ 317 if (hsr->prot_version == HSR_V1) { 318 /* In the HSRv1 supervisor frame, when 319 * one with EtherType = 0x88FB is extracted, the Node A 320 * MAC address is preceded with type and length elements of TLV 321 * data field. 322 * 323 * It needs to be removed to get the remote peer MAC address. 324 */ 325 pull_size = sizeof(struct hsr_sup_tlv); 326 skb_pull(skb, pull_size); 327 total_pull_size += pull_size; 328 } 329 330 hsr_sp = (struct hsr_sup_payload *)skb->data; 331 332 /* Merge node_curr (registered on macaddress_B) into node_real */ 333 node_db = &port_rcv->hsr->node_db; 334 node_real = find_node_by_addr_A(node_db, hsr_sp->macaddress_A); 335 if (!node_real) 336 /* No frame received from AddrA of this node yet */ 337 node_real = hsr_add_node(hsr, node_db, hsr_sp->macaddress_A, 338 HSR_SEQNR_START - 1, true, 339 port_rcv->type); 340 if (!node_real) 341 goto done; /* No mem */ 342 if (node_real == node_curr) 343 /* Node has already been merged */ 344 goto done; 345 346 /* Leave the first HSR sup payload. */ 347 pull_size = sizeof(struct hsr_sup_payload); 348 skb_pull(skb, pull_size); 349 total_pull_size += pull_size; 350 351 /* Get second supervision tlv */ 352 hsr_sup_tlv = (struct hsr_sup_tlv *)skb->data; 353 /* And check if it is a redbox mac TLV */ 354 if (hsr_sup_tlv->HSR_TLV_type == PRP_TLV_REDBOX_MAC) { 355 /* We could stop here after pushing hsr_sup_payload, 356 * or proceed and allow macaddress_B and for redboxes. 357 */ 358 /* Sanity check length */ 359 if (hsr_sup_tlv->HSR_TLV_length != 6) 360 goto done; 361 362 /* Leave the second HSR sup tlv. */ 363 pull_size = sizeof(struct hsr_sup_tlv); 364 skb_pull(skb, pull_size); 365 total_pull_size += pull_size; 366 367 /* Get redbox mac address. */ 368 hsr_sp = (struct hsr_sup_payload *)skb->data; 369 370 /* Check if redbox mac and node mac are equal. */ 371 if (!ether_addr_equal(node_real->macaddress_A, hsr_sp->macaddress_A)) { 372 /* This is a redbox supervision frame for a VDAN! */ 373 goto done; 374 } 375 } 376 377 ether_addr_copy(node_real->macaddress_B, ethhdr->h_source); 378 spin_lock_bh(&node_real->seq_out_lock); 379 for (i = 0; i < HSR_PT_PORTS; i++) { 380 if (!node_curr->time_in_stale[i] && 381 time_after(node_curr->time_in[i], node_real->time_in[i])) { 382 node_real->time_in[i] = node_curr->time_in[i]; 383 node_real->time_in_stale[i] = 384 node_curr->time_in_stale[i]; 385 } 386 if (seq_nr_after(node_curr->seq_out[i], node_real->seq_out[i])) 387 node_real->seq_out[i] = node_curr->seq_out[i]; 388 } 389 spin_unlock_bh(&node_real->seq_out_lock); 390 node_real->addr_B_port = port_rcv->type; 391 392 spin_lock_bh(&hsr->list_lock); 393 if (!node_curr->removed) { 394 list_del_rcu(&node_curr->mac_list); 395 node_curr->removed = true; 396 kfree_rcu(node_curr, rcu_head); 397 } 398 spin_unlock_bh(&hsr->list_lock); 399 400 done: 401 /* Push back here */ 402 skb_push(skb, total_pull_size); 403 } 404
Hi Lukasz, kernel test robot noticed the following build errors: [auto build test ERROR on net-next/main] [also build test ERROR on net/main linus/master v6.5-rc7 next-20230825] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Lukasz-Majewski/net-hsr-Provide-fix-for-HSRv1-supervisor-frames-decoding/20230825-233423 base: net-next/main patch link: https://lore.kernel.org/r/20230825153111.228768-1-lukma%40denx.de patch subject: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding config: riscv-randconfig-001-20230826 (https://download.01.org/0day-ci/archive/20230826/202308260833.erhVKBnc-lkp@intel.com/config) compiler: riscv64-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230826/202308260833.erhVKBnc-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308260833.erhVKBnc-lkp@intel.com/ All errors (new ones prefixed by >>): net/hsr/hsr_framereg.c: In function 'hsr_handle_sup_frame': >> net/hsr/hsr_framereg.c:289:12: error: 'And' undeclared (first use in this function) 289 | * And leave the HSR tag. | ^~~ net/hsr/hsr_framereg.c:289:12: note: each undeclared identifier is reported only once for each function it appears in >> net/hsr/hsr_framereg.c:289:15: error: expected ';' before 'leave' 289 | * And leave the HSR tag. | ^~~~~~ | ; vim +/And +289 net/hsr/hsr_framereg.c 249 250 /* Use the Supervision frame's info about an eventual macaddress_B for merging 251 * nodes that has previously had their macaddress_B registered as a separate 252 * node. 253 */ 254 void hsr_handle_sup_frame(struct hsr_frame_info *frame) 255 { 256 struct hsr_node *node_curr = frame->node_src; 257 struct hsr_port *port_rcv = frame->port_rcv; 258 struct hsr_priv *hsr = port_rcv->hsr; 259 struct hsr_sup_payload *hsr_sp; 260 struct hsr_sup_tlv *hsr_sup_tlv; 261 struct hsr_node *node_real; 262 struct sk_buff *skb = NULL; 263 struct list_head *node_db; 264 struct ethhdr *ethhdr; 265 int i; 266 unsigned int pull_size = 0; 267 unsigned int total_pull_size = 0; 268 269 /* Here either frame->skb_hsr or frame->skb_prp should be 270 * valid as supervision frame always will have protocol 271 * header info. 272 */ 273 if (frame->skb_hsr) 274 skb = frame->skb_hsr; 275 else if (frame->skb_prp) 276 skb = frame->skb_prp; 277 else if (frame->skb_std) 278 skb = frame->skb_std; 279 if (!skb) 280 return; 281 282 /* Leave the ethernet header. */ 283 pull_size = sizeof(struct ethhdr); 284 skb_pull(skb, pull_size); 285 total_pull_size += pull_size; 286 287 ethhdr = (struct ethhdr *)skb_mac_header(skb); 288 > 289 * And leave the HSR tag. 290 * 291 * The HSRv1 supervisory frame encapsulates the v0 frame 292 * with EtherType of 0x88FB 293 */ 294 if (ethhdr->h_proto == htons(ETH_P_HSR)) { 295 if (hsr->prot_version == HSR_V1) 296 /* In the above step the DA, SA and EtherType 297 * (0x892F - HSRv1) bytes has been removed. 298 * 299 * As the HSRv1 has the HSR header added, one need 300 * to remove path_and_LSDU_size and sequence_nr fields. 301 * 302 */ 303 pull_size = 4; 304 else 305 pull_size = sizeof(struct hsr_tag); 306 307 skb_pull(skb, pull_size); 308 total_pull_size += pull_size; 309 } 310 311 /* And leave the HSR sup tag. */ 312 pull_size = sizeof(struct hsr_tag); 313 skb_pull(skb, pull_size); 314 total_pull_size += pull_size; 315 316 /* get HSR sup payload */ 317 if (hsr->prot_version == HSR_V1) { 318 /* In the HSRv1 supervisor frame, when 319 * one with EtherType = 0x88FB is extracted, the Node A 320 * MAC address is preceded with type and length elements of TLV 321 * data field. 322 * 323 * It needs to be removed to get the remote peer MAC address. 324 */ 325 pull_size = sizeof(struct hsr_sup_tlv); 326 skb_pull(skb, pull_size); 327 total_pull_size += pull_size; 328 } 329 330 hsr_sp = (struct hsr_sup_payload *)skb->data; 331 332 /* Merge node_curr (registered on macaddress_B) into node_real */ 333 node_db = &port_rcv->hsr->node_db; 334 node_real = find_node_by_addr_A(node_db, hsr_sp->macaddress_A); 335 if (!node_real) 336 /* No frame received from AddrA of this node yet */ 337 node_real = hsr_add_node(hsr, node_db, hsr_sp->macaddress_A, 338 HSR_SEQNR_START - 1, true, 339 port_rcv->type); 340 if (!node_real) 341 goto done; /* No mem */ 342 if (node_real == node_curr) 343 /* Node has already been merged */ 344 goto done; 345 346 /* Leave the first HSR sup payload. */ 347 pull_size = sizeof(struct hsr_sup_payload); 348 skb_pull(skb, pull_size); 349 total_pull_size += pull_size; 350 351 /* Get second supervision tlv */ 352 hsr_sup_tlv = (struct hsr_sup_tlv *)skb->data; 353 /* And check if it is a redbox mac TLV */ 354 if (hsr_sup_tlv->HSR_TLV_type == PRP_TLV_REDBOX_MAC) { 355 /* We could stop here after pushing hsr_sup_payload, 356 * or proceed and allow macaddress_B and for redboxes. 357 */ 358 /* Sanity check length */ 359 if (hsr_sup_tlv->HSR_TLV_length != 6) 360 goto done; 361 362 /* Leave the second HSR sup tlv. */ 363 pull_size = sizeof(struct hsr_sup_tlv); 364 skb_pull(skb, pull_size); 365 total_pull_size += pull_size; 366 367 /* Get redbox mac address. */ 368 hsr_sp = (struct hsr_sup_payload *)skb->data; 369 370 /* Check if redbox mac and node mac are equal. */ 371 if (!ether_addr_equal(node_real->macaddress_A, hsr_sp->macaddress_A)) { 372 /* This is a redbox supervision frame for a VDAN! */ 373 goto done; 374 } 375 } 376 377 ether_addr_copy(node_real->macaddress_B, ethhdr->h_source); 378 spin_lock_bh(&node_real->seq_out_lock); 379 for (i = 0; i < HSR_PT_PORTS; i++) { 380 if (!node_curr->time_in_stale[i] && 381 time_after(node_curr->time_in[i], node_real->time_in[i])) { 382 node_real->time_in[i] = node_curr->time_in[i]; 383 node_real->time_in_stale[i] = 384 node_curr->time_in_stale[i]; 385 } 386 if (seq_nr_after(node_curr->seq_out[i], node_real->seq_out[i])) 387 node_real->seq_out[i] = node_curr->seq_out[i]; 388 } 389 spin_unlock_bh(&node_real->seq_out_lock); 390 node_real->addr_B_port = port_rcv->type; 391 392 spin_lock_bh(&hsr->list_lock); 393 if (!node_curr->removed) { 394 list_del_rcu(&node_curr->mac_list); 395 node_curr->removed = true; 396 kfree_rcu(node_curr, rcu_head); 397 } 398 spin_unlock_bh(&hsr->list_lock); 399 400 done: 401 /* Push back here */ 402 skb_push(skb, total_pull_size); 403 } 404
Hi Tristram, > > - /* And leave the HSR tag. */ > > + * And leave the HSR tag. > > + * > > + * The HSRv1 supervisory frame encapsulates the v0 frame > > + * with EtherType of 0x88FB > > + */ > > if (ethhdr->h_proto == htons(ETH_P_HSR)) { > > - pull_size = sizeof(struct ethhdr); > > + if (hsr->prot_version == HSR_V1) > > + /* In the above step the DA, SA and > > EtherType > > + * (0x892F - HSRv1) bytes has been removed. > > + * > > + * As the HSRv1 has the HSR header added, > > one need > > + * to remove path_and_LSDU_size and > > sequence_nr fields. > > + * > > + */ > > + pull_size = 4; > > + else > > + pull_size = sizeof(struct hsr_tag); > > + > > skb_pull(skb, pull_size); > > total_pull_size += pull_size; > > } > > @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct > > hsr_frame_info *frame) total_pull_size += pull_size; > > > > /* get HSR sup payload */ > > + if (hsr->prot_version == HSR_V1) { > > + /* In the HSRv1 supervisor frame, when > > + * one with EtherType = 0x88FB is extracted, the > > Node A > > + * MAC address is preceded with type and length > > elements of TLV > > + * data field. > > + * > > + * It needs to be removed to get the remote peer > > MAC address. > > + */ > > + pull_size = sizeof(struct hsr_sup_tlv); > > + skb_pull(skb, pull_size); > > + total_pull_size += pull_size; > > + } > > + > > hsr_sp = (struct hsr_sup_payload *)skb->data; > > I thought the fix is simply this: > > if (ethhdr->h_proto == htons(ETH_P_HSR)) { > - pull_size = sizeof(struct ethhdr); > + pull_size = sizeof(struct hsr_tag); > skb_pull(skb, pull_size); > total_pull_size += pull_size; > } > > - pull_size = sizeof(struct hsr_tag); > + pull_size = sizeof(struct hsr_sup_tag); > > Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes. > The code in 5.15 before this refactored code uses those structures. > When using v0 the EtherType uses the PRP tag instead of the HSR tag so > the HSR related code is not executed. > This would not be enough it seems. Please find below skb->data dump when entering hsr_handle_sup_frame() [0]: SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34 SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 SKB_I100000040: 00 00 With the newest kernel (before applying this patch) in [1] we do remove: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f (which is equal to sizeof(struct ethhdr) = 6 + 6 + 2 B = 14 B) So we do have: 00 34 SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 SKB_I100000040: 00 00 And we need to remove rest of the HSR v1 tag (4 Bytes). Then we do have: SKB_I100000010: 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 SKB_I100000040: 00 00 The 0x88FB is the PRP/HSRv0 supervisory frame ETH type, so the tag needs to be removed (6 Bytes) and then we do have TYPE (0x17) and Length (0x06), which indicate the other HSR host IP address. When I do apply your proposed changes we would have the DA and SA MAC addresses removed implicitly (as the struct hsr_tag and hsr_sup_tag are 6 bytes in size) and we end up with frame starting with HSR v1 tag - i.e.: SKB_I100000000: 89 2f 00 34 SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 SKB_I100000040: 00 00 Hence mine question - is my setup or understanding wrong (as the PRP supervisory frame is encapsulated in HSR v1 frame)? I do use the same kernel on two KSZ9477-EVB boards with Port[12] connected together to work with HSR. I also to explicitly force the HSR driver to use v1 of HSR (by default v0 is enforced). If you don't mind - I would also like to ask a question regarding the node_db for HSR. Why the output of: # cat /sys/kernel/debug/hsr/hsr0/node_table Node Table entries for (HSR) device MAC-Address-A, MAC-Address-B, time_in[A], time_in[B], 00:10:a1:94:77:30 00:00:00:00:00:00 1689193, 1689199, Address-B port, DAN-H 0, 1 Has the MAC-Address-B equal to 00:00:00:00:00:00 ? As I do have the same MAC addresses for both HSR ports (to facilitate frame duplication in KSZ9477 IC removal) I would expect to have this MAC address set to 00:10:a1:94:77:30 as well... Is this expected? Or is there any other issue to fix? Thanks in advance for your help and support :-) Links: [0] - https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L281 [1] - https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L290 Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Tristram, > Hi Tristram, > > > > - /* And leave the HSR tag. */ > > > + * And leave the HSR tag. > > > + * > > > + * The HSRv1 supervisory frame encapsulates the v0 frame > > > + * with EtherType of 0x88FB > > > + */ > > > if (ethhdr->h_proto == htons(ETH_P_HSR)) { > > > - pull_size = sizeof(struct ethhdr); > > > + if (hsr->prot_version == HSR_V1) > > > + /* In the above step the DA, SA and > > > EtherType > > > + * (0x892F - HSRv1) bytes has been > > > removed. > > > + * > > > + * As the HSRv1 has the HSR header added, > > > one need > > > + * to remove path_and_LSDU_size and > > > sequence_nr fields. > > > + * > > > + */ > > > + pull_size = 4; > > > + else > > > + pull_size = sizeof(struct hsr_tag); > > > + > > > skb_pull(skb, pull_size); > > > total_pull_size += pull_size; > > > } > > > @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct > > > hsr_frame_info *frame) total_pull_size += pull_size; > > > > > > /* get HSR sup payload */ > > > + if (hsr->prot_version == HSR_V1) { > > > + /* In the HSRv1 supervisor frame, when > > > + * one with EtherType = 0x88FB is extracted, the > > > Node A > > > + * MAC address is preceded with type and length > > > elements of TLV > > > + * data field. > > > + * > > > + * It needs to be removed to get the remote peer > > > MAC address. > > > + */ > > > + pull_size = sizeof(struct hsr_sup_tlv); > > > + skb_pull(skb, pull_size); > > > + total_pull_size += pull_size; > > > + } > > > + > > > hsr_sp = (struct hsr_sup_payload *)skb->data; > > > > I thought the fix is simply this: > > > > if (ethhdr->h_proto == htons(ETH_P_HSR)) { > > - pull_size = sizeof(struct ethhdr); > > + pull_size = sizeof(struct hsr_tag); > > skb_pull(skb, pull_size); > > total_pull_size += pull_size; > > } > > > > - pull_size = sizeof(struct hsr_tag); > > + pull_size = sizeof(struct hsr_sup_tag); > > > > Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes. > > The code in 5.15 before this refactored code uses those structures. > > When using v0 the EtherType uses the PRP tag instead of the HSR tag > > so the HSR related code is not executed. > > > > This would not be enough it seems. Please find below skb->data dump > when entering hsr_handle_sup_frame() [0]: > > SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34 > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000040: 00 00 > > With the newest kernel (before applying this patch) in [1] we do > remove: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f (which is equal to > sizeof(struct ethhdr) = 6 + 6 + 2 B = 14 B) > > So we do have: > > 00 34 > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000040: 00 00 > > And we need to remove rest of the HSR v1 tag (4 Bytes). > > Then we do have: > > SKB_I100000010: 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000040: 00 00 > > The 0x88FB is the PRP/HSRv0 supervisory frame ETH type, so the tag > needs to be removed (6 Bytes) and then we do have TYPE (0x17) and > Length (0x06), which indicate the other HSR host IP address. > > When I do apply your proposed changes we would have the DA and SA > MAC addresses removed implicitly (as the struct hsr_tag and > hsr_sup_tag are 6 bytes in size) and we end up with frame starting > with HSR v1 tag - i.e.: > > SKB_I100000000: 89 2f 00 34 > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000040: 00 00 > > > Hence mine question - is my setup or understanding wrong (as the PRP > supervisory frame is encapsulated in HSR v1 frame)? > > I do use the same kernel on two KSZ9477-EVB boards with Port[12] > connected together to work with HSR. I also to explicitly force the > HSR driver to use v1 of HSR (by default v0 is enforced). > > > > > > If you don't mind - I would also like to ask a question regarding the > node_db for HSR. > > Why the output of: > > # cat /sys/kernel/debug/hsr/hsr0/node_table > Node Table entries for (HSR) device > MAC-Address-A, MAC-Address-B, time_in[A], time_in[B], > 00:10:a1:94:77:30 00:00:00:00:00:00 1689193, 1689199, > > Address-B port, DAN-H > 0, 1 > > Has the MAC-Address-B equal to 00:00:00:00:00:00 ? > > As I do have the same MAC addresses for both HSR ports (to facilitate > frame duplication in KSZ9477 IC removal) I would expect to have this > MAC address set to 00:10:a1:94:77:30 as well... > > Is this expected? Or is there any other issue to fix? > Tristram, do you have any feedback on those changes? > > Thanks in advance for your help and support :-) > > Links: > > [0] - > https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L281 > > [1] - > https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L290 > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > lukma@denx.de Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Tristram, > Hi Tristram, > > > > - /* And leave the HSR tag. */ > > > + * And leave the HSR tag. > > > + * > > > + * The HSRv1 supervisory frame encapsulates the v0 frame > > > + * with EtherType of 0x88FB > > > + */ > > > if (ethhdr->h_proto == htons(ETH_P_HSR)) { > > > - pull_size = sizeof(struct ethhdr); > > > + if (hsr->prot_version == HSR_V1) > > > + /* In the above step the DA, SA and > > > EtherType > > > + * (0x892F - HSRv1) bytes has been > > > removed. > > > + * > > > + * As the HSRv1 has the HSR header added, > > > one need > > > + * to remove path_and_LSDU_size and > > > sequence_nr fields. > > > + * > > > + */ > > > + pull_size = 4; > > > + else > > > + pull_size = sizeof(struct hsr_tag); > > > + > > > skb_pull(skb, pull_size); > > > total_pull_size += pull_size; > > > } > > > @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct > > > hsr_frame_info *frame) total_pull_size += pull_size; > > > > > > /* get HSR sup payload */ > > > + if (hsr->prot_version == HSR_V1) { > > > + /* In the HSRv1 supervisor frame, when > > > + * one with EtherType = 0x88FB is extracted, the > > > Node A > > > + * MAC address is preceded with type and length > > > elements of TLV > > > + * data field. > > > + * > > > + * It needs to be removed to get the remote peer > > > MAC address. > > > + */ > > > + pull_size = sizeof(struct hsr_sup_tlv); > > > + skb_pull(skb, pull_size); > > > + total_pull_size += pull_size; > > > + } > > > + > > > hsr_sp = (struct hsr_sup_payload *)skb->data; > > > > I thought the fix is simply this: > > > > if (ethhdr->h_proto == htons(ETH_P_HSR)) { > > - pull_size = sizeof(struct ethhdr); > > + pull_size = sizeof(struct hsr_tag); > > skb_pull(skb, pull_size); > > total_pull_size += pull_size; > > } > > > > - pull_size = sizeof(struct hsr_tag); > > + pull_size = sizeof(struct hsr_sup_tag); > > > > Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes. > > The code in 5.15 before this refactored code uses those structures. > > When using v0 the EtherType uses the PRP tag instead of the HSR tag > > so the HSR related code is not executed. > > > > This would not be enough it seems. Please find below skb->data dump > when entering hsr_handle_sup_frame() [0]: > > SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34 > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000040: 00 00 > > With the newest kernel (before applying this patch) in [1] we do > remove: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f (which is equal to > sizeof(struct ethhdr) = 6 + 6 + 2 B = 14 B) > > So we do have: > > 00 34 > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000040: 00 00 > > And we need to remove rest of the HSR v1 tag (4 Bytes). > > Then we do have: > > SKB_I100000010: 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000040: 00 00 > > The 0x88FB is the PRP/HSRv0 supervisory frame ETH type, so the tag > needs to be removed (6 Bytes) and then we do have TYPE (0x17) and > Length (0x06), which indicate the other HSR host IP address. > > When I do apply your proposed changes we would have the DA and SA > MAC addresses removed implicitly (as the struct hsr_tag and > hsr_sup_tag are 6 bytes in size) and we end up with frame starting > with HSR v1 tag - i.e.: > > SKB_I100000000: 89 2f 00 34 > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000040: 00 00 > > > Hence mine question - is my setup or understanding wrong (as the PRP > supervisory frame is encapsulated in HSR v1 frame)? > > I do use the same kernel on two KSZ9477-EVB boards with Port[12] > connected together to work with HSR. I also to explicitly force the > HSR driver to use v1 of HSR (by default v0 is enforced). > > As I've double checked with tshark - the supervision frame format is different for HSR v0 and v1: HSR v0: [Protocols in frame:eth:ethertype:hsr_prp_supervision] HSR v1: [Protocols in frame: eth:ethertype:hsr:hsr_prp_supervision] In v0 the ":hsr:" is missing, hence the error. > > > > If you don't mind - I would also like to ask a question regarding the > node_db for HSR. > > Why the output of: > > # cat /sys/kernel/debug/hsr/hsr0/node_table > Node Table entries for (HSR) device > MAC-Address-A, MAC-Address-B, time_in[A], time_in[B], > 00:10:a1:94:77:30 00:00:00:00:00:00 1689193, 1689199, > > Address-B port, DAN-H > 0, 1 > > Has the MAC-Address-B equal to 00:00:00:00:00:00 ? > > As I do have the same MAC addresses for both HSR ports (to facilitate > frame duplication in KSZ9477 IC removal) I would expect to have this > MAC address set to 00:10:a1:94:77:30 as well... > > Is this expected? Or is there any other issue to fix? I think that this is caused by how KSZ9477 works with HSR core. The HSR core is responsible for setting tag for frame. It sets the "Lane ID" part of HSR tag to 0 as it relies on HSR frames duplication in KSZ9477: Type: High-availability Seamless Redundancy (IEC62439 Part 3) (0x892f) High-availability Seamless Redundancy (IEC62439 Part 3 Chapter 5) 0000 .... .... .... = Path: 0 000. .... .... .... = Network id: 0 ...0 .... .... .... = Lane id: Lane A (0) .... 0000 0011 0100 = LSDU size: 52 [correct] As KSZ9477 duplicates (clones) frames without any modification, only frames for Lane id = 0 are sent. Hence the MAC-Address-B is always equal to 00:00:00:00:00:00 as no supervisory frames are received with Lane B id. This however, may be different in other HSR switch IC's - especially in those, which insert the HSR header to frames - as they know to which HSR "lane" (egress port) the frame is delivered. > > > Thanks in advance for your help and support :-) > > Links: > > [0] - > https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L281 > > [1] - > https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L290 > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > lukma@denx.de Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 2023-08-25 17:31:11 [+0200], Lukasz Majewski wrote: > Provide fix to decode correctly supervisory frames when HSRv1 version of > the HSR protocol is used. > > Without this patch console is polluted with: > ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node > > as a result of destination node's A MAC address equals to: > 00:00:00:00:00:00. > > cat /sys/kernel/debug/hsr/hsr0/node_table > Node Table entries for (HSR) device > MAC-Address-A, MAC-Address-B, time_in[A], time_in[B], Address-B > 00:00:00:00:00:00 00:10:a1:94:77:30 400bf, 399c, 0 > > It was caused by wrong frames decoding in the hsr_handle_sup_frame(). > > As the supervisor frame is encapsulated in HSRv1 frame: > > SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34 > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > SKB_I100000040: 00 00 > > The code had to be adjusted accordingly and the MAC-Address-A now > has the proper address (the MAC-Address-B now has all 0's). Was this broken by commit eafaa88b3eb7f ("net: hsr: Add support for redbox supervision frames") ? Is this frame somehow special? I don't remember this… > Signed-off-by: Lukasz Majewski <lukma@denx.de> Sebastian
Hi Sebastian, > On 2023-08-25 17:31:11 [+0200], Lukasz Majewski wrote: > > Provide fix to decode correctly supervisory frames when HSRv1 > > version of the HSR protocol is used. > > > > Without this patch console is polluted with: > > ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node > > > > as a result of destination node's A MAC address equals to: > > 00:00:00:00:00:00. > > > > cat /sys/kernel/debug/hsr/hsr0/node_table > > Node Table entries for (HSR) device > > MAC-Address-A, MAC-Address-B, time_in[A], time_in[B], > > Address-B 00:00:00:00:00:00 00:10:a1:94:77:30 400bf, > > 399c, 0 > > > > It was caused by wrong frames decoding in the > > hsr_handle_sup_frame(). > > > > As the supervisor frame is encapsulated in HSRv1 frame: > > > > SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34 > > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > SKB_I100000040: 00 00 > > > > The code had to be adjusted accordingly and the MAC-Address-A now > > has the proper address (the MAC-Address-B now has all 0's). > > Was this broken by commit > eafaa88b3eb7f ("net: hsr: Add support for redbox supervision > frames") > Yes, it seems so. > ? Is this frame somehow special? I don't remember this… > Please refer to the whole thread - I've described this issue thoroughly (including hex dump of frames): https://lore.kernel.org/lkml/20230904175419.7bed196b@wsk/T/#m35cbfa4f1b8901d341fbc39659ace6a041f84c98 In short - the HSRv1 is not recognized correctly anymore: HSR v0: [Protocols in frame: eth:ethertype:hsr_prp_supervision] HSR v1: [Protocols in frame: eth:ethertype:hsr:hsr_prp_supervision] > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > Sebastian Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Sebastian, > Hi Sebastian, > > > On 2023-08-25 17:31:11 [+0200], Lukasz Majewski wrote: > > > Provide fix to decode correctly supervisory frames when HSRv1 > > > version of the HSR protocol is used. > > > > > > Without this patch console is polluted with: > > > ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node > > > > > > as a result of destination node's A MAC address equals to: > > > 00:00:00:00:00:00. > > > > > > cat /sys/kernel/debug/hsr/hsr0/node_table > > > Node Table entries for (HSR) device > > > MAC-Address-A, MAC-Address-B, time_in[A], time_in[B], > > > Address-B 00:00:00:00:00:00 00:10:a1:94:77:30 400bf, > > > 399c, 0 > > > > > > It was caused by wrong frames decoding in the > > > hsr_handle_sup_frame(). > > > > > > As the supervisor frame is encapsulated in HSRv1 frame: > > > > > > SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34 > > > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 > > > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > > SKB_I100000040: 00 00 > > > > > > The code had to be adjusted accordingly and the MAC-Address-A now > > > has the proper address (the MAC-Address-B now has all 0's). > > > > Was this broken by commit > > eafaa88b3eb7f ("net: hsr: Add support for redbox supervision > > frames") > > > > Yes, it seems so. > > > ? Is this frame somehow special? I don't remember this… > > > > Please refer to the whole thread - I've described this issue > thoroughly (including hex dump of frames): > https://lore.kernel.org/lkml/20230904175419.7bed196b@wsk/T/#m35cbfa4f1b8901d341fbc39659ace6a041f84c98 > > In short - the HSRv1 is not recognized correctly anymore: > > HSR v0: > [Protocols in frame: eth:ethertype:hsr_prp_supervision] > > HSR v1: > [Protocols in frame: eth:ethertype:hsr:hsr_prp_supervision] > Have you had time to review this patch? Your comments are more than welcome. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > > > Sebastian > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > lukma@denx.de Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 2023-09-11 16:57:08 [+0200], Lukasz Majewski wrote: > Hi Sebastian, Hi, > > Have you had time to review this patch? got distracted a few times. I need a quiet moment… Will do this week… > Your comments are more than welcome. Sebastian
Hi Sebastian, > On 2023-09-11 16:57:08 [+0200], Lukasz Majewski wrote: > > Hi Sebastian, > Hi, > > > > > Have you had time to review this patch? > > got distracted a few times. I need a quiet moment… Will do this week… > Ok. No problem. Thanks for the information. > > Your comments are more than welcome. > > Sebastian Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 2023-09-12 10:18:28 [+0200], Lukasz Majewski wrote: > Hi Sebastian, Hi Lukasz, > Ok. No problem. Thanks for the information. So what happens if you try this: diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c index b77f1189d19d1..6d14d935ee828 100644 --- a/net/hsr/hsr_framereg.c +++ b/net/hsr/hsr_framereg.c @@ -288,13 +288,13 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame) /* And leave the HSR tag. */ if (ethhdr->h_proto == htons(ETH_P_HSR)) { - pull_size = sizeof(struct ethhdr); + pull_size = sizeof(struct hsr_tag); skb_pull(skb, pull_size); total_pull_size += pull_size; } /* And leave the HSR sup tag. */ - pull_size = sizeof(struct hsr_tag); + pull_size = sizeof(struct hsr_sup_tag); skb_pull(skb, pull_size); total_pull_size += pull_size; Sebastian
Hi Sebastian Andrzej, > On 2023-09-12 10:18:28 [+0200], Lukasz Majewski wrote: > > Hi Sebastian, > Hi Lukasz, > > > Ok. No problem. Thanks for the information. > > So what happens if you try this: > > diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c > index b77f1189d19d1..6d14d935ee828 100644 > --- a/net/hsr/hsr_framereg.c > +++ b/net/hsr/hsr_framereg.c > @@ -288,13 +288,13 @@ void hsr_handle_sup_frame(struct hsr_frame_info > *frame) > /* And leave the HSR tag. */ > if (ethhdr->h_proto == htons(ETH_P_HSR)) { > - pull_size = sizeof(struct ethhdr); > + pull_size = sizeof(struct hsr_tag); > skb_pull(skb, pull_size); > total_pull_size += pull_size; > } > > /* And leave the HSR sup tag. */ > - pull_size = sizeof(struct hsr_tag); > + pull_size = sizeof(struct hsr_sup_tag); > skb_pull(skb, pull_size); > total_pull_size += pull_size; > > > Sebastian Yes, this fixes this issue (caused by: SHA1: eafaa88b3eb7f). Such solution has also been pointed out earlier by Tristram. I will prepare v2 of this patch. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On 2023-09-14 14:26:50 [+0200], Lukasz Majewski wrote: > Hi Sebastian Andrzej, Hi, > > Yes, this fixes this issue (caused by: SHA1: eafaa88b3eb7f). > Such solution has also been pointed out earlier by Tristram. Right, now that I looked at it… > I will prepare v2 of this patch. don't worry, I take care of this. > Best regards, > > Lukasz Majewski Sebastian
diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c index 80fc71daf7ca..85abe052e0a9 100644 --- a/net/hsr/hsr_framereg.c +++ b/net/hsr/hsr_framereg.c @@ -300,9 +300,24 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame) ethhdr = (struct ethhdr *)skb_mac_header(skb); - /* And leave the HSR tag. */ + * And leave the HSR tag. + * + * The HSRv1 supervisory frame encapsulates the v0 frame + * with EtherType of 0x88FB + */ if (ethhdr->h_proto == htons(ETH_P_HSR)) { - pull_size = sizeof(struct ethhdr); + if (hsr->prot_version == HSR_V1) + /* In the above step the DA, SA and EtherType + * (0x892F - HSRv1) bytes has been removed. + * + * As the HSRv1 has the HSR header added, one need + * to remove path_and_LSDU_size and sequence_nr fields. + * + */ + pull_size = 4; + else + pull_size = sizeof(struct hsr_tag); + skb_pull(skb, pull_size); total_pull_size += pull_size; } @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame) total_pull_size += pull_size; /* get HSR sup payload */ + if (hsr->prot_version == HSR_V1) { + /* In the HSRv1 supervisor frame, when + * one with EtherType = 0x88FB is extracted, the Node A + * MAC address is preceded with type and length elements of TLV + * data field. + * + * It needs to be removed to get the remote peer MAC address. + */ + pull_size = sizeof(struct hsr_sup_tlv); + skb_pull(skb, pull_size); + total_pull_size += pull_size; + } + hsr_sp = (struct hsr_sup_payload *)skb->data; /* Merge node_curr (registered on macaddress_B) into node_real */
Provide fix to decode correctly supervisory frames when HSRv1 version of the HSR protocol is used. Without this patch console is polluted with: ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node as a result of destination node's A MAC address equals to: 00:00:00:00:00:00. cat /sys/kernel/debug/hsr/hsr0/node_table Node Table entries for (HSR) device MAC-Address-A, MAC-Address-B, time_in[A], time_in[B], Address-B 00:00:00:00:00:00 00:10:a1:94:77:30 400bf, 399c, 0 It was caused by wrong frames decoding in the hsr_handle_sup_frame(). As the supervisor frame is encapsulated in HSRv1 frame: SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34 SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30 SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 SKB_I100000040: 00 00 The code had to be adjusted accordingly and the MAC-Address-A now has the proper address (the MAC-Address-B now has all 0's). Signed-off-by: Lukasz Majewski <lukma@denx.de> --- net/hsr/hsr_framereg.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)