linux-kernel - Re: [PATCH v5 5/8] thunderbolt: Networking state machine

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 28 Jul 2016 13:35:50 +0200
From:	Lukas Wunner <lukas@...ner.de>
To:	Amir Levy <amir.jer.levy@...el.com>
Cc:	andreas.noever@...il.com, gregkh@...uxfoundation.org,
	bhelgaas@...gle.com, corbet@....net, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, thunderbolt-linux@...el.com,
	mika.westerberg@...el.com, tomas.winkler@...el.com
Subject: Re: [PATCH v5 5/8] thunderbolt: Networking state machine

On Thu, Jul 28, 2016 at 11:15:18AM +0300, Amir Levy wrote:
> +static void nhi_handle_notification_msg(struct tbt_nhi_ctxt *nhi_ctxt,
> +					const u8 *msg)
> +{
> +	struct port_net_dev *port;
> +	u8 port_num;
> +
> +#define INTER_DOMAIN_LINK_SHIFT 0
> +#define INTER_DOMAIN_LINK_MASK	GENMASK(2, INTER_DOMAIN_LINK_SHIFT)
> +	switch (msg[3]) {
> +
> +	case NC_INTER_DOMAIN_CONNECTED:
> +		port_num = PORT_NUM_FROM_MSG(msg[5]);
> +#define INTER_DOMAIN_APPROVED BIT(3)
> +		if (likely(port_num < nhi_ctxt->num_ports)) {
> +			if (!(msg[5] & INTER_DOMAIN_APPROVED))

I find these interspersed #defines make the code hard to read,
but maybe that's just me.

> +				nhi_ctxt->net_devices[
> +					port_num].medium_sts =

Looks like a carriage return slipped in here.

In patch [4/8], I've found it a bit puzzling that FW->SW responses and
FW->SW notifications are defined in icm_nhi.c, whereas SW->FW commands
are defined in net.h. It would perhaps be more logical to have them
all in the header file. The FW->SW responses and SW->FW commands are
almost identical, there are odd spelling differences (CONNEXION vs.
CONNECTION).

It would probably be good to explain the PDF acronym somewhere.

I've skimmed over all patches in the series, too superficial to provide
a Reviewed-by, it's just too much code to review thoroughly and I also
lack the hardware to test it, but broadly this LGTM.

Thanks,

Lukas