| Message ID | 1445404495-14144-4-git-send-email-bala.manoharan@linaro.org |
|---|---|
| State | Accepted |
| Commit | bbf7c79868be30269a2b88fa719d3dbad7e83085 |
| Headers | show |
Seems in last patch the following was added to test_pktio_error_cos(): + seqno = cls_pkt_get_seq(pkt); + CU_ASSERT(seqno != TEST_SEQ_INVALID); ... + CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); Why did you skip it here? I expected patch with name "Align..." including this change. For this concrete change: Reviewed-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> On 21.10.15 08:14, Balasubramanian Manoharan wrote: > check for invalid sequence number is performed during packet create and > it is not required during packet receive > > Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> > --- > test/validation/classification/odp_classification_test_pmr.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/test/validation/classification/odp_classification_test_pmr.c b/test/validation/classification/odp_classification_test_pmr.c > index c058caf..4bfe0cb 100644 > --- a/test/validation/classification/odp_classification_test_pmr.c > +++ b/test/validation/classification/odp_classification_test_pmr.c > @@ -443,7 +443,6 @@ static void classification_test_pmr_term_udp_sport(void) > > pkt = receive_packet(&retqueue, ODP_TIME_SEC); > CU_ASSERT(pkt != ODP_PACKET_INVALID); > - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); > CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); > CU_ASSERT(retqueue == defqueue); > odp_packet_free(pkt); > @@ -523,7 +522,6 @@ static void classification_test_pmr_term_ipproto(void) > > pkt = receive_packet(&retqueue, ODP_TIME_SEC); > CU_ASSERT(pkt != ODP_PACKET_INVALID); > - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); > CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); > CU_ASSERT(retqueue == defqueue); > >
I skipped this coz this as I just realised that since this was an error packet and it might not be required for platforms to parse an error packet and hence I removed the sequence number check for this packet and just checked whether this packet was received on error CoS queue. Regards, Bala On 21 October 2015 at 16:50, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote: > Seems in last patch the following was added to test_pktio_error_cos(): > > + seqno = cls_pkt_get_seq(pkt); > + CU_ASSERT(seqno != TEST_SEQ_INVALID); > ... > + CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); > > Why did you skip it here? > I expected patch with name "Align..." including this change. > > For this concrete change: > Reviewed-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> > > > On 21.10.15 08:14, Balasubramanian Manoharan wrote: >> >> check for invalid sequence number is performed during packet create and >> it is not required during packet receive >> >> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> >> --- >> test/validation/classification/odp_classification_test_pmr.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/test/validation/classification/odp_classification_test_pmr.c >> b/test/validation/classification/odp_classification_test_pmr.c >> index c058caf..4bfe0cb 100644 >> --- a/test/validation/classification/odp_classification_test_pmr.c >> +++ b/test/validation/classification/odp_classification_test_pmr.c >> @@ -443,7 +443,6 @@ static void >> classification_test_pmr_term_udp_sport(void) >> >> pkt = receive_packet(&retqueue, ODP_TIME_SEC); >> CU_ASSERT(pkt != ODP_PACKET_INVALID); >> - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); >> CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >> CU_ASSERT(retqueue == defqueue); >> odp_packet_free(pkt); >> @@ -523,7 +522,6 @@ static void classification_test_pmr_term_ipproto(void) >> >> pkt = receive_packet(&retqueue, ODP_TIME_SEC); >> CU_ASSERT(pkt != ODP_PACKET_INVALID); >> - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); >> CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >> CU_ASSERT(retqueue == defqueue); >> >> > > -- > Regards, > Ivan Khoronzhuk
On 21.10.15 15:14, Bala Manoharan wrote: > I skipped this coz this as I just realised that since this was an > error packet and it might not be required for platforms to parse an > error packet and hence I removed the sequence number check for this > packet and just checked whether this packet was received on error CoS > queue. > > Regards, > Bala It rather needed to identify packet, and this packet shouldn't be corrupted also. It think it was correct change. > > On 21 October 2015 at 16:50, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote: >> Seems in last patch the following was added to test_pktio_error_cos(): >> >> + seqno = cls_pkt_get_seq(pkt); >> + CU_ASSERT(seqno != TEST_SEQ_INVALID); >> ... >> + CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >> >> Why did you skip it here? >> I expected patch with name "Align..." including this change. >> >> For this concrete change: >> Reviewed-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >> >> >> On 21.10.15 08:14, Balasubramanian Manoharan wrote: >>> >>> check for invalid sequence number is performed during packet create and >>> it is not required during packet receive >>> >>> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> >>> --- >>> test/validation/classification/odp_classification_test_pmr.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/test/validation/classification/odp_classification_test_pmr.c >>> b/test/validation/classification/odp_classification_test_pmr.c >>> index c058caf..4bfe0cb 100644 >>> --- a/test/validation/classification/odp_classification_test_pmr.c >>> +++ b/test/validation/classification/odp_classification_test_pmr.c >>> @@ -443,7 +443,6 @@ static void >>> classification_test_pmr_term_udp_sport(void) >>> >>> pkt = receive_packet(&retqueue, ODP_TIME_SEC); >>> CU_ASSERT(pkt != ODP_PACKET_INVALID); >>> - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); >>> CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >>> CU_ASSERT(retqueue == defqueue); >>> odp_packet_free(pkt); >>> @@ -523,7 +522,6 @@ static void classification_test_pmr_term_ipproto(void) >>> >>> pkt = receive_packet(&retqueue, ODP_TIME_SEC); >>> CU_ASSERT(pkt != ODP_PACKET_INVALID); >>> - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); >>> CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >>> CU_ASSERT(retqueue == defqueue); >>> >>> >> >> -- >> Regards, >> Ivan Khoronzhuk
Whenever a packet contains an error in L3 layer the HW will not parse the L4 layer and since we are reading the sequence number in the L4 data checking the sequence number in this case of error packet is invalid. Regards, Bala On 21 October 2015 at 18:15, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote: > > On 21.10.15 15:14, Bala Manoharan wrote: >> >> I skipped this coz this as I just realised that since this was an >> error packet and it might not be required for platforms to parse an >> error packet and hence I removed the sequence number check for this >> packet and just checked whether this packet was received on error CoS >> queue. >> >> Regards, >> Bala > > > It rather needed to identify packet, > and this packet shouldn't be corrupted also. > It think it was correct change. > > >> >> On 21 October 2015 at 16:50, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >> wrote: >>> >>> Seems in last patch the following was added to test_pktio_error_cos(): >>> >>> + seqno = cls_pkt_get_seq(pkt); >>> + CU_ASSERT(seqno != TEST_SEQ_INVALID); >>> ... >>> + CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >>> >>> Why did you skip it here? >>> I expected patch with name "Align..." including this change. >>> >>> For this concrete change: >>> Reviewed-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>> >>> >>> On 21.10.15 08:14, Balasubramanian Manoharan wrote: >>>> >>>> >>>> check for invalid sequence number is performed during packet create and >>>> it is not required during packet receive >>>> >>>> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> >>>> --- >>>> test/validation/classification/odp_classification_test_pmr.c | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git >>>> a/test/validation/classification/odp_classification_test_pmr.c >>>> b/test/validation/classification/odp_classification_test_pmr.c >>>> index c058caf..4bfe0cb 100644 >>>> --- a/test/validation/classification/odp_classification_test_pmr.c >>>> +++ b/test/validation/classification/odp_classification_test_pmr.c >>>> @@ -443,7 +443,6 @@ static void >>>> classification_test_pmr_term_udp_sport(void) >>>> >>>> pkt = receive_packet(&retqueue, ODP_TIME_SEC); >>>> CU_ASSERT(pkt != ODP_PACKET_INVALID); >>>> - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); >>>> CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >>>> CU_ASSERT(retqueue == defqueue); >>>> odp_packet_free(pkt); >>>> @@ -523,7 +522,6 @@ static void >>>> classification_test_pmr_term_ipproto(void) >>>> >>>> pkt = receive_packet(&retqueue, ODP_TIME_SEC); >>>> CU_ASSERT(pkt != ODP_PACKET_INVALID); >>>> - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); >>>> CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >>>> CU_ASSERT(retqueue == defqueue); >>>> >>>> >>> >>> -- >>> Regards, >>> Ivan Khoronzhuk > > > -- > Regards, > Ivan Khoronzhuk
On 21.10.15 16:54, Bala Manoharan wrote: > Whenever a packet contains an error in L3 layer the HW will not parse > the L4 layer and since we are reading the sequence number in the L4 > data checking the sequence number in this case of error packet is > invalid. > > Regards, > Bala Yep. It should be checked in another way. > > On 21 October 2015 at 18:15, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote: >> >> On 21.10.15 15:14, Bala Manoharan wrote: >>> >>> I skipped this coz this as I just realised that since this was an >>> error packet and it might not be required for platforms to parse an >>> error packet and hence I removed the sequence number check for this >>> packet and just checked whether this packet was received on error CoS >>> queue. >>> >>> Regards, >>> Bala >> >> >> It rather needed to identify packet, >> and this packet shouldn't be corrupted also. >> It think it was correct change. >> >> >>> >>> On 21 October 2015 at 16:50, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>> wrote: >>>> >>>> Seems in last patch the following was added to test_pktio_error_cos(): >>>> >>>> + seqno = cls_pkt_get_seq(pkt); >>>> + CU_ASSERT(seqno != TEST_SEQ_INVALID); >>>> ... >>>> + CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >>>> >>>> Why did you skip it here? >>>> I expected patch with name "Align..." including this change. >>>> >>>> For this concrete change: >>>> Reviewed-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>>> >>>> >>>> On 21.10.15 08:14, Balasubramanian Manoharan wrote: >>>>> >>>>> >>>>> check for invalid sequence number is performed during packet create and >>>>> it is not required during packet receive >>>>> >>>>> Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> >>>>> --- >>>>> test/validation/classification/odp_classification_test_pmr.c | 2 -- >>>>> 1 file changed, 2 deletions(-) >>>>> >>>>> diff --git >>>>> a/test/validation/classification/odp_classification_test_pmr.c >>>>> b/test/validation/classification/odp_classification_test_pmr.c >>>>> index c058caf..4bfe0cb 100644 >>>>> --- a/test/validation/classification/odp_classification_test_pmr.c >>>>> +++ b/test/validation/classification/odp_classification_test_pmr.c >>>>> @@ -443,7 +443,6 @@ static void >>>>> classification_test_pmr_term_udp_sport(void) >>>>> >>>>> pkt = receive_packet(&retqueue, ODP_TIME_SEC); >>>>> CU_ASSERT(pkt != ODP_PACKET_INVALID); >>>>> - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); >>>>> CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >>>>> CU_ASSERT(retqueue == defqueue); >>>>> odp_packet_free(pkt); >>>>> @@ -523,7 +522,6 @@ static void >>>>> classification_test_pmr_term_ipproto(void) >>>>> >>>>> pkt = receive_packet(&retqueue, ODP_TIME_SEC); >>>>> CU_ASSERT(pkt != ODP_PACKET_INVALID); >>>>> - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); >>>>> CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); >>>>> CU_ASSERT(retqueue == defqueue); >>>>> >>>>> >>>> >>>> -- >>>> Regards, >>>> Ivan Khoronzhuk >> >> >> -- >> Regards, >> Ivan Khoronzhuk
diff --git a/test/validation/classification/odp_classification_test_pmr.c b/test/validation/classification/odp_classification_test_pmr.c index c058caf..4bfe0cb 100644 --- a/test/validation/classification/odp_classification_test_pmr.c +++ b/test/validation/classification/odp_classification_test_pmr.c @@ -443,7 +443,6 @@ static void classification_test_pmr_term_udp_sport(void) pkt = receive_packet(&retqueue, ODP_TIME_SEC); CU_ASSERT(pkt != ODP_PACKET_INVALID); - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); CU_ASSERT(retqueue == defqueue); odp_packet_free(pkt); @@ -523,7 +522,6 @@ static void classification_test_pmr_term_ipproto(void) pkt = receive_packet(&retqueue, ODP_TIME_SEC); CU_ASSERT(pkt != ODP_PACKET_INVALID); - CU_ASSERT(cls_pkt_get_seq(pkt) != TEST_SEQ_INVALID); CU_ASSERT(seqno == cls_pkt_get_seq(pkt)); CU_ASSERT(retqueue == defqueue);
check for invalid sequence number is performed during packet create and it is not required during packet receive Signed-off-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> --- test/validation/classification/odp_classification_test_pmr.c | 2 -- 1 file changed, 2 deletions(-)