Comments for net80211_impl.h and net80211_ioctl.c attached.
--
K. Poon.
[EMAIL PROTECTED]
net80211_impl.h:
46: Is net80211.h a new file? Is it reviewed?
95-100: It is unclear from reading the comment why those values need
to be divided by IEEE80211_INACT_WAIT. Please explain this
in the comment also.
97-98: Nit. It is not intuitive to have "IEEE80211_INACT_AUTH"
represent "associated but not authorized." Maybe
IEEE80211_INACT_ASSOC? Or IEEE80211_INACT_WAIT_AUTH?
206: Please move this macro up, after IEEE80211_MSG_CONFIG.
net80211_ioctl.c:
50: Should we add a DDI strnlen() instead?
53: Please add an empty line.
75: Do we need to NULL terminate ow_essid->wl_essid_essid? And
do we need to make sure that the wldp_buf is big enough before
copying? If not, please add a comment explaining that.
91: Do we really need to zero out the whole buffer? Can we just
NULL terminate the string after copy?
94: So if the wl_essid_length is 0 and the ic_des_essid is zero'ed
out, we won't see the debug message? I think the debug message
is useful when the ic_des_essid is actually changed.
98: It is quite strange that the ioctl succeeds but err is set.
I know that in ieee80211_ioctl(), this particular error value
is checked. But is there a better to communicate this? Or
is it necessary to return this value in this function? If the
desired ESSID is changed, should the driver do something about
it? It seems that the ENETRESET "error" is just ignored. I
that these questions also apply to the other wifi_cfg_XX().
124: Do we need to make sure that outp->wldp_buf is big enough?
If not, please add a comment explaining that.
130: Same comment as in line 98.
143-189: This function is almost identical to wifi_cfg_essid(). So
the above questions also apply to this function.
206, 212, 219, 226: Please add an empty line.
301: Do we need to make sure that the passed in inp->wldp_buf has
enough room for MAX_NWEPKEYS? If not, please add a comment
explaining that.
576: Please add a comment explaining that each "rate" is a uint8_t.
This is why the memory allocation is done that way.
595-608: I suspect that the code can be simplified. Firstly, I think
it does not need to allocate srates. Just use the destination
buffer wl_rates_rates as the scratch. Go over ic_sup_rates
and check if the rate is already in the scratch. If not, add
it in sequence. If yet, skip. Then the loop 584-591 can
be avoided.
BTW, does the code need to make sure that wldp_buf can hold
all the rates? If not, please explain.
672: So if drate is not supported in the negotiated rate set (in_rates)
and ic->ic_state == IEEE80211_S_RUN, the code will continue in 676.
And if it happens that the drate is in the supported rate set by
the driver, the ioctl will return with no error. Is this the correct
behavior? The driver cannot really use the rate being set.
681: Can found be a boolean_t?
721: I guess a comment is needed to explain what the code is trying
to do here. Why rescale the RSSI value to [0, MAX_RSSI]?
749: Will WAIT_SCAN_MAX be better to be a global so that it can be
changed in case of a problem in production?
760: Should 200000 be configurable also? And is it better for
wifi_wait_scan() to take an argument which specifies the max
time to wait? And why can't cv_timedwait_sig() be used for this?
Please add some comment explaining the reason.
768: Please add a comment explaining that this function is the call back
for ieee80211_iterate_nodes() used in wifi_cfg_esslist() to get
all the nodes. The comment should also explain the argument passed
in.
777: Please add a comment saying that it is assumed the passed in
buffer is MAX_BUF_LEN large.
810, 818, 831: Please add an empty line.
887: Please add an empty line.
887: The code does nothing to change the state if the current state
is not IEEE80211_S_INIT. Is it correct? So if the current state
is IEEE80211_S_RUN, should the code just return? Or should the
code continue to initiate a scan? Please add some comment
explaining what the code tried to do.
895: Please explain why the state needs to be changed back to
IEEE80211_S_INIT.
963, 1030-1032: Why can't an mblk be allocated instead and pass in each
wifi_cfg_XX()? Then there is no need to kmem_zalloc() and
them kmem_free(). If this means that a MAX_BUF_LEN mblk needs
to be allocated, why can't each wifi_cfg_XX() allocate its
own mblk and pass back to the wifi_cfg_getset()? I think
mi_mpprintf_putc() should not be used here. It is intended
to add one more character to an mblk, but not to be used
as a copy function.
1082: Can secpolicy_net_config be NULL?
_______________________________________________
networking-discuss mailing list
[email protected]
