diff mbox

validation: pktio: assign MAC address if one loop pktio is used

Message ID 1452768991-25198-1-git-send-email-ivan.khoronzhuk@linaro.org
State Accepted
Commit 5c69fd21455c6889a31dc9882f6a6f79c53590d4
Headers show

Commit Message

Ivan Khoronzhuk Jan. 14, 2016, 10:56 a.m. UTC
In case of one loop pktio the MAC address is not set in the packets
but should be.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 test/validation/pktio/pktio.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Comments

Ivan Khoronzhuk Jan. 18, 2016, 5:52 p.m. UTC | #1
ping

On 14.01.16 12:56, Ivan Khoronzhuk wrote:
> In case of one loop pktio the MAC address is not set in the packets
> but should be.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>   test/validation/pktio/pktio.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
> index 536ef6c..a756af4 100644
> --- a/test/validation/pktio/pktio.c
> +++ b/test/validation/pktio/pktio.c
> @@ -830,11 +830,21 @@ void pktio_test_start_stop(void)
>   		pktio_init_packet(pkt);
>   		if (num_ifaces > 1) {
>   			pktio_pkt_set_macs(pkt, pktio[0], pktio[1]);
> -			if (pktio_fixup_checksums(pkt) != 0) {
> -				odp_packet_free(pkt);
> -				break;
> -			}
> +		} else {
> +			uint32_t len;
> +			odph_ethhdr_t *eth;
> +
> +			eth = (odph_ethhdr_t *)odp_packet_l2_ptr(pkt, &len);
> +			ret = odp_pktio_mac_addr(pktio[0],
> +						 &eth->dst, sizeof(eth->dst));
> +			CU_ASSERT(ret == ODPH_ETHADDR_LEN);
>   		}
> +
> +		if (pktio_fixup_checksums(pkt) != 0) {
> +			odp_packet_free(pkt);
> +			break;
> +		}
> +
>   		tx_ev[alloc] = odp_packet_to_event(pkt);
>   	}
>
>
Stuart Haslam Jan. 22, 2016, 5:22 p.m. UTC | #2
On Thu, Jan 14, 2016 at 12:56:31PM +0200, Ivan Khoronzhuk wrote:
> In case of one loop pktio the MAC address is not set in the packets
> but should be.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  test/validation/pktio/pktio.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
> index 536ef6c..a756af4 100644
> --- a/test/validation/pktio/pktio.c
> +++ b/test/validation/pktio/pktio.c
> @@ -830,11 +830,21 @@ void pktio_test_start_stop(void)
>  		pktio_init_packet(pkt);
>  		if (num_ifaces > 1) {
>  			pktio_pkt_set_macs(pkt, pktio[0], pktio[1]);
> -			if (pktio_fixup_checksums(pkt) != 0) {
> -				odp_packet_free(pkt);
> -				break;
> -			}
> +		} else {
> +			uint32_t len;
> +			odph_ethhdr_t *eth;
> +
> +			eth = (odph_ethhdr_t *)odp_packet_l2_ptr(pkt, &len);
> +			ret = odp_pktio_mac_addr(pktio[0],
> +						 &eth->dst, sizeof(eth->dst));
> +			CU_ASSERT(ret == ODPH_ETHADDR_LEN);

Elsewhere we just call pktio_pkt_set_macs() and pass the same pktio as
both the src and dest, any reason not to do that here?

Actually this reminded me that I sent a different fix for this some time
back that moved the setting of MACs and checksum into the create
function, this way it's less likely the same error will be made in
future -

http://patches.opendataplane.org/patch/3515/

It wouldn't apply now but I could rebase it...

--
Stuart.
Maxim Uvarov Feb. 3, 2016, 2:33 p.m. UTC | #3
Patch itself is ok. I merged this patch as it fixes current problem.
Then will work on Stuart patches and probably we can move macs to other 
place.

Maxim.

On 01/22/2016 20:22, Stuart Haslam wrote:
> On Thu, Jan 14, 2016 at 12:56:31PM +0200, Ivan Khoronzhuk wrote:
>> In case of one loop pktio the MAC address is not set in the packets
>> but should be.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>>   test/validation/pktio/pktio.c | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
>> index 536ef6c..a756af4 100644
>> --- a/test/validation/pktio/pktio.c
>> +++ b/test/validation/pktio/pktio.c
>> @@ -830,11 +830,21 @@ void pktio_test_start_stop(void)
>>   		pktio_init_packet(pkt);
>>   		if (num_ifaces > 1) {
>>   			pktio_pkt_set_macs(pkt, pktio[0], pktio[1]);
>> -			if (pktio_fixup_checksums(pkt) != 0) {
>> -				odp_packet_free(pkt);
>> -				break;
>> -			}
>> +		} else {
>> +			uint32_t len;
>> +			odph_ethhdr_t *eth;
>> +
>> +			eth = (odph_ethhdr_t *)odp_packet_l2_ptr(pkt, &len);
>> +			ret = odp_pktio_mac_addr(pktio[0],
>> +						 &eth->dst, sizeof(eth->dst));
>> +			CU_ASSERT(ret == ODPH_ETHADDR_LEN);
> Elsewhere we just call pktio_pkt_set_macs() and pass the same pktio as
> both the src and dest, any reason not to do that here?
>
> Actually this reminded me that I sent a different fix for this some time
> back that moved the setting of MACs and checksum into the create
> function, this way it's less likely the same error will be made in
> future -
>
> http://patches.opendataplane.org/patch/3515/
>
> It wouldn't apply now but I could rebase it...
>
> --
> Stuart.
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
diff mbox

Patch

diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
index 536ef6c..a756af4 100644
--- a/test/validation/pktio/pktio.c
+++ b/test/validation/pktio/pktio.c
@@ -830,11 +830,21 @@  void pktio_test_start_stop(void)
 		pktio_init_packet(pkt);
 		if (num_ifaces > 1) {
 			pktio_pkt_set_macs(pkt, pktio[0], pktio[1]);
-			if (pktio_fixup_checksums(pkt) != 0) {
-				odp_packet_free(pkt);
-				break;
-			}
+		} else {
+			uint32_t len;
+			odph_ethhdr_t *eth;
+
+			eth = (odph_ethhdr_t *)odp_packet_l2_ptr(pkt, &len);
+			ret = odp_pktio_mac_addr(pktio[0],
+						 &eth->dst, sizeof(eth->dst));
+			CU_ASSERT(ret == ODPH_ETHADDR_LEN);
 		}
+
+		if (pktio_fixup_checksums(pkt) != 0) {
+			odp_packet_free(pkt);
+			break;
+		}
+
 		tx_ev[alloc] = odp_packet_to_event(pkt);
 	}