lkml.org 
[lkml]   [2020]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames
From
Date
Hi Grygoii,

On 7/20/20 10:08 AM, Murali Karicheri wrote:
> Grygorii,
>
> On 7/17/20 1:39 PM, Grygorii Strashko wrote:
>>
>>
>> On 17/07/2020 17:55, Murali Karicheri wrote:
>>> For small Ethernet frames with size less than minimum size 66 for HSR
>>> vs 60 for regular Ethernet frames, hsr driver currently doesn't pad the
>>> frame to make it minimum size. This results in incorrect LSDU size being
>>> populated in the HSR tag for these frames. Fix this by padding the frame
>>> to the minimum size applicable for HSR.
>>>
>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>>> ---
>>>   no change from original version
>>>   Sending this bug fix ahead of PRP patch series as per comment
>>>   net/hsr/hsr_forward.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>>   Sending this bug fix ahead of PRP patch series as per comment
>>> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
>>> index ed13760463de..e42fd356f073 100644
>>> --- a/net/hsr/hsr_forward.c
>>> +++ b/net/hsr/hsr_forward.c
>>> @@ -127,6 +127,9 @@ static void hsr_fill_tag(struct sk_buff *skb,
>>> struct hsr_frame_info *frame,
>>>       int lane_id;
>>>       int lsdu_size;
>>> +    /* pad to minimum packet size which is 60 + 6 (HSR tag) */
>>> +    skb_put_padto(skb, ETH_ZLEN + HSR_HLEN);
>>
>> It may fail.
>> And i worry that it might be not the right place to do that
>> (if packet is small it will be called for every copy of the packet).
>> May be it has to be done once when packet enters LRE device?
>>
> A better place may be to add it at the beginning of
> hsr_fill_frame_info() at which point there is one copy and after that
> code enters hsr_forward_do() to replicate. I don't think we can place it
> anywhere before that code.
>
> hsr_dev_xmit()
>    - hsr_forward_skb()
>       - hsr_fill_frame_info()
>
> Inside hsr_fill_frame_info() we could do
>
>     if (ethhdr->h_proto == htons(ETH_P_8021Q)) {
>         frame->is_vlan = true;
>         /* FIXME: */
>         netdev_warn_once(skb->dev, "VLAN not yet supported");
>     }
> +    min_size = ETH_ZLEN + HSR_HLEN;
> +       if (frame->is_vlan)
> +        min_size += 4;
> +    ret = skb_put_padto(skb, min_size))
> +    if (ret)
> +         return ret;
>
> At this point, it will be ready to tag the frame. Frame will be either a
> supervision frame which is already tagged or standard frame from upper
> layer. Either case, padto() is required. So looks like the right place
> to avoid doing it twice.
>
> And packet would get dropped at the caller if skb_put_padto() fails. So
> we could return the return value to the caller.
>
> This also eliminates similar padto() call in
> send_hsr_supervision_frame() as well.
>
> What do you think?

Dave has already applied the patch. So I will send a follow up
patch that fixes the original issue to check for the return type.

The patch to optimize this at the correct location (second part
of your concern) can be a separate patch that will send as a follow up
to the PRP series as that code touches this as well. So better to
optimize this after the PRP series IMO.

I plan to fix similar issue in the PRP series as well ( not
checking for return type) and re-spin. Don't want to delay merge
of PRP series for this optimization that can be addressed
separately as I have said.

Murali
>
> Murali
>>> +
>>>       if (port->type == HSR_PT_SLAVE_A)
>>>           lane_id = 0;
>>>       else
>>>
>>
>

--
Murali Karicheri
Texas Instruments

\
 
 \ /
  Last update: 2020-07-20 17:26    [W:0.114 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site