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

From	Lukas Wunner <lukas@wunner.de>
Newsgroups	linux.kernel
Subject	Re: [PATCH v5 5/8] thunderbolt: Networking state machine
Date	2016-07-28 13:40 +0200
Message-ID	<rZRPr-5E2-1@gated-at.bofh.it> (permalink)
References	<rZOHT-3yL-3@gated-at.bofh.it> <rZOHT-3yL-7@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

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

Thread

[PATCH v5 0/8] thunderbolt: Introducing Thunderbolt(TM) networking Amir Levy <amir.jer.levy@intel.com> - 2016-07-28 10:20 +0200
  [PATCH v5 2/8] thunderbolt: Updating the register definitions Amir Levy <amir.jer.levy@intel.com> - 2016-07-28 10:20 +0200
  [PATCH v5 8/8] thunderbolt: Adding maintainer entry Amir Levy <amir.jer.levy@intel.com> - 2016-07-28 10:20 +0200
  [PATCH v5 7/8] thunderbolt: Networking doc Amir Levy <amir.jer.levy@intel.com> - 2016-07-28 10:20 +0200
  Re: [PATCH v5 5/8] thunderbolt: Networking state machine Lukas Wunner <lukas@wunner.de> - 2016-07-28 13:40 +0200
    RE: [PATCH v5 5/8] thunderbolt: Networking state machine "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-31 14:00 +0200
  Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM  (firmware) Stephen Hemminger <stephen@networkplumber.org> - 2016-07-29 23:10 +0200
    Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM (firmware) Greg KH <gregkh@linuxfoundation.org> - 2016-07-29 23:50 +0200
      RE: [PATCH v5 4/8] thunderbolt: Communication with the ICM  (firmware) "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-31 12:20 +0200
  Re: [PATCH v5 4/8] thunderbolt: Communication with the ICM  (firmware) Stephen Hemminger <stephen@networkplumber.org> - 2016-07-29 23:10 +0200
    RE: [PATCH v5 4/8] thunderbolt: Communication with the ICM  (firmware) "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-31 14:10 +0200
  Re: [PATCH v5 6/8] thunderbolt: Networking transmit and receive Stephen Hemminger <stephen@networkplumber.org> - 2016-07-29 23:20 +0200
    RE: [PATCH v5 6/8] thunderbolt: Networking transmit and receive "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-31 12:20 +0200
      Re: [PATCH v5 6/8] thunderbolt: Networking transmit and receive David Miller <davem@davemloft.net> - 2016-07-31 20:50 +0200
        RE: [PATCH v5 6/8] thunderbolt: Networking transmit and receive "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-08-01 06:40 +0200

csiph-web