Received-SPF: pass (google.com: domain of linux-wireless+bounces-8860-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <68a33290c667b8e31c4c19f7b2f3c614a124845f.camel@sipsolutions.net>
Subject: Re: [PATCH v2] iw: Scan: Add EHT beacon info support
From: Johannes Berg <johannes@sipsolutions.net>
To: Dylan Eskew <dylan.eskew@candelatech.com>
Cc: linux-wireless@vger.kernel.org
Date: Wed, 12 Jun 2024 14:18:11 +0200
In-Reply-To: <20240318215733.112425-1-dylan.eskew@candelatech.com>
References: <20240318215733.112425-1-dylan.eskew@candelatech.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.2 (3.52.2-1.fc40) 
Precedence: bulk
MIME-Version: 1.0

Hi,

Sorry this took forever to bubble up on my list again ...

> +static void print_eht_capa(const uint8_t type, uint8_t len, const uint8_=
t *data,
> +			   const struct print_ies_data *ie_buffer)
> +{
> +	bool is_he =3D false;
> +	const __u16 he_phy_cap[6] =3D { 0 };
> +	unsigned char *ie =3D ie_buffer->ie;
> +	int ielen =3D ie_buffer->ielen;
> +
> +	while (ielen >=3D 2 && ielen >=3D ie[1]) {
> +		if (ie[0] =3D=3D 255 && ie[2] =3D=3D 35) {
> +			is_he =3D true;
> +			break;
> +		}
> +		ielen -=3D ie[1] + 2;
> +		ie +=3D ie[1] + 2;
> +	}
> +
> +	if (is_he) {
> +		memcpy(&((__u8 *)he_phy_cap)[0],
> +		       ie + 9, 12);
> +	}

Hm, at the very least, that could be a helper; at first I was wondering
what you even did there :)

Also, even if I see that this exists in print_capabilities(), I'm really
not convinced this is a good idea. In the print_capabilities() case it
should probably just print max A-MSDU anyway (or "(reserved for non-
VHT)" with it perhaps). Either way, we end up double-parsing this which
isn't great...

Also, you go to great lengths to get it, and then comment out the usage
of it below?


Not really sure what we should do instead - perhaps more 'elems' style
parsing like mac80211 does, and then print that way. Clearly we can't
simply iterate any more.

> @@ -2639,8 +2667,7 @@ static int print_bss_handler(struct nl_msg *msg, vo=
id *arg)
>  				       nla_len(ies)))))
>  			printf("\tInformation elements from Probe Response "
>  			       "frame:\n");
> -		print_ies(nla_data(ies), nla_len(ies),
> -			  params->unknown, params->type);
> +		print_ies(nla_data(ies), nla_len(ies), params->unknown, params->type);

why?

> -	if (!(he_phy_cap[0] & ((BIT(2) | BIT(3) | BIT(4)) << 8))){
> +	/*if (!(he_phy_cap[0] & ((BIT(2) | BIT(3) | BIT(4)) << 8))){

Don't leave commented out stuff.

johannes