Re: [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames

[Murali Karicheri <m-karicheri2@ti.com>]

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