On 10/08/2018 07:31 AM, Johannes Berg wrote:
>
>> + *currbyte = (u32)0 & DRV_HANDLER_MASK;
>
> You do this a few times, not sure what it's supposed to achieve?
>
>> + if (param->flag & RETRY_LONG) {
>> + u16 limit = param->long_retry_limit;
>> +
>> + if (limit > 0 && limit < 256) {
>> + wid_list[i].id = WID_LONG_RETRY_LIMIT;
>> + wid_list[i].val = (s8 *)¶m->long_retry_limit;
>> + wid_list[i].type = WID_SHORT;
>> + wid_list[i].size = sizeof(u16);
>> + hif_drv->cfg_values.long_retry_limit = limit;
>> + } else {
>> + netdev_err(vif->ndev, "Range(1~256) over\n");
>> + goto unlock;
>> + }
>> + i++;
>> + }
>
> So ... can anyone tell me why there's a complete driver-internal
> messaging infrastructure in this, that even suppresses errors like here
> (out of range just results in a message rather than returning an error
> to wherever it originated)?
>
Agree. parameter validation can be done before scheduling the work, and hence appropriate error can be returned to caller .
> It almost *seems* like it's a to-device infrastructure, but it can't be
> since it uses host pointers everywhere?
>
> I think this code would be far better off without the "bounce in driver
> to resolve host pointers" step.
If I got your point correctly, you are referring to the lines that stores the parameters into the hif_drv->cfg_values.
I agree, the cfg_values isn't read from anywhere in the driver, so can be removed
>> + if (conn_attr->ssid) {
>> + memcpy(cur_byte, conn_attr->ssid, conn_attr->ssid_len);
>> + cur_byte[conn_attr->ssid_len] = '\0';
>> + }
>> + cur_byte += MAX_SSID_LEN;
>
> again, SSIDs are not 0-terminated strings
For this specific code, the device requires the ssid to be null terminated, since it doesn't receive the ssid_len parameter.
For other ssid references in the driver, the null termination can be removed.
>
>> +static void host_int_fill_join_bss_param(struct join_bss_param *param, u8 *ies,
>> + u16 *out_index, u8 *pcipher_tc,
>> + u8 *auth_total_cnt, u32 tsf_lo,
>> + u8 *rates_no)
>> +{
>> + u8 ext_rates_no;
>> + u16 offset;
>> + u8 pcipher_cnt;
>> + u8 auth_cnt;
>> + u8 i, j;
>> + u16 index = *out_index;
>> +
>> + if (ies[index] == WLAN_EID_SUPP_RATES) {
>> + *rates_no = ies[index + 1];
>> + param->supp_rates[0] = *rates_no;
>> + index += 2;
>> +
>> + for (i = 0; i < *rates_no; i++)
>> + param->supp_rates[i + 1] = ies[index + i];
>> +
>> + index += *rates_no;
>> + } else if (ies[index] == WLAN_EID_EXT_SUPP_RATES) {
>> + ext_rates_no = ies[index + 1];
>> + if (ext_rates_no > (MAX_RATES_SUPPORTED - *rates_no))
>> + param->supp_rates[0] = MAX_RATES_SUPPORTED;
>> + else
>> + param->supp_rates[0] += ext_rates_no;
>> + index += 2;
>> + for (i = 0; i < (param->supp_rates[0] - *rates_no); i++)
>> + param->supp_rates[*rates_no + i + 1] = ies[index + i];
>> +
>> + index += ext_rates_no;
>> + } else if (ies[index] == WLAN_EID_HT_CAPABILITY) {
>> + param->ht_capable = true;
>> + index += ies[index + 1] + 2;
>> + } else if ((ies[index] == WLAN_EID_VENDOR_SPECIFIC) &&
>> + (ies[index + 2] == 0x00) && (ies[index + 3] == 0x50) &&
>> + (ies[index + 4] == 0xF2) && (ies[index + 5] == 0x02) &&
>> + ((ies[index + 6] == 0x00) || (ies[index + 6] == 0x01)) &&
>> + (ies[index + 7] == 0x01)) {
>> + param->wmm_cap = true;
>> +
>> + if (ies[index + 8] & BIT(7))
>> + param->uapsd_cap = true;
>> + index += ies[index + 1] + 2;
>> + } else if ((ies[index] == WLAN_EID_VENDOR_SPECIFIC) &&
>> + (ies[index + 2] == 0x50) && (ies[index + 3] == 0x6f) &&
>> + (ies[index + 4] == 0x9a) &&
>> + (ies[index + 5] == 0x09) && (ies[index + 6] == 0x0c)) {
>> + u16 p2p_cnt;
>> +
>> + param->tsf = tsf_lo;
>> + param->noa_enabled = 1;
>> + param->idx = ies[index + 9];
>> +
>> + if (ies[index + 10] & BIT(7)) {
>> + param->opp_enabled = 1;
>> + param->ct_window = ies[index + 10];
>> + } else {
>> + param->opp_enabled = 0;
>> + }
>> +
>> + param->cnt = ies[index + 11];
>> + p2p_cnt = index + 12;
>> +
>> + memcpy(param->duration, ies + p2p_cnt, 4);
>> + p2p_cnt += 4;
>> +
>> + memcpy(param->interval, ies + p2p_cnt, 4);
>> + p2p_cnt += 4;
>> +
>> + memcpy(param->start_time, ies + p2p_cnt, 4);
>> +
>> + index += ies[index + 1] + 2;
>> + } else if ((ies[index] == WLAN_EID_RSN) ||
>> + ((ies[index] == WLAN_EID_VENDOR_SPECIFIC) &&
>> + (ies[index + 2] == 0x00) &&
>> + (ies[index + 3] == 0x50) && (ies[index + 4] == 0xF2) &&
>> + (ies[index + 5] == 0x01))) {
>> + u16 rsn_idx = index;
>> +
>> + if (ies[rsn_idx] == WLAN_EID_RSN) {
>> + param->mode_802_11i = 2;
>> + } else {
>> + if (param->mode_802_11i == 0)
>> + param->mode_802_11i = 1;
>> + rsn_idx += 4;
>> + }
>> +
>> + rsn_idx += 7;
>> + param->rsn_grp_policy = ies[rsn_idx];
>> + rsn_idx++;
>> + offset = ies[rsn_idx] * 4;
>> + pcipher_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
>> + rsn_idx += 2;
>> +
>> + i = *pcipher_tc;
>> + j = 0;
>> + for (; i < (pcipher_cnt + *pcipher_tc) && i < 3; i++, j++) {
>> + u8 *policy = ¶m->rsn_pcip_policy[i];
>> +
>> + *policy = ies[rsn_idx + ((j + 1) * 4) - 1];
>> + }
>> +
>> + *pcipher_tc += pcipher_cnt;
>> + rsn_idx += offset;
>> +
>> + offset = ies[rsn_idx] * 4;
>> +
>> + auth_cnt = (ies[rsn_idx] > 3) ? 3 : ies[rsn_idx];
>> + rsn_idx += 2;
>> + i = *auth_total_cnt;
>> + j = 0;
>> + for (; i < (*auth_total_cnt + auth_cnt); i++, j++) {
>> + u8 *policy = ¶m->rsn_auth_policy[i];
>> +
>> + *policy = ies[rsn_idx + ((j + 1) * 4) - 1];
>> + }
>> +
>> + *auth_total_cnt += auth_cnt;
>> + rsn_idx += offset;
>> +
>> + if (ies[index] == WLAN_EID_RSN) {
>> + param->rsn_cap[0] = ies[rsn_idx];
>> + param->rsn_cap[1] = ies[rsn_idx + 1];
>> + rsn_idx += 2;
>> + }
>> + param->rsn_found = true;
>> + index += ies[index + 1] + 2;
>> + } else {
>> + index += ies[index + 1] + 2;
>> + }
>> +
>> + *out_index = index;
>> +}
>
> Again, use actual kernel infrastructure for much of this.
>
>> + cur_byte = wid.val;
>> + *cur_byte++ = (param->interval & 0xFF);
>> + *cur_byte++ = ((param->interval >> 8) & 0xFF);
>> + *cur_byte++ = ((param->interval >> 16) & 0xFF);
>> + *cur_byte++ = ((param->interval >> 24) & 0xFF);
>
> put_unaligned_le32().
>
Agree
>> + *cur_byte++ = param->aid & 0xFF;
>> + *cur_byte++ = (param->aid >> 8) & 0xFF;
>
> and so on
>
> but then again, I just suggested to not have these "pack" functions to
> start with, or at least not in this way, since it just means you first
> pack everything into host structs, and then repack everything again into
> firmware format ...
>
Agree.
Instead of packing the parameters in host structures like struct add_sta_param, then repacking it in the device format, it can use struct station_parameters and pack them directly into the device format
>
> So far I guess I'd say:
> * use more kernel infra, in particular {get,put}_unaligned_le{16,32}
> * name your device/driver-specific constants better, rather than things
> like "SET_CFG" which leave everyone wondering if it's specific to
> this driver or something from elsewhere
>
> johannes
>
