Message ID | 20211019144127.GA12978@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v2] Add payload to be 32-bit aligned to fix dropped packets | expand |
On Tue, 19 Oct 2021 20:11:27 +0530 Kumar Thangavel wrote: > len += sizeof(struct ncsi_cmd_pkt_hdr) + 4; > - if (nca->payload < 26) > + payload = ALIGN(nca->payload, 4) > + if (payload < 26) > len += 26; > else > - len += nca->payload; > + len += payload; You round up to 4 and then add 26 if the result is smaller. 26 is not a multiple of 4. Is this intentional? Also you can write this on one line: len += max(payload, 26);
On Tue, 19 Oct 2021 20:11:27 +0530 Kumar Thangavel wrote:
> + payload = ALIGN(nca->payload, 4)
This is missing a semicolon.
Hi Kumar, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v5.15-rc6 next-20211019] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Kumar-Thangavel/Add-payload-to-be-32-bit-aligned-to-fix-dropped-packets/20211019-225018 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 519d81956ee277b4419c723adfb154603c2565ba config: i386-randconfig-a005-20211019 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project b37efed957ed0a0193d80020aefd55cb587dfc1f) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/177d89a1966a0830ac30b8962ac9af76c1d675ae git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Kumar-Thangavel/Add-payload-to-be-32-bit-aligned-to-fix-dropped-packets/20211019-225018 git checkout 177d89a1966a0830ac30b8962ac9af76c1d675ae # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): >> net/ncsi/ncsi-cmd.c:298:34: error: expected ';' after expression payload = ALIGN(nca->payload, 4) ^ ; 1 error generated. vim +298 net/ncsi/ncsi-cmd.c 274 275 static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) 276 { 277 struct ncsi_dev_priv *ndp = nca->ndp; 278 struct ncsi_dev *nd = &ndp->ndev; 279 struct net_device *dev = nd->dev; 280 int hlen = LL_RESERVED_SPACE(dev); 281 int tlen = dev->needed_tailroom; 282 int payload; 283 int len = hlen + tlen; 284 struct sk_buff *skb; 285 struct ncsi_request *nr; 286 287 nr = ncsi_alloc_request(ndp, nca->req_flags); 288 if (!nr) 289 return NULL; 290 291 /* NCSI command packet has 16-bytes header, payload, 4 bytes checksum. 292 * Payload needs padding so that the checksum field following payload is 293 * aligned to 32-bit boundary. 294 * The packet needs padding if its payload is less than 26 bytes to 295 * meet 64 bytes minimal ethernet frame length. 296 */ 297 len += sizeof(struct ncsi_cmd_pkt_hdr) + 4; > 298 payload = ALIGN(nca->payload, 4) 299 if (payload < 26) 300 len += 26; 301 else 302 len += payload; 303 304 /* Allocate skb */ 305 skb = alloc_skb(len, GFP_ATOMIC); 306 if (!skb) { 307 ncsi_free_request(nr); 308 return NULL; 309 } 310 311 nr->cmd = skb; 312 skb_reserve(skb, hlen); 313 skb_reset_network_header(skb); 314 315 skb->dev = dev; 316 skb->protocol = htons(ETH_P_NCSI); 317 318 return nr; 319 } 320 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Thanks Jakub for your comments. Please find my response inline below. On Tue, Oct 19, 2021 at 8:22 PM Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 19 Oct 2021 20:11:27 +0530 Kumar Thangavel wrote: > > len += sizeof(struct ncsi_cmd_pkt_hdr) + 4; > > - if (nca->payload < 26) > > + payload = ALIGN(nca->payload, 4) > > + if (payload < 26) > > len += 26; > > else > > - len += nca->payload; > > + len += payload; > > You round up to 4 and then add 26 if the result is smaller. 26 is not > a multiple of 4. Is this intentional? > > Kumar : This is intentional. The total number of bytes should be 64. This 64 bytes includes Ethernet header, NC-SI header and payload, pldm header and payload. Some pldm commands payload is less than 26. So we added remaining bytes to match with 64 and which is 4 bytes aligned. Also you can write this on one line: > > len += max(payload, 26); > Kumar : Ack. Will update in the next patch set.
On Wed, Oct 20, 2021 at 12:37 AM Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 19 Oct 2021 20:11:27 +0530 Kumar Thangavel wrote: > > + payload = ALIGN(nca->payload, 4) > > This is missing a semicolon. > Kumar : Ack. Will fix this in next PS.
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c index ba9ae482141b..29a75b79a811 100644 --- a/net/ncsi/ncsi-cmd.c +++ b/net/ncsi/ncsi-cmd.c @@ -213,12 +213,19 @@ static int ncsi_cmd_handler_oem(struct sk_buff *skb, { struct ncsi_cmd_oem_pkt *cmd; unsigned int len; + /* NC-SI spec DSP_0222_1.2.0, section 8.2.2.2 + * requires payload to be padded with 0 to + * 32-bit boundary before the checksum field. + * Ensure the padding bytes are accounted for in + * skb allocation + */ + unsigned short payload = ALIGN(nca->payload, 4); len = sizeof(struct ncsi_cmd_pkt_hdr) + 4; - if (nca->payload < 26) + if (payload < 26) len += 26; else - len += nca->payload; + len += payload; cmd = skb_put_zero(skb, len); memcpy(&cmd->mfr_id, nca->data, nca->payload); @@ -272,6 +279,7 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) struct net_device *dev = nd->dev; int hlen = LL_RESERVED_SPACE(dev); int tlen = dev->needed_tailroom; + int payload; int len = hlen + tlen; struct sk_buff *skb; struct ncsi_request *nr; @@ -281,14 +289,17 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) return NULL; /* NCSI command packet has 16-bytes header, payload, 4 bytes checksum. + * Payload needs padding so that the checksum field following payload is + * aligned to 32-bit boundary. * The packet needs padding if its payload is less than 26 bytes to * meet 64 bytes minimal ethernet frame length. */ len += sizeof(struct ncsi_cmd_pkt_hdr) + 4; - if (nca->payload < 26) + payload = ALIGN(nca->payload, 4) + if (payload < 26) len += 26; else - len += nca->payload; + len += payload; /* Allocate skb */ skb = alloc_skb(len, GFP_ATOMIC);