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
Subject: Re: [PATCH net-next 0/6] net/ncsi: Allow enabling multiple packages & channels
Date: Fri, 19 Oct 2018 10:05:09 +1100 [thread overview]
Message-ID: <7f3744217190fe94bb1df879fa107593e911d7b5.camel@mendozajonas.com> (raw)
In-Reply-To: <20181018.155647.1045018243241594303.davem@davemloft.net>
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
next prev parent reply other threads:[~2018-10-18 23:05 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-18 3:59 [PATCH net-next 0/6] net/ncsi: Allow enabling multiple packages & channels Samuel Mendoza-Jonas
2018-10-18 3:59 ` [PATCH net-next 1/6] net/ncsi: Don't enable all channels when HWA available Samuel Mendoza-Jonas
2018-10-18 3:59 ` [PATCH net-next 2/6] net/ncsi: Probe single packages to avoid conflict Samuel Mendoza-Jonas
2018-10-18 3:59 ` [PATCH net-next 3/6] net/ncsi: Don't deselect package in suspend if active Samuel Mendoza-Jonas
2018-10-18 3:59 ` [PATCH net-next 4/6] net/ncsi: Don't mark configured channels inactive Samuel Mendoza-Jonas
2018-10-18 3:59 ` [PATCH net-next 5/6] net/ncsi: Reset channel state in ncsi_start_dev() Samuel Mendoza-Jonas
2018-10-30 21:26 ` Justin.Lee1
2018-11-01 22:53 ` Samuel Mendoza-Jonas
2018-11-05 18:01 ` Justin.Lee1
2018-11-06 0:28 ` Samuel Mendoza-Jonas
2018-11-06 17:27 ` Justin.Lee1
2018-10-18 3:59 ` [PATCH net-next 6/6] net/ncsi: Configure multi-package, multi-channel modes with failover Samuel Mendoza-Jonas
2018-10-18 22:56 ` [PATCH net-next 0/6] net/ncsi: Allow enabling multiple packages & channels David Miller
2018-10-18 23:05 ` Samuel Mendoza-Jonas [this message]
2018-10-19 21:38 ` Justin.Lee1
2018-10-22 22:24 ` Samuel Mendoza-Jonas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7f3744217190fe94bb1df879fa107593e911d7b5.camel@mendozajonas.com \
--to=sam@mendozajonas.com \
--cc=Justin.Lee1@Dell.com \
--cc=davem@davemloft.net \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=openbmc@lists.ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).