From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757319AbcG1Lfl (ORCPT ); Thu, 28 Jul 2016 07:35:41 -0400 Received: from mailout2.hostsharing.net ([83.223.90.233]:52225 "EHLO mailout2.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757031AbcG1Lfd (ORCPT ); Thu, 28 Jul 2016 07:35:33 -0400 Date: Thu, 28 Jul 2016 13:35:50 +0200 From: Lukas Wunner To: Amir Levy Cc: andreas.noever@gmail.com, gregkh@linuxfoundation.org, bhelgaas@google.com, corbet@lwn.net, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, netdev@vger.kernel.org, linux-doc@vger.kernel.org, thunderbolt-linux@intel.com, mika.westerberg@intel.com, tomas.winkler@intel.com Subject: Re: [PATCH v5 5/8] thunderbolt: Networking state machine Message-ID: <20160728113550.GA1701@wunner.de> References: <1469693721-5641-1-git-send-email-amir.jer.levy@intel.com> <1469693721-5641-6-git-send-email-amir.jer.levy@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1469693721-5641-6-git-send-email-amir.jer.levy@intel.com> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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