Message ID | 20181018035917.19413-1-sam@mendozajonas.com |
---|---|
Headers | show |
Series | net/ncsi: Allow enabling multiple packages & channels | expand |
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. 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.
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
Hi Sam, When I run the testing (enable multi-package (package 0 and 1) and multi-channel (channel 0 and 1 on both packages)), I see there are two channels with TX enable. When I unplug/plug network cable, it seems the behavior is to keep one TX per package, is it the behavior you expect? All channels are behind the same eth2, I would expect there is only one active TX. I create the below file to expose driver's status. RX and TX are printing from the follow variables. nc->modes[NCSI_MODE_ENABLE].enable, nc->modes[NCSI_MODE_TX_ENABLE].enable, cat /sys/kernel/debug/ncsi_protocol/ncsi_device_status; IFIDX IFNAME NAME PID CID RX TX MP MC WP WC PS RU CR NQ ========================================================= 2 eth2 ncsi0 000 000 1 1 1 1 1 1 1 1 1 0 2 eth2 ncsi1 000 001 1 0 1 1 1 1 1 1 1 0 2 eth2 ncsi2 001 000 1 1 1 1 1 1 1 1 1 0 2 eth2 ncsi3 001 001 1 0 1 1 1 1 1 1 1 0 ========================================================= M: Multi-mode P: Package ID W: Whitelist C: Channel ID PS: Poll Status RU: Running CR: Carrier OK NQ: Queue Stopped Thanks, Justin 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.
On Fri, 2018-10-19 at 21:38 +0000, Justin.Lee1@Dell.com wrote: > Hi Sam, > > When I run the testing (enable multi-package (package 0 and 1) and > multi-channel (channel 0 and 1 on both packages)), I see there are two channels > with TX enable. > > When I unplug/plug network cable, it seems the behavior is to keep one TX per > package, is it the behavior you expect? All channels are behind the same eth2, > I would expect there is only one active TX. > > I create the below file to expose driver's status. RX and TX are printing > from the follow variables. > nc->modes[NCSI_MODE_ENABLE].enable, > nc->modes[NCSI_MODE_TX_ENABLE].enable, > > cat /sys/kernel/debug/ncsi_protocol/ncsi_device_status; > IFIDX IFNAME NAME PID CID RX TX MP MC WP WC PS RU CR NQ > ========================================================= > 2 eth2 ncsi0 000 000 1 1 1 1 1 1 1 1 1 0 > 2 eth2 ncsi1 000 001 1 0 1 1 1 1 1 1 1 0 > 2 eth2 ncsi2 001 000 1 1 1 1 1 1 1 1 1 0 > 2 eth2 ncsi3 001 001 1 0 1 1 1 1 1 1 1 0 > ========================================================= > M: Multi-mode P: Package ID > W: Whitelist C: Channel ID > PS: Poll Status > RU: Running > CR: Carrier OK > NQ: Queue Stopped Hi Justin, Thanks for testing; this is probably an oversight in ncsi_channel_is_tx(), where channels from a package other than the current one could be ignored when checking for the current channel's link status. I'll update this to be more robust. Sam > > Thanks, > Justin > > > 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. > >