<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 18, 2021 at 10:44 PM Eric Dumazet <<a href="mailto:eric.dumazet@gmail.com" target="_blank">eric.dumazet@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 11/18/21 8:03 AM, Kumar Thangavel wrote:<br>
> Update NC-SI command handler (both standard and OEM) to take into<br>
> account of payload paddings in allocating skb (in case of payload<br>
> size is not 32-bit aligned).<br>
> <br>
> The checksum field follows payload field, without taking payload<br>
> padding into account can cause checksum being truncated, leading to<br>
> dropped packets.<br>
> <br>
<br>
Patch title should start with a prefix, identifying which layer/driver is<br>
involved.<br>
<br>
Probably in this case<br>
<br>
net/ncsi: Add payload to be 32-bit aligned to fix dropped packets<br></blockquote><div>ACK. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> Fixes: fb4ee67529ff ("net/ncsi: Add NCSI OEM command support")<br>
> Signed-off-by: Kumar Thangavel <<a href="mailto:thangavel.k@hcl.com" target="_blank">thangavel.k@hcl.com</a>><br>
> Acked-by: Samuel Mendoza-Jonas <<a href="mailto:sam@mendozajonas.com" target="_blank">sam@mendozajonas.com</a>><br>
> Reviewed-by: Paul Menzel <<a href="mailto:pmenzel@molgen.mpg.de" target="_blank">pmenzel@molgen.mpg.de</a>><br>
> <br>
> ---<br>
>   v7:<br>
>    - Updated padding_bytes as const static int variable<br>
> <br>
>   v6:<br>
>    - Updated type of padding_bytes variable<br>
>    - Updated type of payload<br>
>    - Seperated variable declarations and code<br>
> <br>
>   v5:<br>
>    - Added Fixes tag<br>
>    - Added const variable for padding_bytes<br>
> <br>
>   v4:<br>
>    - Used existing macro for max function<br>
> <br>
>   v3:<br>
>    - Added Macro for MAX<br>
>    - Fixed the missed semicolon<br>
> <br>
>   v2:<br>
>    - Added NC-SI spec version and section<br>
>    - Removed blank line<br>
>    - corrected spellings<br>
> <br>
>   v1:<br>
>    - Initial draft<br>
> <br>
> ---<br>
> ---<br>
>  net/ncsi/ncsi-cmd.c | 24 ++++++++++++++++--------<br>
>  1 file changed, 16 insertions(+), 8 deletions(-)<br>
> <br>
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c<br>
> index ba9ae482141b..9a6f10f4833e 100644<br>
> --- a/net/ncsi/ncsi-cmd.c<br>
> +++ b/net/ncsi/ncsi-cmd.c<br>
> @@ -18,6 +18,8 @@<br>
>  #include "internal.h"<br>
>  #include "ncsi-pkt.h"<br>
>  <br>
> +const static int padding_bytes = 26;<br>
<br>
It would be nice to have some kind of reference, to explain<br>
what is this magic value.<br>
<br></blockquote><div>ACK.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Moving the comment [1] here might make sense.<br>
<br>
<br>
> +<br>
>  u32 ncsi_calculate_checksum(unsigned char *data, int len)<br>
>  {<br>
>       u32 checksum = 0;<br>
> @@ -213,12 +215,17 @@ static int ncsi_cmd_handler_oem(struct sk_buff *skb,<br>
>  {<br>
>       struct ncsi_cmd_oem_pkt *cmd;<br>
>       unsigned int len;<br>
> +     int payload;<br>
<br>
[1] This comment could be moved before @padding_bytes<br>
<br>
> +     /* NC-SI spec DSP_0222_1.2.0, section 8.2.2.2<br>
> +      * requires payload to be padded with 0 to<br>
> +      * 32-bit boundary before the checksum field.<br>
> +      * Ensure the padding bytes are accounted for in<br>
> +      * skb allocation<br>
> +      */<br>
>  <br>
> +     payload = ALIGN(nca->payload, 4);<br>
>       len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;<br>
> -     if (nca->payload < 26)<br>
> -             len += 26;<br>
> -     else<br>
> -             len += nca->payload;<br>
> +     len += max(payload, padding_bytes);<br>
>  <br>
>       cmd = skb_put_zero(skb, len);<br>
>       memcpy(&cmd->mfr_id, nca->data, nca->payload);<br>
> @@ -272,6 +279,7 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca)<br>
>       struct net_device *dev = nd->dev;<br>
>       int hlen = LL_RESERVED_SPACE(dev);<br>
>       int tlen = dev->needed_tailroom;<br>
> +     int payload;<br>
>       int len = hlen + tlen;<br>
>       struct sk_buff *skb;<br>
>       struct ncsi_request *nr;<br>
> @@ -281,14 +289,14 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca)<br>
>               return NULL;<br>
>  <br>
>       /* NCSI command packet has 16-bytes header, payload, 4 bytes checksum.<br>
> +      * Payload needs padding so that the checksum field following payload is<br>
> +      * aligned to 32-bit boundary.<br>
>        * The packet needs padding if its payload is less than 26 bytes to<br>
>        * meet 64 bytes minimal ethernet frame length.<br>
>        */<br>
>       len += sizeof(struct ncsi_cmd_pkt_hdr) + 4;<br>
> -     if (nca->payload < 26)<br>
> -             len += 26;<br>
> -     else<br>
> -             len += nca->payload;<br>
> +     payload = ALIGN(nca->payload, 4);<br>
> +     len += max(payload, padding_bytes);<br>
>  <br>
>       /* Allocate skb */<br>
>       skb = alloc_skb(len, GFP_ATOMIC);<br>
> <br>
</blockquote></div></div>