Re: [PATCH 04/19] wilc: add host_interface.c

[<Adham.Abozaeid@microchip.com>]

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 *)&param->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 =  &param->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 =  &param->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
>