Messages in this thread | | | Subject | Re: [PATCH net-next 0/6] net/ncsi: Allow enabling multiple packages & channels | From | Samuel Mendoza-Jonas <> | Date | Fri, 19 Oct 2018 10:05:09 +1100 |
| |
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
| |