Re: [PATCH net,stable] net: cdc_ncm: correct overhead in delayed_ndp_size

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi "Jouni,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11-rc1 next-20201223]
[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/Jouni-Sepp-nen/net-cdc_ncm-correct-overhead-in-delayed_ndp_size/20210103-224538
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3516bd729358a2a9b090c1905bd2a3fa926e24c6
config: x86_64-randconfig-a003-20210103 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 7af6a134508cd1c7f75c6e3441ce436f220f30a4)
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
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/3d8cc665ef1cf4705135a5a96893a6fdc6dcd398
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jouni-Sepp-nen/net-cdc_ncm-correct-overhead-in-delayed_ndp_size/20210103-224538
        git checkout 3d8cc665ef1cf4705135a5a96893a6fdc6dcd398
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@xxxxxxxxx>

All warnings (new ones prefixed by >>):

>> drivers/net/usb/cdc_ncm.c:1203:4: warning: comparison of distinct pointer types ('typeof (ctx->tx_ndp_modulus) *' (aka 'unsigned short *') and 'typeof (ctx->tx_modulus + ctx->tx_remainder) *' (aka 'int *')) [-Wcompare-distinct-pointer-types]
                           max(ctx->tx_ndp_modulus,
                           ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:58:19: note: expanded from macro 'max'
   #define max(x, y)       __careful_cmp(x, y, >)
                           ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:42:24: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                 ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:32:4: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                    ^~~~~~~~~~~~~~~~~
   include/linux/minmax.h:18:28: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   1 warning generated.


vim +1203 drivers/net/usb/cdc_ncm.c

  1179	
  1180	struct sk_buff *
  1181	cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
  1182	{
  1183		struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
  1184		union {
  1185			struct usb_cdc_ncm_nth16 *nth16;
  1186			struct usb_cdc_ncm_nth32 *nth32;
  1187		} nth;
  1188		union {
  1189			struct usb_cdc_ncm_ndp16 *ndp16;
  1190			struct usb_cdc_ncm_ndp32 *ndp32;
  1191		} ndp;
  1192		struct sk_buff *skb_out;
  1193		u16 n = 0, index, ndplen;
  1194		u8 ready2send = 0;
  1195		u32 delayed_ndp_size;
  1196		size_t padding_count;
  1197	
  1198		/* When our NDP gets written in cdc_ncm_ndp(), then skb_out->len gets updated
  1199		 * accordingly. Otherwise, we should check here.
  1200		 */
  1201		if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END)
  1202			delayed_ndp_size = ctx->max_ndp_size +
> 1203				max(ctx->tx_ndp_modulus,
  1204				    ctx->tx_modulus + ctx->tx_remainder) - 1;
  1205		else
  1206			delayed_ndp_size = 0;
  1207	
  1208		/* if there is a remaining skb, it gets priority */
  1209		if (skb != NULL) {
  1210			swap(skb, ctx->tx_rem_skb);
  1211			swap(sign, ctx->tx_rem_sign);
  1212		} else {
  1213			ready2send = 1;
  1214		}
  1215	
  1216		/* check if we are resuming an OUT skb */
  1217		skb_out = ctx->tx_curr_skb;
  1218	
  1219		/* allocate a new OUT skb */
  1220		if (!skb_out) {
  1221			if (ctx->tx_low_mem_val == 0) {
  1222				ctx->tx_curr_size = ctx->tx_max;
  1223				skb_out = alloc_skb(ctx->tx_curr_size, GFP_ATOMIC);
  1224				/* If the memory allocation fails we will wait longer
  1225				 * each time before attempting another full size
  1226				 * allocation again to not overload the system
  1227				 * further.
  1228				 */
  1229				if (skb_out == NULL) {
  1230					ctx->tx_low_mem_max_cnt = min(ctx->tx_low_mem_max_cnt + 1,
  1231								      (unsigned)CDC_NCM_LOW_MEM_MAX_CNT);
  1232					ctx->tx_low_mem_val = ctx->tx_low_mem_max_cnt;
  1233				}
  1234			}
  1235			if (skb_out == NULL) {
  1236				/* See if a very small allocation is possible.
  1237				 * We will send this packet immediately and hope
  1238				 * that there is more memory available later.
  1239				 */
  1240				if (skb)
  1241					ctx->tx_curr_size = max(skb->len,
  1242						(u32)USB_CDC_NCM_NTB_MIN_OUT_SIZE);
  1243				else
  1244					ctx->tx_curr_size = USB_CDC_NCM_NTB_MIN_OUT_SIZE;
  1245				skb_out = alloc_skb(ctx->tx_curr_size, GFP_ATOMIC);
  1246	
  1247				/* No allocation possible so we will abort */
  1248				if (skb_out == NULL) {
  1249					if (skb != NULL) {
  1250						dev_kfree_skb_any(skb);
  1251						dev->net->stats.tx_dropped++;
  1252					}
  1253					goto exit_no_skb;
  1254				}
  1255				ctx->tx_low_mem_val--;
  1256			}
  1257			if (ctx->is_ndp16) {
  1258				/* fill out the initial 16-bit NTB header */
  1259				nth.nth16 = skb_put_zero(skb_out, sizeof(struct usb_cdc_ncm_nth16));
  1260				nth.nth16->dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
  1261				nth.nth16->wHeaderLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16));
  1262				nth.nth16->wSequence = cpu_to_le16(ctx->tx_seq++);
  1263			} else {
  1264				/* fill out the initial 32-bit NTB header */
  1265				nth.nth32 = skb_put_zero(skb_out, sizeof(struct usb_cdc_ncm_nth32));
  1266				nth.nth32->dwSignature = cpu_to_le32(USB_CDC_NCM_NTH32_SIGN);
  1267				nth.nth32->wHeaderLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_nth32));
  1268				nth.nth32->wSequence = cpu_to_le16(ctx->tx_seq++);
  1269			}
  1270	
  1271			/* count total number of frames in this NTB */
  1272			ctx->tx_curr_frame_num = 0;
  1273	
  1274			/* recent payload counter for this skb_out */
  1275			ctx->tx_curr_frame_payload = 0;
  1276		}
  1277	
  1278		for (n = ctx->tx_curr_frame_num; n < ctx->tx_max_datagrams; n++) {
  1279			/* send any remaining skb first */
  1280			if (skb == NULL) {
  1281				skb = ctx->tx_rem_skb;
  1282				sign = ctx->tx_rem_sign;
  1283				ctx->tx_rem_skb = NULL;
  1284	
  1285				/* check for end of skb */
  1286				if (skb == NULL)
  1287					break;
  1288			}
  1289	
  1290			/* get the appropriate NDP for this skb */
  1291			if (ctx->is_ndp16)
  1292				ndp.ndp16 = cdc_ncm_ndp16(ctx, skb_out, sign, skb->len + ctx->tx_modulus + ctx->tx_remainder);
  1293			else
  1294				ndp.ndp32 = cdc_ncm_ndp32(ctx, skb_out, sign, skb->len + ctx->tx_modulus + ctx->tx_remainder);
  1295	
  1296			/* align beginning of next frame */
  1297			cdc_ncm_align_tail(skb_out,  ctx->tx_modulus, ctx->tx_remainder, ctx->tx_curr_size);
  1298	
  1299			/* check if we had enough room left for both NDP and frame */
  1300			if ((ctx->is_ndp16 && !ndp.ndp16) || (!ctx->is_ndp16 && !ndp.ndp32) ||
  1301			    skb_out->len + skb->len + delayed_ndp_size > ctx->tx_curr_size) {
  1302				if (n == 0) {
  1303					/* won't fit, MTU problem? */
  1304					dev_kfree_skb_any(skb);
  1305					skb = NULL;
  1306					dev->net->stats.tx_dropped++;
  1307				} else {
  1308					/* no room for skb - store for later */
  1309					if (ctx->tx_rem_skb != NULL) {
  1310						dev_kfree_skb_any(ctx->tx_rem_skb);
  1311						dev->net->stats.tx_dropped++;
  1312					}
  1313					ctx->tx_rem_skb = skb;
  1314					ctx->tx_rem_sign = sign;
  1315					skb = NULL;
  1316					ready2send = 1;
  1317					ctx->tx_reason_ntb_full++;	/* count reason for transmitting */
  1318				}
  1319				break;
  1320			}
  1321	
  1322			/* calculate frame number within this NDP */
  1323			if (ctx->is_ndp16) {
  1324				ndplen = le16_to_cpu(ndp.ndp16->wLength);
  1325				index = (ndplen - sizeof(struct usb_cdc_ncm_ndp16)) / sizeof(struct usb_cdc_ncm_dpe16) - 1;
  1326	
  1327				/* OK, add this skb */
  1328				ndp.ndp16->dpe16[index].wDatagramLength = cpu_to_le16(skb->len);
  1329				ndp.ndp16->dpe16[index].wDatagramIndex = cpu_to_le16(skb_out->len);
  1330				ndp.ndp16->wLength = cpu_to_le16(ndplen + sizeof(struct usb_cdc_ncm_dpe16));
  1331			} else {
  1332				ndplen = le16_to_cpu(ndp.ndp32->wLength);
  1333				index = (ndplen - sizeof(struct usb_cdc_ncm_ndp32)) / sizeof(struct usb_cdc_ncm_dpe32) - 1;
  1334	
  1335				ndp.ndp32->dpe32[index].dwDatagramLength = cpu_to_le32(skb->len);
  1336				ndp.ndp32->dpe32[index].dwDatagramIndex = cpu_to_le32(skb_out->len);
  1337				ndp.ndp32->wLength = cpu_to_le16(ndplen + sizeof(struct usb_cdc_ncm_dpe32));
  1338			}
  1339			skb_put_data(skb_out, skb->data, skb->len);
  1340			ctx->tx_curr_frame_payload += skb->len;	/* count real tx payload data */
  1341			dev_kfree_skb_any(skb);
  1342			skb = NULL;
  1343	
  1344			/* send now if this NDP is full */
  1345			if (index >= CDC_NCM_DPT_DATAGRAMS_MAX) {
  1346				ready2send = 1;
  1347				ctx->tx_reason_ndp_full++;	/* count reason for transmitting */
  1348				break;
  1349			}
  1350		}
  1351	
  1352		/* free up any dangling skb */
  1353		if (skb != NULL) {
  1354			dev_kfree_skb_any(skb);
  1355			skb = NULL;
  1356			dev->net->stats.tx_dropped++;
  1357		}
  1358	
  1359		ctx->tx_curr_frame_num = n;
  1360	
  1361		if (n == 0) {
  1362			/* wait for more frames */
  1363			/* push variables */
  1364			ctx->tx_curr_skb = skb_out;
  1365			goto exit_no_skb;
  1366	
  1367		} else if ((n < ctx->tx_max_datagrams) && (ready2send == 0) && (ctx->timer_interval > 0)) {
  1368			/* wait for more frames */
  1369			/* push variables */
  1370			ctx->tx_curr_skb = skb_out;
  1371			/* set the pending count */
  1372			if (n < CDC_NCM_RESTART_TIMER_DATAGRAM_CNT)
  1373				ctx->tx_timer_pending = CDC_NCM_TIMER_PENDING_CNT;
  1374			goto exit_no_skb;
  1375	
  1376		} else {
  1377			if (n == ctx->tx_max_datagrams)
  1378				ctx->tx_reason_max_datagram++;	/* count reason for transmitting */
  1379			/* frame goes out */
  1380			/* variables will be reset at next call */
  1381		}
  1382	
  1383		/* If requested, put NDP at end of frame. */
  1384		if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END) {
  1385			if (ctx->is_ndp16) {
  1386				nth.nth16 = (struct usb_cdc_ncm_nth16 *)skb_out->data;
  1387				cdc_ncm_align_tail(skb_out, ctx->tx_ndp_modulus, 0, ctx->tx_curr_size - ctx->max_ndp_size);
  1388				nth.nth16->wNdpIndex = cpu_to_le16(skb_out->len);
  1389				skb_put_data(skb_out, ctx->delayed_ndp16, ctx->max_ndp_size);
  1390	
  1391				/* Zero out delayed NDP - signature checking will naturally fail. */
  1392				ndp.ndp16 = memset(ctx->delayed_ndp16, 0, ctx->max_ndp_size);
  1393			} else {
  1394				nth.nth32 = (struct usb_cdc_ncm_nth32 *)skb_out->data;
  1395				cdc_ncm_align_tail(skb_out, ctx->tx_ndp_modulus, 0, ctx->tx_curr_size - ctx->max_ndp_size);
  1396				nth.nth32->dwNdpIndex = cpu_to_le32(skb_out->len);
  1397				skb_put_data(skb_out, ctx->delayed_ndp32, ctx->max_ndp_size);
  1398	
  1399				ndp.ndp32 = memset(ctx->delayed_ndp32, 0, ctx->max_ndp_size);
  1400			}
  1401		}
  1402	
  1403		/* If collected data size is less or equal ctx->min_tx_pkt
  1404		 * bytes, we send buffers as it is. If we get more data, it
  1405		 * would be more efficient for USB HS mobile device with DMA
  1406		 * engine to receive a full size NTB, than canceling DMA
  1407		 * transfer and receiving a short packet.
  1408		 *
  1409		 * This optimization support is pointless if we end up sending
  1410		 * a ZLP after full sized NTBs.
  1411		 */
  1412		if (!(dev->driver_info->flags & FLAG_SEND_ZLP) &&
  1413		    skb_out->len > ctx->min_tx_pkt) {
  1414			padding_count = ctx->tx_curr_size - skb_out->len;
  1415			if (!WARN_ON(padding_count > ctx->tx_curr_size))
  1416				skb_put_zero(skb_out, padding_count);
  1417		} else if (skb_out->len < ctx->tx_curr_size &&
  1418			   (skb_out->len % dev->maxpacket) == 0) {
  1419			skb_put_u8(skb_out, 0);	/* force short packet */
  1420		}
  1421	
  1422		/* set final frame length */
  1423		if (ctx->is_ndp16) {
  1424			nth.nth16 = (struct usb_cdc_ncm_nth16 *)skb_out->data;
  1425			nth.nth16->wBlockLength = cpu_to_le16(skb_out->len);
  1426		} else {
  1427			nth.nth32 = (struct usb_cdc_ncm_nth32 *)skb_out->data;
  1428			nth.nth32->dwBlockLength = cpu_to_le32(skb_out->len);
  1429		}
  1430	
  1431		/* return skb */
  1432		ctx->tx_curr_skb = NULL;
  1433	
  1434		/* keep private stats: framing overhead and number of NTBs */
  1435		ctx->tx_overhead += skb_out->len - ctx->tx_curr_frame_payload;
  1436		ctx->tx_ntbs++;
  1437	
  1438		/* usbnet will count all the framing overhead by default.
  1439		 * Adjust the stats so that the tx_bytes counter show real
  1440		 * payload data instead.
  1441		 */
  1442		usbnet_set_skb_tx_stats(skb_out, n,
  1443					(long)ctx->tx_curr_frame_payload - skb_out->len);
  1444	
  1445		return skb_out;
  1446	
  1447	exit_no_skb:
  1448		/* Start timer, if there is a remaining non-empty skb */
  1449		if (ctx->tx_curr_skb != NULL && n > 0)
  1450			cdc_ncm_tx_timeout_start(ctx);
  1451		return NULL;
  1452	}
  1453	EXPORT_SYMBOL_GPL(cdc_ncm_fill_tx_frame);
  1454	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@xxxxxxxxxxxx

Attachment: .config.gz
Description: application/gzip


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux