- Download Diff

Summary:

10778 mac: NULL pointer errors

Review Request #1743 — Created April 14, 2019 and submitted April 20, 2019, 8:15 a.m.

Information
Owner:	tsoome
Repository:	illumos-gate
Branch:
Bugs:	10778
Depends On:
Commit:	0a4ee87...
Reviewers
Groups:	general
People:

Description

../../common/io/mac/mac_hio.c: In function 'i_mac_share_free':
../../common/io/mac/mac_hio.c:97:22: error: comparison between pointer and integer [-Werror]
  if (mcip->mci_share == NULL) {
                      ^~
../../common/io/mac/mac_hio.c: In function 'mac_share_bind':
../../common/io/mac/mac_hio.c:121:22: error: comparison between pointer and integer [-Werror]
  if (mcip->mci_share == NULL) {
                      ^~
../../common/io/mac/mac_hio.c: In function 'mac_share_unbind':
../../common/io/mac/mac_hio.c:171:22: error: comparison between pointer and integer [-Werror]
  if (mcip->mci_share == NULL) {
                      ^~

../../common/io/mac/mac_datapath_setup.c: In function 'mac_next_bind_cpu':
../../common/io/mac/mac_datapath_setup.c:553:9: error: return makes integer from pointer without a cast [-Werror=int-conversion]
  return (NULL);
         ^
../../common/io/mac/mac_datapath_setup.c: In function 'mac_datapath_setup':
../../common/io/mac/mac_datapath_setup.c:2936:33: error: comparison between pointer and integer [-Werror]
        (rxhw || mcip->mci_share != NULL)) {
                                 ^~
../../common/io/mac/mac_datapath_setup.c:2962:33: error: comparison between pointer and integer [-Werror]
        (txhw || mcip->mci_share != NULL)) {
                                 ^~

../../common/io/mac/mac_client.c: In function 'mac_client_open':
../../common/io/mac/mac_client.c:1450:18: error: assignment makes integer from pointer without a cast [-Werror=int-conversion]
  mcip->mci_share = NULL;
                  ^
../../common/io/mac/mac_client.c: In function 'mac_client_set_rings_prop':
../../common/io/mac/mac_client.c:1705:22: error: comparison between pointer and integer [-Werror]
  if (mcip->mci_share != NULL)
                      ^~
In file included from ../../common/sys/param.h:48:0,
                 from ../../common/sys/t_lock.h:38,
                 from ../../common/sys/conf.h:37,
                 from ../../common/io/mac/mac_client.c:109:
../../common/io/mac/mac_client.c: In function 'mac_tx':
../../common/sys/null.h:32:14: error: initialization makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_client.c:3437:28: note: in expansion of macro 'NULL'
  mac_tx_cookie_t  cookie = NULL;
                            ^~~~
../../common/io/mac/mac_client.c:3454:11: error: return makes integer from pointer without a cast [-Werror=int-conversion]
    return (NULL);
           ^
../../common/io/mac/mac_client.c:3535:11: error: assignment makes integer from pointer without a cast [-Werror=int-conversion]
    cookie = NULL;
           ^
../../common/io/mac/mac_client.c: In function 'mac_tx_is_flow_blocked':
../../common/io/mac/mac_client.c:3605:14: error: comparison between pointer and integer [-Werror]
   if (cookie != NULL) {
              ^~

In file included from ../../common/sys/param.h:48:0,
                 from ../../common/sys/t_lock.h:38,
                 from ../../common/sys/callb.h:29,
                 from ../../common/io/mac/mac_sched.c:971:
../../common/io/mac/mac_sched.c: In function 'mac_tx_srs_no_desc':
../../common/sys/null.h:32:14: error: initialization makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_sched.c:3448:27: note: in expansion of macro 'NULL'
  mac_tx_cookie_t cookie = NULL;
                           ^~~~
../../common/io/mac/mac_sched.c: In function 'mac_tx_srs_enqueue':
../../common/sys/null.h:32:14: error: initialization makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_sched.c:3504:27: note: in expansion of macro 'NULL'
  mac_tx_cookie_t cookie = NULL;
                           ^~~~
../../common/io/mac/mac_sched.c: In function 'mac_tx_single_ring_mode':
../../common/sys/null.h:32:14: error: initialization makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_sched.c:3646:28: note: in expansion of macro 'NULL'
  mac_tx_cookie_t  cookie = NULL;
                            ^~~~
../../common/io/mac/mac_sched.c:3695:9: error: return makes integer from pointer without a cast [-Werror=int-conversion]
  return (NULL);
         ^
In file included from ../../common/sys/param.h:48:0,
                 from ../../common/sys/t_lock.h:38,
                 from ../../common/sys/callb.h:29,
                 from ../../common/io/mac/mac_sched.c:971:
../../common/io/mac/mac_sched.c: In function 'mac_tx_serializer_mode':
../../common/sys/null.h:32:14: error: initialization makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_sched.c:3712:28: note: in expansion of macro 'NULL'
  mac_tx_cookie_t  cookie = NULL;
                            ^~~~
../../common/sys/null.h:32:14: error: passing argument 4 of 'mac_tx_srs_enqueue' makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_sched.c:3728:13: note: in expansion of macro 'NULL'
       flag, NULL, ret_mp);
             ^~~~
../../common/io/mac/mac_sched.c:3501:1: note: expected 'uintptr_t {aka long unsigned int}' but argument is of type 'void *'
 mac_tx_srs_enqueue(mac_soft_ring_set_t *mac_srs, mblk_t *mp_chain,
 ^~~~~~~~~~~~~~~~~~
In file included from ../../common/sys/param.h:48:0,
                 from ../../common/sys/t_lock.h:38,
                 from ../../common/sys/callb.h:29,
                 from ../../common/io/mac/mac_sched.c:971:
../../common/sys/null.h:32:14: error: passing argument 4 of 'mac_tx_srs_enqueue' makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_sched.c:3747:23: note: in expansion of macro 'NULL'
       mp_chain, flag, NULL, ret_mp);
                       ^~~~
../../common/io/mac/mac_sched.c:3501:1: note: expected 'uintptr_t {aka long unsigned int}' but argument is of type 'void *'
 mac_tx_srs_enqueue(mac_soft_ring_set_t *mac_srs, mblk_t *mp_chain,
 ^~~~~~~~~~~~~~~~~~
../../common/io/mac/mac_sched.c:3759:13: error: comparison between pointer and integer [-Werror]
  if (cookie == NULL)
             ^~
In file included from ../../common/sys/param.h:48:0,
                 from ../../common/sys/t_lock.h:38,
                 from ../../common/sys/callb.h:29,
                 from ../../common/io/mac/mac_sched.c:971:
../../common/io/mac/mac_sched.c: In function 'mac_tx_fanout_mode':
../../common/sys/null.h:32:14: error: initialization makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_sched.c:3791:28: note: in expansion of macro 'NULL'
  mac_tx_cookie_t  cookie = NULL;
                            ^~~~
../../common/io/mac/mac_sched.c:3856:10: error: assignment makes integer from pointer without a cast [-Werror=int-conversion]
   cookie = NULL;
          ^
In file included from ../../common/sys/param.h:48:0,
                 from ../../common/sys/t_lock.h:38,
                 from ../../common/sys/callb.h:29,
                 from ../../common/io/mac/mac_sched.c:971:
../../common/io/mac/mac_sched.c: In function 'mac_tx_bw_mode':
../../common/sys/null.h:32:14: error: initialization makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_sched.c:3876:28: note: in expansion of macro 'NULL'
  mac_tx_cookie_t  cookie = NULL;
                            ^~~~
../../common/io/mac/mac_sched.c:3966:10: error: return makes integer from pointer without a cast [-Werror=int-conversion]
   return (NULL);
          ^
../../common/io/mac/mac_sched.c: In function 'mac_tx_aggr_mode':
../../common/io/mac/mac_sched.c:4004:10: error: return makes integer from pointer without a cast [-Werror=int-conversion]
   return (NULL);
          ^
In file included from ../../common/sys/param.h:48:0,
                 from ../../common/sys/t_lock.h:38,
                 from ../../common/sys/callb.h:29,
                 from ../../common/io/mac/mac_sched.c:971:
../../common/io/mac/mac_sched.c: In function 'mac_tx_sring_enqueue':
../../common/sys/null.h:32:14: error: initialization makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_sched.c:4818:27: note: in expansion of macro 'NULL'
  mac_tx_cookie_t cookie = NULL;
                           ^~~~
../../common/io/mac/mac_sched.c: In function 'mac_tx_soft_ring_process':
../../common/sys/null.h:32:14: error: initialization makes integer from pointer without a cast [-Werror=int-conversion]
 #define NULL ((void *)0)
              ^
../../common/io/mac/mac_sched.c:4902:27: note: in expansion of macro 'NULL'
  mac_tx_cookie_t cookie = NULL;
                           ^~~~
../../common/io/mac/mac_sched.c:4992:10: error: return makes integer from pointer without a cast [-Werror=int-conversion]
   return (NULL);
          ^

../../common/io/mac/mac.c: In function 'i_mac_group_allocate_rings':
../../common/io/mac/mac.c:6166:12: error: comparison between pointer and integer [-Werror]
  if (share != NULL) {
            ^~
../../common/io/mac/mac.c:6275:12: error: comparison between pointer and integer [-Werror]
  if (share != NULL)
            ^~
../../common/io/mac/mac.c: In function 'mac_reserve_rx_group':
../../common/io/mac/mac.c:6464:28: error: comparison between pointer and integer [-Werror]
     if (gclient->mci_share == NULL &&
                            ^~
../../common/io/mac/mac.c:6511:15: error: comparison between pointer and integer [-Werror]
     if (share != NULL) {
               ^~
../../common/io/mac/mac.c:6598:15: error: comparison between pointer and integer [-Werror]
     if (share != NULL) {
               ^~
../../common/io/mac/mac.c: In function 'mac_release_rx_group':
../../common/io/mac/mac.c:6661:22: error: comparison between pointer and integer [-Werror]
  if (mcip->mci_share != NULL) {
                      ^~
../../common/io/mac/mac.c: In function 'mac_reserve_tx_group':
../../common/io/mac/mac.c:7002:28: error: comparison between pointer and integer [-Werror]
     if (gclient->mci_share == NULL &&
                            ^~
../../common/io/mac/mac.c:7070:15: error: comparison between pointer and integer [-Werror]
     if (share != NULL) {
               ^~
../../common/io/mac/mac.c:7104:12: error: comparison between pointer and integer [-Werror]
  if (share != NULL) {
            ^~
../../common/io/mac/mac.c: In function 'mac_release_tx_group':
../../common/io/mac/mac.c:7138:12: error: comparison between pointer and integer [-Werror]
  if (share != NULL)
            ^~
cc1: all warnings being treated as errors

Testing Done

Issues

0
0
2
0
All Issues: 2

Description	From	Last Updated
This seems to be treated like an error condition, although it amounts to a default to cpu_id 0. It probably ...	pmooney	April 17, 2019, 5:34 a.m.
Something like "No matching CPU found online" might be more accurate, since in some cases is the CPU partition constraint ...	pmooney	April 17, 2019, 7:24 p.m.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Looks good, save for the one question about mac_next_bind_cpu.

usr/src/uts/common/io/mac/mac_datapath_setup.c (Diff revision 1)

Show all issues

This seems to be treated like an error condition, although it amounts to a default to cpu_id 0. It probably warrants further investigation, rather than smoothing over a compiler warning which does indicate a potential issue.

tsoome April 16, 2019, 12:07 p.m.

um, well... they do use -1 to denote unusable cpuid, assuming we should return error, -1 should be used there. I am not sure at all what does happen if we actually do return -1 there...

rzezeski April 16, 2019, 9:21 p.m.

I spent the last two hours going over every consumer of mac_next_bind_cpu() and traced how its return value is used. My takeaway is that we should return -1, not 0. The consumers of this value (namely MAC's Tx/Rx CPU binding of soft rings/workers/polling/interrupts) all treat -1 as "no bind" or "no CPU found". It would treat the value 0 as "bind to CPU 0". This is not desirable for three reaaons:


CPU 0 may be offline.


CPU 0 may not be a member of the CPU partition assigned to this flow.


We could end up piling many many threads and interrupts onto CPU 0.


All of the consumers of this value end up calling cpu_get() first. This value is always checked against NULL and if so the binding operation is not performed. Furtermore, the only time mac_next_bind_cpu() would return -1 is if a CPU partition was defined but all of its CPUs are offline -- if that's the case then we have much bigger things to worry about I'm sure. So really I don't think this value is ever actually returned, but if we have to put a value there I strongly believe it should be -1 to indicate "no CPU found" and let the rest of MAC decide what it wants to do (which in this case means just don't bind the thread/interrupt).

Change Summary:

return -1 to denote condition that no online cpu was found.

Commit:

-	350f52cde4b46d9d15829b8e7762e852789503f5
+	a55cd419f2dd3508cc6020aba93b25ae1d46a517

Diff:

Revision 2 (+185 -185)

Show changes

	usr/src/uts/common/io/mac/mac.c
	usr/src/uts/common/io/mac/mac_client.c
	usr/src/uts/common/io/mac/mac_datapath_setup.c
	usr/src/uts/common/io/mac/mac_hio.c
	usr/src/uts/common/io/mac/mac_sched.c

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

usr/src/uts/common/io/mac/mac_datapath_setup.c (Diff revisions 1 - 2)

Show all issues

Something like "No matching CPU found online" might be more accurate, since in some cases is the CPU partition constraint that would cause the search to fail.

Commit:

-	a55cd419f2dd3508cc6020aba93b25ae1d46a517
+	0a4ee878891ce149ba49341908c4d8ee14096d74

Diff:

Revision 3 (+185 -185)

Show changes

	usr/src/uts/common/io/mac/mac.c
	usr/src/uts/common/io/mac/mac_client.c
	usr/src/uts/common/io/mac/mac_datapath_setup.c
	usr/src/uts/common/io/mac/mac_hio.c
	usr/src/uts/common/io/mac/mac_sched.c

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

10778 mac: NULL pointer errors

Change Summary:

Commit:

Diff:

Commit:

Diff:

Status: Closed (submitted)