Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <7f3744217190fe94bb1df879fa107593e911d7b5.camel@mendozajonas.com>
Subject: Re: [PATCH net-next 0/6] net/ncsi: Allow enabling multiple packages
 & channels
From:   Samuel Mendoza-Jonas <sam@mendozajonas.com>
To:     David Miller <davem@davemloft.net>
Cc:     netdev@vger.kernel.org, Justin.Lee1@Dell.com,
        linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org
Date:   Fri, 19 Oct 2018 10:05:09 +1100
In-Reply-To: <20181018.155647.1045018243241594303.davem@davemloft.net>
References: <20181018035917.19413-1-sam@mendozajonas.com>
         <20181018.155647.1045018243241594303.davem@davemloft.net>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.30.1 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 2018-10-18 at 15:56 -0700, David Miller wrote:
> From: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> Date: Thu, 18 Oct 2018 14:59:11 +1100
> 
> > This series extends the NCSI driver to configure multiple packages
> > and/or channels simultaneously. Since the RFC series this includes a few
> > extra changes to fix areas in the driver that either made this harder or
> > were roadblocks due to deviations from the NCSI specification.
> > 
> > Patches 1 & 2 fix two issues where the driver made assumptions about the
> > capabilities of the NCSI topology.
> > Patches 3 & 4 change some internal semantics slightly to make multi-mode
> > easier.
> > Patch 5 introduces a cleaner way of reconfiguring the NCSI configuration
> > and keeping track of channel states.
> > Patch 6 implements the main multi-package/multi-channel configuration,
> > configured via the Netlink interface.
> > 
> > Readers who have an interesting NCSI setup - especially multi-package
> > with HWA - please test! I think I've covered all permutations but I
> > don't have infinite hardware to test on.
> 
> This doesn't apply cleanly to net-next.  Does it depend upon changes
> applied elsewhere?  You must always make that explicit.

Ah, my mistake; I hadn't updated my net-next branch recently enough and
missed Vijay's OEM command patch. Will rebase.

> 
> Also, please explain this locking in ncsi_reset_dev():
> 
> +       NCSI_FOR_EACH_PACKAGE(ndp, np) {
> +               NCSI_FOR_EACH_CHANNEL(np, nc) {
> +                       spin_lock_irqsave(&nc->lock, flags);
> +                       enabled = nc->monitor.enabled;
> +                       state = nc->state;
> +                       spin_unlock_irqrestore(&nc->lock, flags);
> +
> +                       if (enabled)
> +                               ncsi_stop_channel_monitor(nc);
> +                       if (state == NCSI_CHANNEL_ACTIVE) {
> +                               active = nc;
> +                               break;
> +                       }
> 
> Is that really protecting anything?
> 
> Right after you drop np->lock those two values can change, the state
> of the 'nc' can change such that it isn't NCSI_CHANNEL_ACTIVE anymore
> etc.
> 
> At best this locking makes sure thatn enabled and state are consistent
> with respect to eachother, only.  It doesn't guarantee anything about
> the stability of the state of the object at all, and it can change
> right from under you.

And you've caught this correctly, I'll fix this up in the rebase to
protect actually checking the channel/monitor state.

Thanks,
Sam