Re: [PATCH net-next 0/6] net/ncsi: Allow enabling multiple packages & channels

From: Samuel Mendoza-Jonas
Date: Thu Oct 18 2018 - 19:12:54 EST


On Thu, 2018-10-18 at 15:56 -0700, David Miller wrote:
> From: Samuel Mendoza-Jonas <sam@xxxxxxxxxxxxxxxx>
> 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