* [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
From: Richard Cochran @ 2014-11-17 23:06 UTC (permalink / raw)
To: netdev
Cc: David Miller, bruce.w.allan, Jacob Keller, Jeff Kirsher,
John Ronciak, Matthew Vick
In-Reply-To: <cover.1416265321.git.richardcochran@gmail.com>
The i210 device offers a number of special PTP Hardware Clock features on
the Software Defined Pins (SDPs). This patch adds support for two of the
possible functions, namely time stamping external events, and periodic
output signals.
The assignment of PHC functions to the four SDP can be freely chosen by
the user.
Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
drivers/net/ethernet/intel/igb/igb.h | 9 ++
drivers/net/ethernet/intel/igb/igb_main.c | 47 ++++++-
drivers/net/ethernet/intel/igb/igb_ptp.c | 219 ++++++++++++++++++++++++++++-
3 files changed, 269 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 82d891e..f6aca7c 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -343,6 +343,9 @@ struct hwmon_buff {
};
#endif
+#define IGB_N_EXTTS 2
+#define IGB_N_PEROUT 2
+#define IGB_N_SDP 4
#define IGB_RETA_SIZE 128
/* board specific private data structure */
@@ -439,6 +442,12 @@ struct igb_adapter {
u32 tx_hwtstamp_timeouts;
u32 rx_hwtstamp_cleared;
+ struct ptp_pin_desc sdp_config[IGB_N_SDP];
+ struct {
+ struct timespec start;
+ struct timespec period;
+ } perout[IGB_N_PEROUT];
+
char fw_version[32];
#ifdef CONFIG_IGB_HWMON
struct hwmon_buff *igb_hwmon_buff;
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 9412661..3a25661 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5387,7 +5387,8 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
{
struct e1000_hw *hw = &adapter->hw;
struct ptp_clock_event event;
- u32 tsicr = rd32(E1000_TSICR);
+ struct timespec ts;
+ u32 tsauxc, sec, nsec, tsicr = rd32(E1000_TSICR);
if (tsicr & TSINTR_SYS_WRAP) {
event.type = PTP_CLOCK_PPS;
@@ -5400,6 +5401,50 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
/* retrieve hardware timestamp */
schedule_work(&adapter->ptp_tx_work);
}
+
+ if (tsicr & TSINTR_TT0) {
+ spin_lock(&adapter->tmreg_lock);
+ ts = timespec_add(adapter->perout[0].start,
+ adapter->perout[0].period);
+ wr32(E1000_TRGTTIML0, ts.tv_nsec);
+ wr32(E1000_TRGTTIMH0, ts.tv_sec);
+ tsauxc = rd32(E1000_TSAUXC);
+ tsauxc |= TSAUXC_EN_TT0;
+ wr32(E1000_TSAUXC, tsauxc);
+ adapter->perout[0].start = ts;
+ spin_unlock(&adapter->tmreg_lock);
+ }
+
+ if (tsicr & TSINTR_TT1) {
+ spin_lock(&adapter->tmreg_lock);
+ ts = timespec_add(adapter->perout[1].start,
+ adapter->perout[1].period);
+ wr32(E1000_TRGTTIML1, ts.tv_nsec);
+ wr32(E1000_TRGTTIMH1, ts.tv_sec);
+ tsauxc = rd32(E1000_TSAUXC);
+ tsauxc |= TSAUXC_EN_TT1;
+ wr32(E1000_TSAUXC, tsauxc);
+ adapter->perout[1].start = ts;
+ spin_unlock(&adapter->tmreg_lock);
+ }
+
+ if (tsicr & TSINTR_AUTT0) {
+ nsec = rd32(E1000_AUXSTMPL0);
+ sec = rd32(E1000_AUXSTMPH0);
+ event.type = PTP_CLOCK_EXTTS;
+ event.index = 0;
+ event.timestamp = sec * 1000000000ULL + nsec;
+ ptp_clock_event(adapter->ptp_clock, &event);
+ }
+
+ if (tsicr & TSINTR_AUTT1) {
+ nsec = rd32(E1000_AUXSTMPL1);
+ sec = rd32(E1000_AUXSTMPH1);
+ event.type = PTP_CLOCK_EXTTS;
+ event.index = 1;
+ event.timestamp = sec * 1000000000ULL + nsec;
+ ptp_clock_event(adapter->ptp_clock, &event);
+ }
}
static irqreturn_t igb_msix_other(int irq, void *data)
diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
index 70d3933..37b9fe6 100644
--- a/drivers/net/ethernet/intel/igb/igb_ptp.c
+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
@@ -360,16 +360,203 @@ static int igb_ptp_settime_i210(struct ptp_clock_info *ptp,
return 0;
}
+static void igb_pin_direction(int pin, int input, u32 *ctrl, u32 *ctrl_ext)
+{
+ u32 *ptr = pin < 2 ? ctrl : ctrl_ext;
+ u32 mask[IGB_N_SDP] = {
+ E1000_CTRL_SDP0_DIR,
+ E1000_CTRL_SDP1_DIR,
+ E1000_CTRL_EXT_SDP2_DIR,
+ E1000_CTRL_EXT_SDP3_DIR,
+ };
+
+ if (input)
+ *ptr &= ~mask[pin];
+ else
+ *ptr |= mask[pin];
+}
+
+static void igb_pin_extts(struct igb_adapter *igb, int chan, int pin)
+{
+ struct e1000_hw *hw = &igb->hw;
+ u32 aux0_sel_sdp[IGB_N_SDP] = {
+ AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
+ };
+ u32 aux1_sel_sdp[IGB_N_SDP] = {
+ AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
+ };
+ u32 ts_sdp_en[IGB_N_SDP] = {
+ TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
+ };
+ u32 ctrl, ctrl_ext, tssdp = 0;
+
+ ctrl = rd32(E1000_CTRL);
+ ctrl_ext = rd32(E1000_CTRL_EXT);
+ tssdp = rd32(E1000_TSSDP);
+
+ igb_pin_direction(pin, 1, &ctrl, &ctrl_ext);
+
+ /* Make sure this pin is not enabled as an ouput. */
+ tssdp &= ~ts_sdp_en[pin];
+
+ if (chan == 1) {
+ tssdp &= ~AUX1_SEL_SDP3;
+ tssdp |= aux1_sel_sdp[pin] | AUX1_TS_SDP_EN;
+ } else {
+ tssdp &= ~AUX0_SEL_SDP3;
+ tssdp |= aux0_sel_sdp[pin] | AUX0_TS_SDP_EN;
+ }
+
+ wr32(E1000_TSSDP, tssdp);
+ wr32(E1000_CTRL, ctrl);
+ wr32(E1000_CTRL_EXT, ctrl_ext);
+}
+
+static void igb_pin_perout(struct igb_adapter *igb, int chan, int pin)
+{
+ struct e1000_hw *hw = &igb->hw;
+ u32 aux0_sel_sdp[IGB_N_SDP] = {
+ AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
+ };
+ u32 aux1_sel_sdp[IGB_N_SDP] = {
+ AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
+ };
+ u32 ts_sdp_en[IGB_N_SDP] = {
+ TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
+ };
+ u32 ts_sdp_sel_tt0[IGB_N_SDP] = {
+ TS_SDP0_SEL_TT0, TS_SDP1_SEL_TT0,
+ TS_SDP2_SEL_TT0, TS_SDP3_SEL_TT0,
+ };
+ u32 ts_sdp_sel_tt1[IGB_N_SDP] = {
+ TS_SDP0_SEL_TT1, TS_SDP1_SEL_TT1,
+ TS_SDP2_SEL_TT1, TS_SDP3_SEL_TT1,
+ };
+ u32 ts_sdp_sel_clr[IGB_N_SDP] = {
+ TS_SDP0_SEL_FC1, TS_SDP1_SEL_FC1,
+ TS_SDP2_SEL_FC1, TS_SDP3_SEL_FC1,
+ };
+ u32 ctrl, ctrl_ext, tssdp = 0;
+
+ ctrl = rd32(E1000_CTRL);
+ ctrl_ext = rd32(E1000_CTRL_EXT);
+ tssdp = rd32(E1000_TSSDP);
+
+ igb_pin_direction(pin, 0, &ctrl, &ctrl_ext);
+
+ /* Make sure this pin is not enabled as an input. */
+ if ((tssdp & AUX0_SEL_SDP3) == aux0_sel_sdp[pin])
+ tssdp &= ~AUX0_TS_SDP_EN;
+
+ if ((tssdp & AUX1_SEL_SDP3) == aux1_sel_sdp[pin])
+ tssdp &= ~AUX1_TS_SDP_EN;
+
+ tssdp &= ~ts_sdp_sel_clr[pin];
+ if (chan == 1)
+ tssdp |= ts_sdp_sel_tt1[pin];
+ else
+ tssdp |= ts_sdp_sel_tt0[pin];
+
+ tssdp |= ts_sdp_en[pin];
+
+ wr32(E1000_TSSDP, tssdp);
+ wr32(E1000_CTRL, ctrl);
+ wr32(E1000_CTRL_EXT, ctrl_ext);
+}
+
static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
struct ptp_clock_request *rq, int on)
{
struct igb_adapter *igb =
container_of(ptp, struct igb_adapter, ptp_caps);
struct e1000_hw *hw = &igb->hw;
+ u32 tsauxc, tsim, tsauxc_mask, tsim_mask, trgttiml, trgttimh;
unsigned long flags;
- u32 tsim;
+ struct timespec ts;
+ int pin;
+ s64 ns;
switch (rq->type) {
+ case PTP_CLK_REQ_EXTTS:
+ if (on) {
+ pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS,
+ rq->extts.index);
+ if (pin < 0)
+ return -EBUSY;
+ igb_pin_extts(igb, rq->extts.index, pin);
+ }
+ if (rq->extts.index == 1) {
+ tsauxc_mask = TSAUXC_EN_TS1;
+ tsim_mask = TSINTR_AUTT1;
+ } else {
+ tsauxc_mask = TSAUXC_EN_TS0;
+ tsim_mask = TSINTR_AUTT0;
+ }
+ spin_lock_irqsave(&igb->tmreg_lock, flags);
+ tsauxc = rd32(E1000_TSAUXC);
+ tsim = rd32(E1000_TSIM);
+ if (on) {
+ tsauxc |= tsauxc_mask;
+ tsim |= tsim_mask;
+ } else {
+ tsauxc &= ~tsauxc_mask;
+ tsim &= ~tsim_mask;
+ }
+ wr32(E1000_TSAUXC, tsauxc);
+ wr32(E1000_TSIM, tsim);
+ spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+ return 0;
+
+ case PTP_CLK_REQ_PEROUT:
+ if (on) {
+ pin = ptp_find_pin(igb->ptp_clock, PTP_PF_PEROUT,
+ rq->perout.index);
+ if (pin < 0)
+ return -EBUSY;
+ igb_pin_perout(igb, rq->perout.index, pin);
+ }
+ ts.tv_sec = rq->perout.period.sec;
+ ts.tv_nsec = rq->perout.period.nsec;
+ ns = timespec_to_ns(&ts);
+ ns = ns >> 1;
+ if (on && ns < 500000LL) {
+ /* 2k interrupts per second is an awful lot. */
+ return -EINVAL;
+ }
+ ts = ns_to_timespec(ns);
+ if (rq->perout.index == 1) {
+ tsauxc_mask = TSAUXC_EN_TT1;
+ tsim_mask = TSINTR_TT1;
+ trgttiml = E1000_TRGTTIML1;
+ trgttimh = E1000_TRGTTIMH1;
+ } else {
+ tsauxc_mask = TSAUXC_EN_TT0;
+ tsim_mask = TSINTR_TT0;
+ trgttiml = E1000_TRGTTIML0;
+ trgttimh = E1000_TRGTTIMH0;
+ }
+ spin_lock_irqsave(&igb->tmreg_lock, flags);
+ tsauxc = rd32(E1000_TSAUXC);
+ tsim = rd32(E1000_TSIM);
+ if (on) {
+ int i = rq->perout.index;
+ igb->perout[i].start.tv_sec = rq->perout.start.sec;
+ igb->perout[i].start.tv_nsec = rq->perout.start.nsec;
+ igb->perout[i].period.tv_sec = ts.tv_sec;
+ igb->perout[i].period.tv_nsec = ts.tv_nsec;
+ wr32(trgttiml, rq->perout.start.sec);
+ wr32(trgttimh, rq->perout.start.nsec);
+ tsauxc |= tsauxc_mask;
+ tsim |= tsim_mask;
+ } else {
+ tsauxc &= ~tsauxc_mask;
+ tsim &= ~tsim_mask;
+ }
+ wr32(E1000_TSAUXC, tsauxc);
+ wr32(E1000_TSIM, tsim);
+ spin_unlock_irqrestore(&igb->tmreg_lock, flags);
+ return 0;
+
case PTP_CLK_REQ_PPS:
spin_lock_irqsave(&igb->tmreg_lock, flags);
tsim = rd32(E1000_TSIM);
@@ -380,9 +567,6 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
wr32(E1000_TSIM, tsim);
spin_unlock_irqrestore(&igb->tmreg_lock, flags);
return 0;
-
- default:
- break;
}
return -EOPNOTSUPP;
@@ -394,6 +578,20 @@ static int igb_ptp_feature_enable(struct ptp_clock_info *ptp,
return -EOPNOTSUPP;
}
+static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
+ enum ptp_pin_function func, unsigned int chan)
+{
+ switch (func) {
+ case PTP_PF_NONE:
+ case PTP_PF_EXTTS:
+ case PTP_PF_PEROUT:
+ break;
+ case PTP_PF_PHYSYNC:
+ return -1;
+ }
+ return 0;
+}
+
/**
* igb_ptp_tx_work
* @work: pointer to work struct
@@ -784,6 +982,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
{
struct e1000_hw *hw = &adapter->hw;
struct net_device *netdev = adapter->netdev;
+ int i;
switch (hw->mac.type) {
case e1000_82576:
@@ -826,16 +1025,26 @@ void igb_ptp_init(struct igb_adapter *adapter)
break;
case e1000_i210:
case e1000_i211:
+ for (i = 0; i < IGB_N_SDP; i++) {
+ struct ptp_pin_desc *ppd = &adapter->sdp_config[i];
+ snprintf(ppd->name, sizeof(ppd->name), "SDP%d", i);
+ ppd->index = i;
+ ppd->func = PTP_PF_NONE;
+ }
snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr);
adapter->ptp_caps.owner = THIS_MODULE;
adapter->ptp_caps.max_adj = 62499999;
- adapter->ptp_caps.n_ext_ts = 0;
+ adapter->ptp_caps.n_ext_ts = IGB_N_EXTTS;
+ adapter->ptp_caps.n_per_out = IGB_N_PEROUT;
+ adapter->ptp_caps.n_pins = IGB_N_SDP;
adapter->ptp_caps.pps = 1;
+ adapter->ptp_caps.pin_config = adapter->sdp_config;
adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580;
adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
adapter->ptp_caps.gettime = igb_ptp_gettime_i210;
adapter->ptp_caps.settime = igb_ptp_settime_i210;
adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
+ adapter->ptp_caps.verify = igb_ptp_verify_pin;
/* Enable the timer functions by clearing bit 31. */
wr32(E1000_TSAUXC, 0x0);
break;
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH net-next 0/4] igb: auxiliary PHC functions for the i210.
From: Jeff Kirsher @ 2014-11-17 23:15 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, David Miller, bruce.w.allan, Jacob Keller, John Ronciak,
Matthew Vick
In-Reply-To: <cover.1416265321.git.richardcochran@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 890 bytes --]
On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> This patch series adds three features: time stamping external events,
> producing a periodic output signal, and an internal PPS event. The
> i210 PCIe card has a 6 pin header with SDP0-3, making it easy to try
> out this new functionality.
>
> An earlier version of this series was posted way back in May, 2013,
> but that version lacked a good way to assign functions to the pins. In
> the mean time, the PHC core has a standard method to configure the
> pins, and this series makes use of it.
>
> Thanks,
> Richard
>
> Richard Cochran (4):
> igb: refactor time sync interrupt handling
> igb: do not clobber the TSAUXC bits on reset.
> igb: enable internal PPS for the i210.
> igb: enable auxiliary PHC functions for the i210.
Thanks Richard, I have added your series of igb patches to my queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 2/4] arch: Add lightweight memory barriers fast_rmb() and fast_wmb()
From: Paul E. McKenney @ 2014-11-17 23:17 UTC (permalink / raw)
To: Alexander Duyck
Cc: linux-arch, netdev, linux-kernel, mathieu.desnoyers, peterz, benh,
heiko.carstens, mingo, mikey, linux, donald.c.skidmore,
matthew.vick, geert, jeffrey.t.kirsher, romieu, nic_swsd,
will.deacon, michael, tony.luck, torvalds, oleg, schwidefsky,
fweisbec, davem
In-Reply-To: <546A649D.2000007@redhat.com>
On Mon, Nov 17, 2014 at 01:11:57PM -0800, Alexander Duyck wrote:
> On 11/17/2014 12:18 PM, Paul E. McKenney wrote:
> >On Mon, Nov 17, 2014 at 09:18:13AM -0800, Alexander Duyck wrote:
> >>There are a number of situations where the mandatory barriers rmb() and
> >>wmb() are used to order memory/memory operations in the device drivers
> >>and those barriers are much heavier than they actually need to be. For
> >>example in the case of PowerPC wmb() calls the heavy-weight sync
> >>instruction when for memory/memory operations all that is really needed is
> >>an lsync or eieio instruction.
> >
> >Is this still the case if one of the memory operations is MMIO? Last
> >I knew, it was not.
>
> This barrier is not meant for use in MMIO operations, for that you
> still need a full barrier as I call out in the documentation
> section. What the barrier does is allow for a lightweight barrier
> for accesses to coherent system memory. So for example many device
> drivers have to perform a read of the descriptor to see if the
> device is done with it. We need an rmb() following that check to
> prevent any other accesses.
>
> Right now on x86 that rmb() becomes an lfence instruction and is
> quite expensive, and as it turns out we don't need it since the x86
> doesn't reorder reads. The same kind of thing applies to PowerPC,
> only in that case we use a sync when what we really need is a
> lwsync.
Would it make sense to have a memory barrier that enforced the
non-store-buffer orderings, that is prior reads before later
reads and writes and prior writes before later writes? This was
discussed earlier this year ((http://lwn.net/Articles/586838/,
https://lwn.net/Articles/588300/). If I recall correctly, one of
the biggest obstacles was the name. ;-)
> >>This commit adds a fast (and loose) version of the mandatory memory
> >>barriers rmb() and wmb(). The prefix to the name is actually based on the
> >>version of the functions that already exist in the mips and tile trees.
> >>However I thought it applicable since it gets at what we are trying to
> >>accomplish with these barriers and somethat implies their risky nature.
> >>
> >>These new barriers are not as safe as the standard rmb() and wmb().
> >>Specifically they do not guarantee ordering between cache-enabled and
> >>cache-inhibited memories. The primary use case for these would be to
> >>enforce ordering of memory reads/writes when accessing cache-enabled memory
> >>that is shared between the CPU and a device.
> >>
> >>It may also be noted that there is no fast_mb(). This is due to the fact
> >>that most architectures didn't seem to have a good way to do a full memory
> >>barrier quickly and so they usually resorted to an mb() for their smp_mb
> >>call. As such there no point in adding a fast_mb() function if it is going
> >>to map to mb() for all architectures anyway.
> >
> >I must confess that I still don't entirely understand the motivation.
>
> The motivation is to provide finer grained barriers. So this
> provides an in-between that allows us to "choose the right hammer".
> In the case of PowerPC it is the difference between sync/lwsync, on
> ARM it is dsb()/dmb(), and on x86 it is lfence/barrier().
Ah, so ARM will motivate a fast_wmb(), given its instruction set.
> >Some problems in PowerPC barrier.h called out below.
> >
> > Thanx, Paul
> >
>
> <snip>
>
> >>diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> >>index 22a969c..fe55dea 100644
> >>--- a/Documentation/memory-barriers.txt
> >>+++ b/Documentation/memory-barriers.txt
> >>@@ -1615,6 +1615,47 @@ There are some more advanced barrier functions:
> >> operations" subsection for information on where to use these.
> >>
> >>
> >>+ (*) fast_wmb();
> >>+ (*) fast_rmb();
> >>+
> >>+ These are for use with memory based device I/O to guarantee the ordering
> >>+ of cache-enabled writes or reads with respect to other writes or reads
> >>+ to cache-enabled memory.
> >>+
> >>+ For example, consider a device driver that shares memory with a device
> >>+ and uses a descriptor status value to indicate if the descriptor belongs
> >>+ to the device or the CPU, and a doorbell to notify it when new
> >>+ descriptors are available:
> >>+
> >>+ if (desc->status != DEVICE_OWN) {
> >>+ /* do not read data until we own descriptor */
> >>+ fast_rmb();
> >>+
> >>+ /* read/modify data */
> >>+ read_data = desc->data;
> >>+ desc->data = write_data;
> >>+
> >>+ /* flush modifications before status update */
> >>+ fast_wmb();
> >>+
> >>+ /* assign ownership */
> >>+ desc->status = DEVICE_OWN;
> >>+
> >>+ /* force memory to sync before notifying device via MMIO */
> >>+ wmb();
> >>+
> >>+ /* notify device of new descriptors */
> >>+ writel(DESC_NOTIFY, doorbell);
> >>+ }
> >>+
> >>+ The fast_rmb() allows us guarantee the device has released ownership
> >>+ before we read the data from the descriptor, and he fast_wmb() allows
> >>+ us to guarantee the data is written to the descriptor before the device
> >>+ can see it now has ownership. The wmb() is needed to guarantee that the
> >>+ cache-enabled memory writes have completed before attempting a write to
> >>+ the cache-inhibited MMIO region.
> >>+
> >>+
> >> MMIO WRITE BARRIER
> >> ------------------
>
> The general idea is that the device/CPU follow acquire/release style
> semantics and we need the lightweight barriers to enforce ordering
> to prevent us from accessing the descriptors during those periods
> where we do not own them. As the example shows we still need a full
> barrier when going between MMIO and standard memory. Hopefully that
> is what is conveyed in the documentation bits I have above.
Thank you for the clarification.
> <snip>
>
> >>diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
> >>index cb6d66c..f480097 100644
> >>--- a/arch/powerpc/include/asm/barrier.h
> >>+++ b/arch/powerpc/include/asm/barrier.h
> >>@@ -36,22 +36,20 @@
> >>
> >> #define set_mb(var, value) do { var = value; mb(); } while (0)
> >>
> >>-#ifdef CONFIG_SMP
> >>-
> >> #ifdef __SUBARCH_HAS_LWSYNC
> >> # define SMPWMB LWSYNC
> >> #else
> >> # define SMPWMB eieio
> >> #endif
> >>
> >>-#define __lwsync() __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
> >>+#define fast_rmb() __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
> >>+#define fast_wmb() __asm__ __volatile__ (stringify_in_c(SMPWMB) : : :"memory")
> >>
> >>+#ifdef CONFIG_SMP
> >> #define smp_mb() mb()
> >>-#define smp_rmb() __lwsync()
> >>-#define smp_wmb() __asm__ __volatile__ (stringify_in_c(SMPWMB) : : :"memory")
> >>+#define smp_rmb() fast_rmb()
> >>+#define smp_wmb() fast_wmb()
> >> #else
> >>-#define __lwsync() barrier()
> >>-
> >> #define smp_mb() barrier()
> >> #define smp_rmb() barrier()
> >> #define smp_wmb() barrier()
> >>@@ -69,10 +67,16 @@
> >> #define data_barrier(x) \
> >> asm volatile("twi 0,%0,0; isync" : : "r" (x) : "memory");
> >>
> >>+/*
> >>+ * The use of smp_rmb() in these functions are actually meant to map from
> >>+ * smp_rmb()->fast_rmb()->LWSYNC. This way if smp is disabled then
> >>+ * smp_rmb()->barrier(), or if the platform doesn't support lwsync it will
> >>+ * map to the more heavy-weight sync.
> >>+ */
> >> #define smp_store_release(p, v) \
> >> do { \
> >> compiletime_assert_atomic_type(*p); \
> >>- __lwsync(); \
> >>+ smp_rmb(); \
> >
> >This is not good at all. For smp_store_release(), we absolutely
> >must order prior loads and stores against the assignment on the following
> >line. This is not something that smp_rmb() does, nor is it something
> >that smp_wmb() does. Yes, it might happen to now, but this could easily
> >break in the future -- plus this change is extremely misleading.
> >
> >The original __lwsync() is much more clear.
>
> The problem I had with __lwsync is that it really wasn't all that
> clear. It was the lwsync instruction if SMP was enabled, otherwise
> it was just a barrier call. What I did is move the definition of
> __lwsync in the SMP case into fast_rmb, which in turn is accessed by
> smp_rmb. I tried to make this clear in the comment just above the
> two calls. The resultant assembly code should be exactly the same.
>
> What I could do is have it added back as a smp_lwsync if that works
> for you. That way there is something there to give you a hint that
> it becomes a barrier() call as soon as SMP is disabled.
An smp_lwsync() would be a great improvement!
Thanx, Paul
^ permalink raw reply
* Re: [PATCH net-next 2/4] igb: do not clobber the TSAUXC bits on reset.
From: Keller, Jacob E @ 2014-11-17 23:22 UTC (permalink / raw)
To: richardcochran@gmail.com
Cc: netdev@vger.kernel.org, davem@davemloft.net, Allan, Bruce W,
Ronciak, John, Kirsher, Jeffrey T, Vick, Matthew
In-Reply-To: <5c892a6a13d120193fdbd2dd62e9604ae0d5d404.1416265321.git.richardcochran@gmail.com>
On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> The TSAUXC register has a number of different bits, one of which disables
> the main clock function. Previously, the clock was re-enabled by clearing
> the entire register. This patch changes the code to preserve the values
> of the other bits in that register.
>
This is a step in the right direction, but won't fully work due to this
being called after a MAC reset.
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
> drivers/net/ethernet/intel/igb/igb_ptp.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 794c139..ce57128 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -905,6 +905,8 @@ void igb_ptp_stop(struct igb_adapter *adapter)
> void igb_ptp_reset(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
> + unsigned long flags;
> + u32 tsauxc;
>
> if (!(adapter->flags & IGB_FLAG_PTP))
> return;
> @@ -923,7 +925,11 @@ void igb_ptp_reset(struct igb_adapter *adapter)
> case e1000_i210:
> case e1000_i211:
> /* Enable the timer functions and interrupts. */
> - wr32(E1000_TSAUXC, 0x0);
> + spin_lock_irqsave(&adapter->tmreg_lock, flags);
> + tsauxc = rd32(E1000_TSAUXC);
> + tsauxc &= ~TSAUXC_DISABLE;
> + wr32(E1000_TSAUXC, tsauxc);
> + spin_unlock_irqrestore(&adapter->tmreg_lock, flags);
This won't work. We call igb_ptp_reset directly after a HW reset (MAC
reset). TSAUXC data is not maintained. We probably need to store driver
state of the TSAUXC bits, and then re-enable them in this reset path.
This code today is fine, because there was no other state we cared about
in the path.
That being said, I don't think this change breaks anything. I just think
it will not solve the problem.
Thanks,
Jake
> wr32(E1000_TSIM, TSYNC_INTERRUPTS);
> wr32(E1000_IMS, E1000_IMS_TS);
> break;
^ permalink raw reply
* Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.
From: Keller, Jacob E @ 2014-11-17 23:24 UTC (permalink / raw)
To: richardcochran@gmail.com
Cc: netdev@vger.kernel.org, davem@davemloft.net, Allan, Bruce W,
Ronciak, John, Kirsher, Jeffrey T, Vick, Matthew
In-Reply-To: <e5b4ef7c56119e2d86e42f5680b1b82c9ec1a38e.1416265321.git.richardcochran@gmail.com>
On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> The i210 device can produce an interrupt on the full second. This
> patch allows using this interrupt to generate an internal PPS event
> for adjusting the kernel system time.
>
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++++
> drivers/net/ethernet/intel/igb/igb_ptp.c | 32 +++++++++++++++++++++++++++--
> 2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 7183a56..9412661 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter,
> static void igb_tsync_interrupt(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
> + struct ptp_clock_event event;
> u32 tsicr = rd32(E1000_TSICR);
>
> + if (tsicr & TSINTR_SYS_WRAP) {
> + event.type = PTP_CLOCK_PPS;
> + ptp_clock_event(adapter->ptp_clock, &event);
> + }
> +
Nice! I hadn't thought of using the SYS_WRAP around interrupt for pps.
Much simpler than having to setup a SDP pin trigger.
Regards,
Jake
^ permalink raw reply
* Re: [PATCH net-next 4/4] igb: enable auxiliary PHC functions for the i210.
From: Keller, Jacob E @ 2014-11-17 23:28 UTC (permalink / raw)
To: richardcochran@gmail.com
Cc: netdev@vger.kernel.org, davem@davemloft.net, Allan, Bruce W,
Ronciak, John, Kirsher, Jeffrey T, Vick, Matthew
In-Reply-To: <c13112d45627bab7a6d55c6727f022da46d1a2a2.1416265321.git.richardcochran@gmail.com>
On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote:
> The i210 device offers a number of special PTP Hardware Clock features on
> the Software Defined Pins (SDPs). This patch adds support for two of the
> possible functions, namely time stamping external events, and periodic
> output signals.
>
> The assignment of PHC functions to the four SDP can be freely chosen by
> the user.
>
This patch probably needs some code added to igb_ptp_reset which will
re-enable the SDP pins after a MAC reset to the same current settings
stored by the driver. It looks like you maintain all this data in
internal structures, so it should be trivial to add the correct function
calls to the reset path.
If you don't have this code, then any time there is a hardware reset,
these will be disabled. Normal operation can sometimes have resets due
to TX hang. I think even one of the ethtool operations can cause a reset
to occur sometimes. See my comments on one of the other patch in the
series about this, also.
Thanks,
Jake
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
> drivers/net/ethernet/intel/igb/igb.h | 9 ++
> drivers/net/ethernet/intel/igb/igb_main.c | 47 ++++++-
> drivers/net/ethernet/intel/igb/igb_ptp.c | 219 ++++++++++++++++++++++++++++-
> 3 files changed, 269 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 82d891e..f6aca7c 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -343,6 +343,9 @@ struct hwmon_buff {
> };
> #endif
>
> +#define IGB_N_EXTTS 2
> +#define IGB_N_PEROUT 2
> +#define IGB_N_SDP 4
> #define IGB_RETA_SIZE 128
>
> /* board specific private data structure */
> @@ -439,6 +442,12 @@ struct igb_adapter {
> u32 tx_hwtstamp_timeouts;
> u32 rx_hwtstamp_cleared;
>
> + struct ptp_pin_desc sdp_config[IGB_N_SDP];
> + struct {
> + struct timespec start;
> + struct timespec period;
> + } perout[IGB_N_PEROUT];
> +
> char fw_version[32];
> #ifdef CONFIG_IGB_HWMON
> struct hwmon_buff *igb_hwmon_buff;
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 9412661..3a25661 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5387,7 +5387,8 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
> struct ptp_clock_event event;
> - u32 tsicr = rd32(E1000_TSICR);
> + struct timespec ts;
> + u32 tsauxc, sec, nsec, tsicr = rd32(E1000_TSICR);
>
> if (tsicr & TSINTR_SYS_WRAP) {
> event.type = PTP_CLOCK_PPS;
> @@ -5400,6 +5401,50 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
> /* retrieve hardware timestamp */
> schedule_work(&adapter->ptp_tx_work);
> }
> +
> + if (tsicr & TSINTR_TT0) {
> + spin_lock(&adapter->tmreg_lock);
> + ts = timespec_add(adapter->perout[0].start,
> + adapter->perout[0].period);
> + wr32(E1000_TRGTTIML0, ts.tv_nsec);
> + wr32(E1000_TRGTTIMH0, ts.tv_sec);
> + tsauxc = rd32(E1000_TSAUXC);
> + tsauxc |= TSAUXC_EN_TT0;
> + wr32(E1000_TSAUXC, tsauxc);
> + adapter->perout[0].start = ts;
> + spin_unlock(&adapter->tmreg_lock);
> + }
> +
> + if (tsicr & TSINTR_TT1) {
> + spin_lock(&adapter->tmreg_lock);
> + ts = timespec_add(adapter->perout[1].start,
> + adapter->perout[1].period);
> + wr32(E1000_TRGTTIML1, ts.tv_nsec);
> + wr32(E1000_TRGTTIMH1, ts.tv_sec);
> + tsauxc = rd32(E1000_TSAUXC);
> + tsauxc |= TSAUXC_EN_TT1;
> + wr32(E1000_TSAUXC, tsauxc);
> + adapter->perout[1].start = ts;
> + spin_unlock(&adapter->tmreg_lock);
> + }
> +
> + if (tsicr & TSINTR_AUTT0) {
> + nsec = rd32(E1000_AUXSTMPL0);
> + sec = rd32(E1000_AUXSTMPH0);
> + event.type = PTP_CLOCK_EXTTS;
> + event.index = 0;
> + event.timestamp = sec * 1000000000ULL + nsec;
> + ptp_clock_event(adapter->ptp_clock, &event);
> + }
> +
> + if (tsicr & TSINTR_AUTT1) {
> + nsec = rd32(E1000_AUXSTMPL1);
> + sec = rd32(E1000_AUXSTMPH1);
> + event.type = PTP_CLOCK_EXTTS;
> + event.index = 1;
> + event.timestamp = sec * 1000000000ULL + nsec;
> + ptp_clock_event(adapter->ptp_clock, &event);
> + }
> }
>
> static irqreturn_t igb_msix_other(int irq, void *data)
> diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c
> index 70d3933..37b9fe6 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ptp.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c
> @@ -360,16 +360,203 @@ static int igb_ptp_settime_i210(struct ptp_clock_info *ptp,
> return 0;
> }
>
> +static void igb_pin_direction(int pin, int input, u32 *ctrl, u32 *ctrl_ext)
> +{
> + u32 *ptr = pin < 2 ? ctrl : ctrl_ext;
> + u32 mask[IGB_N_SDP] = {
> + E1000_CTRL_SDP0_DIR,
> + E1000_CTRL_SDP1_DIR,
> + E1000_CTRL_EXT_SDP2_DIR,
> + E1000_CTRL_EXT_SDP3_DIR,
> + };
> +
> + if (input)
> + *ptr &= ~mask[pin];
> + else
> + *ptr |= mask[pin];
> +}
> +
> +static void igb_pin_extts(struct igb_adapter *igb, int chan, int pin)
> +{
> + struct e1000_hw *hw = &igb->hw;
> + u32 aux0_sel_sdp[IGB_N_SDP] = {
> + AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
> + };
> + u32 aux1_sel_sdp[IGB_N_SDP] = {
> + AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
> + };
> + u32 ts_sdp_en[IGB_N_SDP] = {
> + TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
> + };
> + u32 ctrl, ctrl_ext, tssdp = 0;
> +
> + ctrl = rd32(E1000_CTRL);
> + ctrl_ext = rd32(E1000_CTRL_EXT);
> + tssdp = rd32(E1000_TSSDP);
> +
> + igb_pin_direction(pin, 1, &ctrl, &ctrl_ext);
> +
> + /* Make sure this pin is not enabled as an ouput. */
> + tssdp &= ~ts_sdp_en[pin];
> +
> + if (chan == 1) {
> + tssdp &= ~AUX1_SEL_SDP3;
> + tssdp |= aux1_sel_sdp[pin] | AUX1_TS_SDP_EN;
> + } else {
> + tssdp &= ~AUX0_SEL_SDP3;
> + tssdp |= aux0_sel_sdp[pin] | AUX0_TS_SDP_EN;
> + }
> +
> + wr32(E1000_TSSDP, tssdp);
> + wr32(E1000_CTRL, ctrl);
> + wr32(E1000_CTRL_EXT, ctrl_ext);
> +}
> +
> +static void igb_pin_perout(struct igb_adapter *igb, int chan, int pin)
> +{
> + struct e1000_hw *hw = &igb->hw;
> + u32 aux0_sel_sdp[IGB_N_SDP] = {
> + AUX0_SEL_SDP0, AUX0_SEL_SDP1, AUX0_SEL_SDP2, AUX0_SEL_SDP3,
> + };
> + u32 aux1_sel_sdp[IGB_N_SDP] = {
> + AUX1_SEL_SDP0, AUX1_SEL_SDP1, AUX1_SEL_SDP2, AUX1_SEL_SDP3,
> + };
> + u32 ts_sdp_en[IGB_N_SDP] = {
> + TS_SDP0_EN, TS_SDP1_EN, TS_SDP2_EN, TS_SDP3_EN,
> + };
> + u32 ts_sdp_sel_tt0[IGB_N_SDP] = {
> + TS_SDP0_SEL_TT0, TS_SDP1_SEL_TT0,
> + TS_SDP2_SEL_TT0, TS_SDP3_SEL_TT0,
> + };
> + u32 ts_sdp_sel_tt1[IGB_N_SDP] = {
> + TS_SDP0_SEL_TT1, TS_SDP1_SEL_TT1,
> + TS_SDP2_SEL_TT1, TS_SDP3_SEL_TT1,
> + };
> + u32 ts_sdp_sel_clr[IGB_N_SDP] = {
> + TS_SDP0_SEL_FC1, TS_SDP1_SEL_FC1,
> + TS_SDP2_SEL_FC1, TS_SDP3_SEL_FC1,
> + };
> + u32 ctrl, ctrl_ext, tssdp = 0;
> +
> + ctrl = rd32(E1000_CTRL);
> + ctrl_ext = rd32(E1000_CTRL_EXT);
> + tssdp = rd32(E1000_TSSDP);
> +
> + igb_pin_direction(pin, 0, &ctrl, &ctrl_ext);
> +
> + /* Make sure this pin is not enabled as an input. */
> + if ((tssdp & AUX0_SEL_SDP3) == aux0_sel_sdp[pin])
> + tssdp &= ~AUX0_TS_SDP_EN;
> +
> + if ((tssdp & AUX1_SEL_SDP3) == aux1_sel_sdp[pin])
> + tssdp &= ~AUX1_TS_SDP_EN;
> +
> + tssdp &= ~ts_sdp_sel_clr[pin];
> + if (chan == 1)
> + tssdp |= ts_sdp_sel_tt1[pin];
> + else
> + tssdp |= ts_sdp_sel_tt0[pin];
> +
> + tssdp |= ts_sdp_en[pin];
> +
> + wr32(E1000_TSSDP, tssdp);
> + wr32(E1000_CTRL, ctrl);
> + wr32(E1000_CTRL_EXT, ctrl_ext);
> +}
> +
> static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
> struct ptp_clock_request *rq, int on)
> {
> struct igb_adapter *igb =
> container_of(ptp, struct igb_adapter, ptp_caps);
> struct e1000_hw *hw = &igb->hw;
> + u32 tsauxc, tsim, tsauxc_mask, tsim_mask, trgttiml, trgttimh;
> unsigned long flags;
> - u32 tsim;
> + struct timespec ts;
> + int pin;
> + s64 ns;
>
> switch (rq->type) {
> + case PTP_CLK_REQ_EXTTS:
> + if (on) {
> + pin = ptp_find_pin(igb->ptp_clock, PTP_PF_EXTTS,
> + rq->extts.index);
> + if (pin < 0)
> + return -EBUSY;
> + igb_pin_extts(igb, rq->extts.index, pin);
> + }
> + if (rq->extts.index == 1) {
> + tsauxc_mask = TSAUXC_EN_TS1;
> + tsim_mask = TSINTR_AUTT1;
> + } else {
> + tsauxc_mask = TSAUXC_EN_TS0;
> + tsim_mask = TSINTR_AUTT0;
> + }
> + spin_lock_irqsave(&igb->tmreg_lock, flags);
> + tsauxc = rd32(E1000_TSAUXC);
> + tsim = rd32(E1000_TSIM);
> + if (on) {
> + tsauxc |= tsauxc_mask;
> + tsim |= tsim_mask;
> + } else {
> + tsauxc &= ~tsauxc_mask;
> + tsim &= ~tsim_mask;
> + }
> + wr32(E1000_TSAUXC, tsauxc);
> + wr32(E1000_TSIM, tsim);
> + spin_unlock_irqrestore(&igb->tmreg_lock, flags);
> + return 0;
> +
> + case PTP_CLK_REQ_PEROUT:
> + if (on) {
> + pin = ptp_find_pin(igb->ptp_clock, PTP_PF_PEROUT,
> + rq->perout.index);
> + if (pin < 0)
> + return -EBUSY;
> + igb_pin_perout(igb, rq->perout.index, pin);
> + }
> + ts.tv_sec = rq->perout.period.sec;
> + ts.tv_nsec = rq->perout.period.nsec;
> + ns = timespec_to_ns(&ts);
> + ns = ns >> 1;
> + if (on && ns < 500000LL) {
> + /* 2k interrupts per second is an awful lot. */
> + return -EINVAL;
> + }
> + ts = ns_to_timespec(ns);
> + if (rq->perout.index == 1) {
> + tsauxc_mask = TSAUXC_EN_TT1;
> + tsim_mask = TSINTR_TT1;
> + trgttiml = E1000_TRGTTIML1;
> + trgttimh = E1000_TRGTTIMH1;
> + } else {
> + tsauxc_mask = TSAUXC_EN_TT0;
> + tsim_mask = TSINTR_TT0;
> + trgttiml = E1000_TRGTTIML0;
> + trgttimh = E1000_TRGTTIMH0;
> + }
> + spin_lock_irqsave(&igb->tmreg_lock, flags);
> + tsauxc = rd32(E1000_TSAUXC);
> + tsim = rd32(E1000_TSIM);
> + if (on) {
> + int i = rq->perout.index;
> + igb->perout[i].start.tv_sec = rq->perout.start.sec;
> + igb->perout[i].start.tv_nsec = rq->perout.start.nsec;
> + igb->perout[i].period.tv_sec = ts.tv_sec;
> + igb->perout[i].period.tv_nsec = ts.tv_nsec;
> + wr32(trgttiml, rq->perout.start.sec);
> + wr32(trgttimh, rq->perout.start.nsec);
> + tsauxc |= tsauxc_mask;
> + tsim |= tsim_mask;
> + } else {
> + tsauxc &= ~tsauxc_mask;
> + tsim &= ~tsim_mask;
> + }
> + wr32(E1000_TSAUXC, tsauxc);
> + wr32(E1000_TSIM, tsim);
> + spin_unlock_irqrestore(&igb->tmreg_lock, flags);
> + return 0;
> +
> case PTP_CLK_REQ_PPS:
> spin_lock_irqsave(&igb->tmreg_lock, flags);
> tsim = rd32(E1000_TSIM);
> @@ -380,9 +567,6 @@ static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp,
> wr32(E1000_TSIM, tsim);
> spin_unlock_irqrestore(&igb->tmreg_lock, flags);
> return 0;
> -
> - default:
> - break;
> }
>
> return -EOPNOTSUPP;
> @@ -394,6 +578,20 @@ static int igb_ptp_feature_enable(struct ptp_clock_info *ptp,
> return -EOPNOTSUPP;
> }
>
> +static int igb_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
> + enum ptp_pin_function func, unsigned int chan)
> +{
> + switch (func) {
> + case PTP_PF_NONE:
> + case PTP_PF_EXTTS:
> + case PTP_PF_PEROUT:
> + break;
> + case PTP_PF_PHYSYNC:
> + return -1;
> + }
> + return 0;
> +}
> +
> /**
> * igb_ptp_tx_work
> * @work: pointer to work struct
> @@ -784,6 +982,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
> struct net_device *netdev = adapter->netdev;
> + int i;
>
> switch (hw->mac.type) {
> case e1000_82576:
> @@ -826,16 +1025,26 @@ void igb_ptp_init(struct igb_adapter *adapter)
> break;
> case e1000_i210:
> case e1000_i211:
> + for (i = 0; i < IGB_N_SDP; i++) {
> + struct ptp_pin_desc *ppd = &adapter->sdp_config[i];
> + snprintf(ppd->name, sizeof(ppd->name), "SDP%d", i);
> + ppd->index = i;
> + ppd->func = PTP_PF_NONE;
> + }
> snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr);
> adapter->ptp_caps.owner = THIS_MODULE;
> adapter->ptp_caps.max_adj = 62499999;
> - adapter->ptp_caps.n_ext_ts = 0;
> + adapter->ptp_caps.n_ext_ts = IGB_N_EXTTS;
> + adapter->ptp_caps.n_per_out = IGB_N_PEROUT;
> + adapter->ptp_caps.n_pins = IGB_N_SDP;
> adapter->ptp_caps.pps = 1;
> + adapter->ptp_caps.pin_config = adapter->sdp_config;
> adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580;
> adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210;
> adapter->ptp_caps.gettime = igb_ptp_gettime_i210;
> adapter->ptp_caps.settime = igb_ptp_settime_i210;
> adapter->ptp_caps.enable = igb_ptp_feature_enable_i210;
> + adapter->ptp_caps.verify = igb_ptp_verify_pin;
> /* Enable the timer functions by clearing bit 31. */
> wr32(E1000_TSAUXC, 0x0);
> break;
^ permalink raw reply
* Re: [PATCH] ixgbe: Look up MAC address in Open Firmware
From: Rustad, Mark D @ 2014-11-17 23:29 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: Sergei Shtylyov, netdev@vger.kernel.org, Linux NICS
In-Reply-To: <yq1mw7tv8jq.fsf@sermon.lab.mkp.net>
[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]
On Nov 14, 2014, at 12:20 PM, Martin K. Petersen <martin.petersen@oracle.com> wrote:
> /**
> + * ixgbe_of_mac_addr - Look up MAC address in Open Firmware
> + * @adapter: Pointer to adapter struct
> + */
> +static void ixgbe_of_mac_addr(struct ixgbe_adapter *adapter)
> +{
> +#ifdef CONFIG_OF
> + struct device_node *dp = pci_device_to_OF_node(adapter->pdev);
> + struct ixgbe_hw *hw = &adapter->hw;
> + const unsigned char *addr;
> + int len;
> +
> + addr = of_get_property(dp, "local-mac-address", &len);
> + if (addr && len == 6) {
> + e_dev_info("Using OpenPROM MAC address\n");
> + memcpy(hw->mac.perm_addr, addr, 6);
> + }
> +
> + if (!is_valid_ether_addr(hw->mac.perm_addr)) {
> + e_dev_info("Using IDPROM MAC address\n");
> + memcpy(hw->mac.perm_addr, idprom->id_ethaddr, 6);
> + }
> +#endif
> +}
I would prefer that the entire function be wrapped by the #if so that a null implementation can include the prototype. That allows the unused parameter to properly be declared as __always_unused as in:
#else
static void ixgbe_of_mac_addr(struct ixgbe_adapter __always_unused *adapter)
{}
#endif /* CONFIG_OF */
Otherwise, it will throw warnings when compiled with enhanced warning messages.
--
Mark Rustad, Networking Division, Intel Corporation
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]
^ permalink raw reply
* Re: [PATCH v3] ixgbe: Look up MAC address in Open Firmware or IDPROM
From: Rustad, Mark D @ 2014-11-17 23:35 UTC (permalink / raw)
To: Martin K. Petersen; +Cc: netdev@vger.kernel.org, Linux NICS, Kirsher, Jeffrey T
In-Reply-To: <yq17fyxus1y.fsf@sermon.lab.mkp.net>
[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]
On Nov 14, 2014, at 6:16 PM, Martin K. Petersen <mkp@mkp.net> wrote:
> @@ -7960,6 +7970,29 @@ int ixgbe_wol_supported(struct ixgbe_adapter *adapter, u16 device_id,
> }
>
> /**
> + * ixgbe_get_platform_mac_addr - Look up MAC address in Open Firmware / IDPROM
> + * @adapter: Pointer to adapter struct
> + */
> +static void ixgbe_get_platform_mac_addr(struct ixgbe_adapter *adapter)
How about tagging adapter with __maybe_unused in this case to avoid warnings when neither CONFIG_OF or CONFIG_SPARC are defined?
> +{
> +#ifdef CONFIG_OF
> + struct device_node *dp = pci_device_to_OF_node(adapter->pdev);
> + struct ixgbe_hw *hw = &adapter->hw;
> + const unsigned char *addr;
> +
> + addr = of_get_mac_address(dp);
> + if (addr) {
> + ether_addr_copy(hw->mac.perm_addr, addr);
> + return;
> + }
> +#endif /* CONFIG_OF */
> +
> +#ifdef CONFIG_SPARC
> + ether_addr_copy(hw->mac.perm_addr, idprom->id_ethaddr);
> +#endif /* CONFIG_SPARC */
> +}
> +
> +/**
> * ixgbe_probe - Device Initialization Routine
> * @pdev: PCI device information struct
> * @ent: entry in ixgbe_pci_tbl
--
Mark Rustad, Networking Division, Intel Corporation
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]
^ permalink raw reply
* [PATCH net] ixgbe: fix use after free adapter->state test in ixgbe_remove/ixgbe_probe
From: Daniel Borkmann @ 2014-11-17 23:43 UTC (permalink / raw)
To: davem; +Cc: netdev, Mark Rustad, Jeff Kirsher
While working on a different issue, I noticed an annoying use
after free bug on my machine when unloading the ixgbe driver:
[ 8642.318797] ixgbe 0000:02:00.1: removed PHC on p2p2
[ 8642.742716] ixgbe 0000:02:00.1: complete
[ 8642.743784] BUG: unable to handle kernel paging request at ffff8807d3740a90
[ 8642.744828] IP: [<ffffffffa01c77dc>] ixgbe_remove+0xfc/0x1b0 [ixgbe]
[ 8642.745886] PGD 20c6067 PUD 81c1f6067 PMD 81c15a067 PTE 80000007d3740060
[ 8642.746956] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
[ 8642.748039] Modules linked in: [...]
[ 8642.752929] CPU: 1 PID: 1225 Comm: rmmod Not tainted 3.18.0-rc2+ #49
[ 8642.754203] Hardware name: Supermicro X10SLM-F/X10SLM-F, BIOS 1.1b 11/01/2013
[ 8642.755505] task: ffff8807e34d3fe0 ti: ffff8807b7204000 task.ti: ffff8807b7204000
[ 8642.756831] RIP: 0010:[<ffffffffa01c77dc>] [<ffffffffa01c77dc>] ixgbe_remove+0xfc/0x1b0 [ixgbe]
[...]
[ 8642.774335] Stack:
[ 8642.775805] ffff8807ee824098 ffff8807ee824098 ffffffffa01f3000 ffff8807ee824000
[ 8642.777326] ffff8807b7207e18 ffffffff8137720f ffff8807ee824098 ffff8807ee824098
[ 8642.778848] ffffffffa01f3068 ffff8807ee8240f8 ffff8807b7207e38 ffffffff8144180f
[ 8642.780365] Call Trace:
[ 8642.781869] [<ffffffff8137720f>] pci_device_remove+0x3f/0xc0
[ 8642.783395] [<ffffffff8144180f>] __device_release_driver+0x7f/0xf0
[ 8642.784876] [<ffffffff814421f8>] driver_detach+0xb8/0xc0
[ 8642.786352] [<ffffffff814414a9>] bus_remove_driver+0x59/0xe0
[ 8642.787783] [<ffffffff814429d0>] driver_unregister+0x30/0x70
[ 8642.789202] [<ffffffff81375c65>] pci_unregister_driver+0x25/0xa0
[ 8642.790657] [<ffffffffa01eb38e>] ixgbe_exit_module+0x1c/0xc8e [ixgbe]
[ 8642.792064] [<ffffffff810f93a2>] SyS_delete_module+0x132/0x1c0
[ 8642.793450] [<ffffffff81012c61>] ? do_notify_resume+0x61/0xa0
[ 8642.794837] [<ffffffff816d2029>] system_call_fastpath+0x12/0x17
The issue is that test_and_set_bit() done on adapter->state is being
performed *after* the netdevice has been freed via free_netdev().
When netdev is being allocated on initialization time, it allocates
a private area, here struct ixgbe_adapter, that resides after the
net_device structure. In ixgbe_probe(), the device init routine,
we set up the adapter after alloc_etherdev_mq() on the private area
and add a reference for the pci_dev as well via pci_set_drvdata().
Both in the error path of ixgbe_probe(), but also on module unload
when ixgbe_remove() is being called, commit 41c62843eb6a accesses
adapter after free_netdev(). The patch stores the result in a bool
and thus fixes above oops on my side.
Fixes: 41c62843eb6a ("ixgbe: Fix rcu warnings induced by LER")
Cc: Mark Rustad <mark.d.rustad@intel.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index d2df4e3..7185703 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7979,6 +7979,7 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
int i, err, pci_using_dac, expected_gts;
unsigned int indices = MAX_TX_QUEUES;
u8 part_str[IXGBE_PBANUM_LENGTH];
+ bool disable_dev = false;
#ifdef IXGBE_FCOE
u16 device_caps;
#endif
@@ -8369,13 +8370,14 @@ err_sw_init:
iounmap(adapter->io_addr);
kfree(adapter->mac_table);
err_ioremap:
+ disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter->state);
free_netdev(netdev);
err_alloc_etherdev:
pci_release_selected_regions(pdev,
pci_select_bars(pdev, IORESOURCE_MEM));
err_pci_reg:
err_dma:
- if (!adapter || !test_and_set_bit(__IXGBE_DISABLED, &adapter->state))
+ if (!adapter || disable_dev)
pci_disable_device(pdev);
return err;
}
@@ -8393,6 +8395,7 @@ static void ixgbe_remove(struct pci_dev *pdev)
{
struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
struct net_device *netdev = adapter->netdev;
+ bool disable_dev;
ixgbe_dbg_adapter_exit(adapter);
@@ -8442,11 +8445,12 @@ static void ixgbe_remove(struct pci_dev *pdev)
e_dev_info("complete\n");
kfree(adapter->mac_table);
+ disable_dev = !test_and_set_bit(__IXGBE_DISABLED, &adapter->state);
free_netdev(netdev);
pci_disable_pcie_error_reporting(pdev);
- if (!test_and_set_bit(__IXGBE_DISABLED, &adapter->state))
+ if (disable_dev)
pci_disable_device(pdev);
}
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH net] ixgbe: fix use after free adapter->state test in ixgbe_remove/ixgbe_probe
From: Jeff Kirsher @ 2014-11-17 23:48 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, Mark Rustad
In-Reply-To: <1416267827-11545-1-git-send-email-dborkman@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3192 bytes --]
On Tue, 2014-11-18 at 00:43 +0100, Daniel Borkmann wrote:
> While working on a different issue, I noticed an annoying use
> after free bug on my machine when unloading the ixgbe driver:
>
> [ 8642.318797] ixgbe 0000:02:00.1: removed PHC on p2p2
> [ 8642.742716] ixgbe 0000:02:00.1: complete
> [ 8642.743784] BUG: unable to handle kernel paging request at
> ffff8807d3740a90
> [ 8642.744828] IP: [<ffffffffa01c77dc>] ixgbe_remove+0xfc/0x1b0
> [ixgbe]
> [ 8642.745886] PGD 20c6067 PUD 81c1f6067 PMD 81c15a067 PTE
> 80000007d3740060
> [ 8642.746956] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC
> [ 8642.748039] Modules linked in: [...]
> [ 8642.752929] CPU: 1 PID: 1225 Comm: rmmod Not tainted 3.18.0-rc2+
> #49
> [ 8642.754203] Hardware name: Supermicro X10SLM-F/X10SLM-F, BIOS 1.1b
> 11/01/2013
> [ 8642.755505] task: ffff8807e34d3fe0 ti: ffff8807b7204000 task.ti:
> ffff8807b7204000
> [ 8642.756831] RIP: 0010:[<ffffffffa01c77dc>] [<ffffffffa01c77dc>]
> ixgbe_remove+0xfc/0x1b0 [ixgbe]
> [...]
> [ 8642.774335] Stack:
> [ 8642.775805] ffff8807ee824098 ffff8807ee824098 ffffffffa01f3000
> ffff8807ee824000
> [ 8642.777326] ffff8807b7207e18 ffffffff8137720f ffff8807ee824098
> ffff8807ee824098
> [ 8642.778848] ffffffffa01f3068 ffff8807ee8240f8 ffff8807b7207e38
> ffffffff8144180f
> [ 8642.780365] Call Trace:
> [ 8642.781869] [<ffffffff8137720f>] pci_device_remove+0x3f/0xc0
> [ 8642.783395] [<ffffffff8144180f>] __device_release_driver+0x7f/0xf0
> [ 8642.784876] [<ffffffff814421f8>] driver_detach+0xb8/0xc0
> [ 8642.786352] [<ffffffff814414a9>] bus_remove_driver+0x59/0xe0
> [ 8642.787783] [<ffffffff814429d0>] driver_unregister+0x30/0x70
> [ 8642.789202] [<ffffffff81375c65>] pci_unregister_driver+0x25/0xa0
> [ 8642.790657] [<ffffffffa01eb38e>] ixgbe_exit_module+0x1c/0xc8e
> [ixgbe]
> [ 8642.792064] [<ffffffff810f93a2>] SyS_delete_module+0x132/0x1c0
> [ 8642.793450] [<ffffffff81012c61>] ? do_notify_resume+0x61/0xa0
> [ 8642.794837] [<ffffffff816d2029>] system_call_fastpath+0x12/0x17
>
> The issue is that test_and_set_bit() done on adapter->state is being
> performed *after* the netdevice has been freed via free_netdev().
>
> When netdev is being allocated on initialization time, it allocates
> a private area, here struct ixgbe_adapter, that resides after the
> net_device structure. In ixgbe_probe(), the device init routine,
> we set up the adapter after alloc_etherdev_mq() on the private area
> and add a reference for the pci_dev as well via pci_set_drvdata().
>
> Both in the error path of ixgbe_probe(), but also on module unload
> when ixgbe_remove() is being called, commit 41c62843eb6a accesses
> adapter after free_netdev(). The patch stores the result in a bool
> and thus fixes above oops on my side.
>
> Fixes: 41c62843eb6a ("ixgbe: Fix rcu warnings induced by LER")
> Cc: Mark Rustad <mark.d.rustad@intel.com>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
Thanks Daniel, I will add your patch to my net queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH net-next] tcp: make connect() mem charging friendly
From: Eric Dumazet @ 2014-11-17 23:49 UTC (permalink / raw)
To: Denys Fedoryshchenko, David Miller; +Cc: netdev, Neal Cardwell, Yuchung Cheng
In-Reply-To: <1416266755.18588.12.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
While working on sk_forward_alloc problems reported by Denys
Fedoryshchenko, we found that tcp connect() (and fastopen) do not call
sk_wmem_schedule() for SYN packet (and/or SYN/DATA packet), so
sk_forward_alloc is negative while connect is in progress.
We can fix this by calling regular sk_stream_alloc_skb() both for the
SYN packet (in tcp_connect()) and the syn_data packet in
tcp_send_syn_data()
Then, tcp_send_syn_data() can avoid copying syn_data as we simply
can manipulate syn_data->cb[] to remove SYN flag (and increment seq)
Instead of open coding memcpy_fromiovecend(), simply use this helper.
This leaves in socket write queue clean fast clone skbs.
This was tested against our fastopen packetdrill tests.
Reported-by: Denys Fedoryshchenko <nuclearcat@nuclearcat.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_output.c | 66 +++++++++++++++-------------------------
1 file changed, 26 insertions(+), 40 deletions(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index eb73a1dccf56..377e15d0ba96 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3011,9 +3011,9 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn)
{
struct tcp_sock *tp = tcp_sk(sk);
struct tcp_fastopen_request *fo = tp->fastopen_req;
- int syn_loss = 0, space, i, err = 0, iovlen = fo->data->msg_iovlen;
- struct sk_buff *syn_data = NULL, *data;
+ int syn_loss = 0, space, err = 0;
unsigned long last_syn_loss = 0;
+ struct sk_buff *syn_data;
tp->rx_opt.mss_clamp = tp->advmss; /* If MSS is not cached */
tcp_fastopen_cache_get(sk, &tp->rx_opt.mss_clamp, &fo->cookie,
@@ -3044,48 +3044,38 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn)
/* limit to order-0 allocations */
space = min_t(size_t, space, SKB_MAX_HEAD(MAX_TCP_HEADER));
- syn_data = skb_copy_expand(syn, MAX_TCP_HEADER, space,
- sk->sk_allocation);
- if (syn_data == NULL)
+ syn_data = sk_stream_alloc_skb(sk, space, sk->sk_allocation);
+ if (!syn_data)
+ goto fallback;
+ syn_data->ip_summed = CHECKSUM_PARTIAL;
+ memcpy(syn_data->cb, syn->cb, sizeof(syn->cb));
+ if (memcpy_fromiovecend(skb_put(syn_data, space),
+ fo->data->msg_iov, 0, space))
goto fallback;
- for (i = 0; i < iovlen && syn_data->len < space; ++i) {
- struct iovec *iov = &fo->data->msg_iov[i];
- unsigned char __user *from = iov->iov_base;
- int len = iov->iov_len;
+ /* No more data pending in inet_wait_for_connect() */
+ if (space == fo->size)
+ fo->data = NULL;
+ fo->copied = space;
- if (syn_data->len + len > space)
- len = space - syn_data->len;
- else if (i + 1 == iovlen)
- /* No more data pending in inet_wait_for_connect() */
- fo->data = NULL;
+ tcp_connect_queue_skb(sk, syn_data);
- if (skb_add_data(syn_data, from, len))
- goto fallback;
- }
+ err = tcp_transmit_skb(sk, syn_data, 1, sk->sk_allocation);
- /* Queue a data-only packet after the regular SYN for retransmission */
- data = pskb_copy(syn_data, sk->sk_allocation);
- if (data == NULL)
- goto fallback;
- TCP_SKB_CB(data)->seq++;
- TCP_SKB_CB(data)->tcp_flags &= ~TCPHDR_SYN;
- TCP_SKB_CB(data)->tcp_flags = (TCPHDR_ACK|TCPHDR_PSH);
- tcp_connect_queue_skb(sk, data);
- fo->copied = data->len;
-
- /* syn_data is about to be sent, we need to take current time stamps
- * for the packets that are in write queue : SYN packet and DATA
- */
- skb_mstamp_get(&syn->skb_mstamp);
- data->skb_mstamp = syn->skb_mstamp;
+ syn->skb_mstamp = syn_data->skb_mstamp;
- if (tcp_transmit_skb(sk, syn_data, 0, sk->sk_allocation) == 0) {
+ /* Now full SYN+DATA was cloned and sent (or not),
+ * remove the SYN from the original skb (syn_data)
+ * we keep in write queue in case of a retransmit, as we
+ * also have the SYN packet (with no data) in the same queue.
+ */
+ TCP_SKB_CB(syn_data)->seq++;
+ TCP_SKB_CB(syn_data)->tcp_flags = TCPHDR_ACK | TCPHDR_PSH;
+ if (!err) {
tp->syn_data = (fo->copied > 0);
NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPORIGDATASENT);
goto done;
}
- syn_data = NULL;
fallback:
/* Send a regular SYN with Fast Open cookie request option */
@@ -3094,7 +3084,6 @@ fallback:
err = tcp_transmit_skb(sk, syn, 1, sk->sk_allocation);
if (err)
tp->syn_fastopen = 0;
- kfree_skb(syn_data);
done:
fo->cookie.len = -1; /* Exclude Fast Open option for SYN retries */
return err;
@@ -3114,13 +3103,10 @@ int tcp_connect(struct sock *sk)
return 0;
}
- buff = alloc_skb_fclone(MAX_TCP_HEADER + 15, sk->sk_allocation);
- if (unlikely(buff == NULL))
+ buff = sk_stream_alloc_skb(sk, 0, sk->sk_allocation);
+ if (unlikely(!buff))
return -ENOBUFS;
- /* Reserve space for headers. */
- skb_reserve(buff, MAX_TCP_HEADER);
-
tcp_init_nondata_skb(buff, tp->write_seq++, TCPHDR_SYN);
tp->retrans_stamp = tcp_time_stamp;
tcp_connect_queue_skb(sk, buff);
^ permalink raw reply related
* [PATCH net] vxlan: Inline vxlan_gso_check().
From: Joe Stringer @ 2014-11-18 0:24 UTC (permalink / raw)
To: netdev; +Cc: ogerlitz, linux-kernel
Suggested-by: Or Gerlitz <ogerlitz@mellanox.com>
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
drivers/net/vxlan.c | 19 -------------------
include/net/vxlan.h | 18 +++++++++++++++++-
2 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 6b65863..e1e335c 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -67,12 +67,6 @@
#define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags required value. */
-/* VXLAN protocol header */
-struct vxlanhdr {
- __be32 vx_flags;
- __be32 vx_vni;
-};
-
/* UDP port for VXLAN traffic.
* The IANA assigned port is 4789, but the Linux default is 8472
* for compatibility with early adopters.
@@ -1571,19 +1565,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
return false;
}
-bool vxlan_gso_check(struct sk_buff *skb)
-{
- if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
- (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
- skb->inner_protocol != htons(ETH_P_TEB) ||
- (skb_inner_mac_header(skb) - skb_transport_header(skb) !=
- sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
- return false;
-
- return true;
-}
-EXPORT_SYMBOL_GPL(vxlan_gso_check);
-
#if IS_ENABLED(CONFIG_IPV6)
static int vxlan6_xmit_skb(struct vxlan_sock *vs,
struct dst_entry *dst, struct sk_buff *skb,
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index afadf8e..57cccd0 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -8,6 +8,12 @@
#define VNI_HASH_BITS 10
#define VNI_HASH_SIZE (1<<VNI_HASH_BITS)
+/* VXLAN protocol header */
+struct vxlanhdr {
+ __be32 vx_flags;
+ __be32 vx_vni;
+};
+
struct vxlan_sock;
typedef void (vxlan_rcv_t)(struct vxlan_sock *vh, struct sk_buff *skb, __be32 key);
@@ -45,7 +51,17 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
__be32 src, __be32 dst, __u8 tos, __u8 ttl, __be16 df,
__be16 src_port, __be16 dst_port, __be32 vni, bool xnet);
-bool vxlan_gso_check(struct sk_buff *skb);
+static inline bool vxlan_gso_check(struct sk_buff *skb)
+{
+ if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
+ (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
+ skb->inner_protocol != htons(ETH_P_TEB) ||
+ (skb_inner_mac_header(skb) - skb_transport_header(skb) !=
+ sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
+ return false;
+
+ return true;
+}
/* IP header + UDP + VXLAN + Ethernet header */
#define VXLAN_HEADROOM (20 + 8 + 8 + 14)
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCHv2 net 1/4] net: Add vxlan_gso_check() helper
From: Joe Stringer @ 2014-11-18 0:26 UTC (permalink / raw)
To: Or Gerlitz; +Cc: Linux Netdev List
In-Reply-To: <CAJ3xEMhwq2Cuiw8sjY3DHFBEdmgKieNZgxWhGsfZ=Rtz41KhTQ@mail.gmail.com>
On Sunday, November 16, 2014 01:35:52 Or Gerlitz wrote:
> On Fri, Nov 14, 2014 at 2:38 AM, Joe Stringer <joestringer@nicira.com>
wrote:
> > Most NICs that report NETIF_F_GSO_UDP_TUNNEL support VXLAN, and not
> > other UDP-based encapsulation protocols where the format and size of the
> > header differs. This patch implements a generic ndo_gso_check() for
> > VXLAN which will only advertise GSO support when the skb looks like it
> > contains VXLAN (or no UDP tunnelling at all).
> >
> > Implementation shamelessly stolen from Tom Herbert:
> > http://thread.gmane.org/gmane.linux.network/332428/focus=333111
> >
> > Signed-off-by: Joe Stringer <joestringer@nicira.com>
> > ---
> > v2: Merge helpers for be2net, mlx4, qlcnic
> >
> > Use (sizeof(struct udphdr) + sizeof(struct vxlanhdr))
> >
> > v1: Initial post
> > ---
> >
> > drivers/net/vxlan.c | 13 +++++++++++++
> > include/net/vxlan.h | 2 ++
> > 2 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> > index fa9dc45..6b65863 100644
> > --- a/drivers/net/vxlan.c
> > +++ b/drivers/net/vxlan.c
> > @@ -1571,6 +1571,19 @@ static bool route_shortcircuit(struct net_device
> > *dev, struct sk_buff *skb)
> >
> > return false;
> >
> > }
> >
> > +bool vxlan_gso_check(struct sk_buff *skb)
> > +{
> > + if ((skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL) &&
> > + (skb->inner_protocol_type != ENCAP_TYPE_ETHER ||
> > + skb->inner_protocol != htons(ETH_P_TEB) ||
> > + (skb_inner_mac_header(skb) - skb_transport_header(skb) !=
> > + sizeof(struct udphdr) + sizeof(struct vxlanhdr))))
> > + return false;
> > +
> > + return true;
> > +}
> > +EXPORT_SYMBOL_GPL(vxlan_gso_check);
>
> Joe, any chance you can make the extra step and inline that in
> vxlan.h? this is fast path call... you will only need to move struct
> vxlanhdr there too.
Thanks for looking this over, I sent a patch.
^ permalink raw reply
* Re: [PATCH 2/4] arch: Add lightweight memory barriers fast_rmb() and fast_wmb()
From: Benjamin Herrenschmidt @ 2014-11-18 0:38 UTC (permalink / raw)
To: paulmck
Cc: Alexander Duyck, linux-arch, netdev, linux-kernel,
mathieu.desnoyers, peterz, heiko.carstens, mingo, mikey, linux,
donald.c.skidmore, matthew.vick, geert, jeffrey.t.kirsher, romieu,
nic_swsd, will.deacon, michael, tony.luck, torvalds, oleg,
schwidefsky, fweisbec, davem
In-Reply-To: <20141117201823.GD5050@linux.vnet.ibm.com>
On Mon, 2014-11-17 at 12:18 -0800, Paul E. McKenney wrote:
> On Mon, Nov 17, 2014 at 09:18:13AM -0800, Alexander Duyck wrote:
> > There are a number of situations where the mandatory barriers rmb() and
> > wmb() are used to order memory/memory operations in the device drivers
> > and those barriers are much heavier than they actually need to be. For
> > example in the case of PowerPC wmb() calls the heavy-weight sync
> > instruction when for memory/memory operations all that is really needed is
> > an lsync or eieio instruction.
>
> Is this still the case if one of the memory operations is MMIO? Last
> I knew, it was not.
I *think* (Alexander, correct me if I'm wrong), that what he wants is
the memory<->memory barriers (the smp_* ones) basically for ordering his
loads or stores from/to the DMA area.
The problem is that the smp_* ones aren't compiled for !CONFIG_SMP
IE. Something like:
- Read valid bit from descriptor
- Read rest of descriptor
That needs an rmb of some sort in between, but a full blown "rmb" will
also order vs. MMIOs and end up being a full sync, while an smp_rmb is a
lwsync which is more lightweight.
Similarily:
- Populate descriptor
- Write valid bit
Same deal with wmb ...
Basically, rmb and wmb order both cachable and non-cachable (memory and
MMIO) which makes them needlessly heavy on powerpc and possibly others
when all you need is to order memory accesses to some DMA data
structures. In that case you really want the normal smp_* variants
except they may not be around...
Cheers,
Ben.
^ permalink raw reply
* Re: Device Tree Binding for Marvell DSA Switch on imx28 board over Mdio Interface
From: Fabio Estevam @ 2014-11-18 0:39 UTC (permalink / raw)
To: Oliver Graute; +Cc: Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <CA+KjHfaKhQL6J3FZjZrs-eh3URBL5geDWV9adpcvo-6SU-9aRg@mail.gmail.com>
Hi Oliver,
On Mon, Nov 17, 2014 at 2:54 PM, Oliver Graute <oliver.graute@gmail.com> wrote:
> Hello Fabio,
>
> and what's the correct one for the imx28 CPU?
I am not familiar with it, sorry.
^ permalink raw reply
* Re: [PATCH 2/4] arch: Add lightweight memory barriers fast_rmb() and fast_wmb()
From: Benjamin Herrenschmidt @ 2014-11-18 0:39 UTC (permalink / raw)
To: Alexander Duyck
Cc: Alexander Duyck, linux-arch, netdev, linux-kernel,
mathieu.desnoyers, peterz, heiko.carstens, mingo, mikey, linux,
donald.c.skidmore, matthew.vick, geert, jeffrey.t.kirsher, romieu,
paulmck, nic_swsd, will.deacon, michael, tony.luck, torvalds,
oleg, schwidefsky, fweisbec, davem
In-Reply-To: <546A5968.1090201@gmail.com>
On Mon, 2014-11-17 at 12:24 -0800, Alexander Duyck wrote:
> Yes and no. So for example on ARM I used the dmb() operation, however
> I
> have to use the barrier at the system level instead of just the inner
> shared domain. However on many other architectures they are just the
> same as the smp_* variants.
>
> Basically the resultant code is somewhere between the smp and non-smp
> barriers in terms of what they cover.
There I don't quite follow you. You need to explain better especially in
the documentation because otherwise people will get it wrong...
If it's ordering in the coherent domain, I fail to see how a DMA agent
is different than another processor when it comes to barriers, so I fail
to see the difference with smp_*
I understand the MMIO vs. memory issue, we do have the same on powerpc,
but that other aspect eludes me.
Ben.
^ permalink raw reply
* Re: [PATCH 2/4] arch: Add lightweight memory barriers fast_rmb() and fast_wmb()
From: Benjamin Herrenschmidt @ 2014-11-18 0:41 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexander Duyck, linux-arch@vger.kernel.org, Network Development,
Linux Kernel Mailing List, Mathieu Desnoyers, Peter Zijlstra,
Heiko Carstens, Ingo Molnar, Michael Neuling,
Russell King - ARM Linux, donald.c.skidmore, matthew.vick,
Geert Uytterhoeven, Jeff Kirsher, Francois Romieu, Paul McKenney,
nic_swsd, Will Deacon, Michael Ellerman, Tony Luck, Oleg Nesterov,
Martin Schwidefsky <sch
In-Reply-To: <CA+55aFxZ9dhB074p3OAQLyQ53TuRjVOo33h6OOda1soh-9xxnQ@mail.gmail.com>
On Mon, 2014-11-17 at 12:52 -0800, Linus Torvalds wrote:
> Maybe "dma_*mb()" would be acceptable, and ends up having the same
> naming convention as "smb_*mb()", and explains what it's about.
Yes, that was what I was about to suggest as well.
> And yes, in the same spirit, it would probably be good to try to
> eventually get rid of the plain "*mb()" functions, and perhaps call
> them "mmio_*mb()" to clarify that they are about ordering memory wrt
> mmio.
It will always be somewhat unclear to users who don't read the doc
anyway :)
IE. the dma_* ones do only DMA vs DMA (or vs other processors) but the
mmio_* one do anything vs anything. Not a huge deal tho. I still like
dma_* for Alexander's new stuff but I wouldn't bother with changing the
existing ones.
Ben.
^ permalink raw reply
* Re: [PATCH 2/4] arch: Add lightweight memory barriers fast_rmb() and fast_wmb()
From: Benjamin Herrenschmidt @ 2014-11-18 0:43 UTC (permalink / raw)
To: Alexander Duyck
Cc: Linus Torvalds, linux-arch@vger.kernel.org, Network Development,
Linux Kernel Mailing List, Mathieu Desnoyers, Peter Zijlstra,
Heiko Carstens, Ingo Molnar, Michael Neuling,
Russell King - ARM Linux, donald.c.skidmore, matthew.vick,
Geert Uytterhoeven, Jeff Kirsher, Francois Romieu, Paul McKenney,
nic_swsd, Will Deacon, Michael Ellerman, Tony Luck, Oleg Nesterov,
Martin Schwidefsky <sch
In-Reply-To: <546A6EB2.6020304@redhat.com>
On Mon, 2014-11-17 at 13:54 -0800, Alexander Duyck wrote:
> What would you think of the name "coherent_*mb()"? I would prefer to
> avoid dma in the name since, at least in my mind, that implies MMIO.
I'm lazy, I like typing less, so I like dma_* but I don't object to
coherent_* if at least one more person thinks it makes the semantics
clearer :)
> It also ties in well with dma_alloc_coherent/dma_free_coherent which is
> what would typically be used to allocate the memory we would be using
> the barrier to protect anyway.
Agreed.
> > And yes, in the same spirit, it would probably be good to try to
> > eventually get rid of the plain "*mb()" functions, and perhaps call
> > them "mmio_*mb()" to clarify that they are about ordering memory wrt
> > mmio.
> >
> > Hmm?
> >
> > Linus
>
> I will work on pulling all of the coherent barrier cases out of using
> the plain "*mb()" calls first. We need to sort that out before we could
> look at renaming the plain barrier functions.
Makes sense.
Cheers,
Ben.
> - Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH] [bonding]: clear header_ops when last slave detached
From: Jay Vosburgh @ 2014-11-18 1:38 UTC (permalink / raw)
To: Wengang; +Cc: netdev
In-Reply-To: <54694D09.4080304@oracle.com>
Wengang <wen.gang.wang@oracle.com> wrote:
>Hi,
>
>Could anybody please review this patch?
I don't see that the original of this ever came through netdev.
>thanks,
>wengang
>
>于 2014年11月13日 10:19, Wengang Wang 写道:
>> When last slave of a bonding master is removed, the bonding then does not work.
>> When packet_snd is called against with a master net_device, it accesses
>> header_ops. In case the header_ops is not valid any longer(say module unloaded)
>> it will then access an invalid memory address.
>> This patch try to fix this issue by clearing header_ops when last slave
>> detached.
Am I correct in presuming that this behavior is limited to ipoib
slaves only? I don't see that this could occur with ethernet slaves, as
eth_header_ops isn't part of a module. This needs to be mentioned in
the commit log.
>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>> ---
>> drivers/net/bonding/bond_main.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index c9ac06c..84a34fc 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1728,6 +1728,8 @@ static int __bond_release_one(struct net_device *bond_dev,
>> unblock_netpoll_tx();
>> synchronize_rcu();
>> bond->slave_cnt--;
>> + if (!bond->slave_cnt)
>> + bond->dev->header_ops = NULL;
>> if (!bond_has_slaves(bond)) {
>> call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
I believe your addition could be moved into the block for the
next if, as "!bond->slave_cnt" is essentially "!bond_has_slaves()".
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply
* Re: [PATCH net-next] PPC: bpf_jit_comp: Unify BPF_MOD | BPF_X and BPF_DIV | BPF_X
From: Michael Ellerman @ 2014-11-18 1:50 UTC (permalink / raw)
To: Denis Kirjanov
Cc: netdev, Philippe Bergheaud, linuxppc-dev, Daniel Borkmann,
Alexei Starovoitov
In-Reply-To: <1416254861-3879-1-git-send-email-kda@linux-powerpc.org>
On Mon, 2014-11-17 at 23:07 +0300, Denis Kirjanov wrote:
> Reduce duplicated code by unifying
> BPF_ALU | BPF_MOD | BPF_X and BPF_ALU | BPF_DIV | BPF_X
>
> CC: Alexei Starovoitov<alexei.starovoitov@gmail.com>
> CC: Daniel Borkmann<dborkman@redhat.com>
> CC: Philippe Bergheaud<felix@linux.vnet.ibm.com>
> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
Please include the output of the test suite.
Assuming that's OK I'm happy for it to go in.
cheers
^ permalink raw reply
* Re: [PATCH] [bonding]: clear header_ops when last slave detached
From: Wengang @ 2014-11-18 1:56 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev
In-Reply-To: <866.1416274729@famine>
Hi Jay,
于 2014年11月18日 09:38, Jay Vosburgh 写道:
> Wengang <wen.gang.wang@oracle.com> wrote:
>
>> Hi,
>>
>> Could anybody please review this patch?
> I don't see that the original of this ever came through netdev.
Oh, that' bad. I sent this to netdev@vger.kernel.org. The mail address
is wrong?
>> thanks,
>> wengang
>>
>> 于 2014年11月13日 10:19, Wengang Wang 写道:
>>> When last slave of a bonding master is removed, the bonding then does not work.
>>> When packet_snd is called against with a master net_device, it accesses
>>> header_ops. In case the header_ops is not valid any longer(say module unloaded)
>>> it will then access an invalid memory address.
>>> This patch try to fix this issue by clearing header_ops when last slave
>>> detached.
> Am I correct in presuming that this behavior is limited to ipoib
> slaves only? I don't see that this could occur with ethernet slaves, as
> eth_header_ops isn't part of a module. This needs to be mentioned in
> the commit log.
Yes, the problem is found with ipoib slaves.
>>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>>> ---
>>> drivers/net/bonding/bond_main.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index c9ac06c..84a34fc 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -1728,6 +1728,8 @@ static int __bond_release_one(struct net_device *bond_dev,
>>> unblock_netpoll_tx();
>>> synchronize_rcu();
>>> bond->slave_cnt--;
>>> + if (!bond->slave_cnt)
>>> + bond->dev->header_ops = NULL;
>>> if (!bond_has_slaves(bond)) {
>>> call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
> I believe your addition could be moved into the block for the
> next if, as "!bond->slave_cnt" is essentially "!bond_has_slaves()".
Yes, Agree.
I will send the second prompt soon with commit message mentioning ipoib.
thanks,
wengang
> -J
>
> ---
> -Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply
* [PATCH (sent originally to netdev)] device: Add dev_<level>_once variants
From: Joe Perches @ 2014-11-18 2:18 UTC (permalink / raw)
To: Greg KH; +Cc: Jeff Kirsher, davem, Florian Fainelli, netdev, LKML
In-Reply-To: <5467C3BA.6090003@gmail.com>
Add the equivalents to pr_<level>_once.
Suggested-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
include/linux/device.h | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/include/linux/device.h b/include/linux/device.h
index ce1f2160..a1ee071 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1118,6 +1118,41 @@ do { \
})
#endif
+#ifdef CONFIG_PRINTK
+#define dev_level_once(dev_level, dev, fmt, ...) \
+do { \
+ static bool __print_once __read_mostly; \
+ \
+ if (!__print_once) { \
+ __print_once = true; \
+ dev_level(dev, fmt, ##__VA_ARGS__); \
+ } \
+} while (0)
+#else
+#define dev_level_once(dev_level, dev, fmt, ...) \
+do { \
+ if (0) \
+ dev_level(dev, fmt, ##__VA_ARGS__); \
+} while (0)
+#endif
+
+#define dev_emerg_once(dev, fmt, ...) \
+ dev_level_once(dev_emerg, dev, fmt, ##__VA_ARGS__)
+#define dev_alert_once(dev, fmt, ...) \
+ dev_level_once(dev_alert, dev, fmt, ##__VA_ARGS__)
+#define dev_crit_once(dev, fmt, ...) \
+ dev_level_once(dev_crit, dev, fmt, ##__VA_ARGS__)
+#define dev_err_once(dev, fmt, ...) \
+ dev_level_once(dev_err, dev, fmt, ##__VA_ARGS__)
+#define dev_warn_once(dev, fmt, ...) \
+ dev_level_once(dev_warn, dev, fmt, ##__VA_ARGS__)
+#define dev_notice_once(dev, fmt, ...) \
+ dev_level_once(dev_notice, dev, fmt, ##__VA_ARGS__)
+#define dev_info_once(dev, fmt, ...) \
+ dev_level_once(dev_info, dev, fmt, ##__VA_ARGS__)
+#define dev_dbg_once(dev, fmt, ...) \
+ dev_level_once(dev_info, dev, fmt, ##__VA_ARGS__)
+
#define dev_level_ratelimited(dev_level, dev, fmt, ...) \
do { \
static DEFINE_RATELIMIT_STATE(_rs, \
^ permalink raw reply related
* Re: [PATCH] [bonding]: clear header_ops when last slave detached
From: Eric Dumazet @ 2014-11-18 2:19 UTC (permalink / raw)
To: Wengang; +Cc: Jay Vosburgh, netdev
In-Reply-To: <546AA746.8090008@oracle.com>
On Tue, 2014-11-18 at 09:56 +0800, Wengang wrote:
> Hi Jay,
>
> 于 2014年11月18日 09:38, Jay Vosburgh 写道:
> > Wengang <wen.gang.wang@oracle.com> wrote:
> >
> >> Hi,
> >>
> >> Could anybody please review this patch?
> > I don't see that the original of this ever came through netdev.
>
> Oh, that' bad. I sent this to netdev@vger.kernel.org. The mail address
> is wrong?
>
> >> thanks,
> >> wengang
> >>
> >> 于 2014年11月13日 10:19, Wengang Wang 写道:
> >>> When last slave of a bonding master is removed, the bonding then does not work.
> >>> When packet_snd is called against with a master net_device, it accesses
> >>> header_ops. In case the header_ops is not valid any longer(say module unloaded)
> >>> it will then access an invalid memory address.
> >>> This patch try to fix this issue by clearing header_ops when last slave
> >>> detached.
> > Am I correct in presuming that this behavior is limited to ipoib
> > slaves only? I don't see that this could occur with ethernet slaves, as
> > eth_header_ops isn't part of a module. This needs to be mentioned in
> > the commit log.
> Yes, the problem is found with ipoib slaves.
> >>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> >>> ---
> >>> drivers/net/bonding/bond_main.c | 2 ++
> >>> 1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >>> index c9ac06c..84a34fc 100644
> >>> --- a/drivers/net/bonding/bond_main.c
> >>> +++ b/drivers/net/bonding/bond_main.c
> >>> @@ -1728,6 +1728,8 @@ static int __bond_release_one(struct net_device *bond_dev,
> >>> unblock_netpoll_tx();
> >>> synchronize_rcu();
> >>> bond->slave_cnt--;
> >>> + if (!bond->slave_cnt)
> >>> + bond->dev->header_ops = NULL;
> >>> if (!bond_has_slaves(bond)) {
> >>> call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
> > I believe your addition could be moved into the block for the
> > next if, as "!bond->slave_cnt" is essentially "!bond_has_slaves()".
>
> Yes, Agree.
> I will send the second prompt soon with commit message mentioning ipoib.
I really don't like this patch. Its quite racy.
bond_setup_by_slave() kind of assume slave_dev->header_ops is always
present.
No rcu protection, no module refcount protection for struct header_ops
Considering ipoib_hard_header() is quite small, you might instead move
ipoib_hard_header() and ipoib_header_ops in static vmlinux, like we do
for eth_header_ops.
^ permalink raw reply
* Re: [PATCH (sent originally to netdev)] device: Add dev_<level>_once variants
From: Jeff Kirsher @ 2014-11-18 2:23 UTC (permalink / raw)
To: Joe Perches; +Cc: Greg KH, davem, Florian Fainelli, netdev, LKML
In-Reply-To: <1416277102.29010.14.camel@perches.com>
[-- Attachment #1: Type: text/plain, Size: 381 bytes --]
On Mon, 2014-11-17 at 18:18 -0800, Joe Perches wrote:
> Add the equivalents to pr_<level>_once.
>
> Suggested-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Joe Perches <joe@perches.com>
Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> include/linux/device.h | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] [bonding]: clear header_ops when last slave detached
From: Jay Vosburgh @ 2014-11-18 2:58 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Wengang, netdev
In-Reply-To: <1416277163.18588.33.camel@edumazet-glaptop2.roam.corp.google.com>
Eric Dumazet <eric.dumazet@gmail.com> wrote:
>On Tue, 2014-11-18 at 09:56 +0800, Wengang wrote:
>> Hi Jay,
>>
>> 于 2014年11月18日 09:38, Jay Vosburgh 写道:
>> > Wengang <wen.gang.wang@oracle.com> wrote:
>> >
>> >> Hi,
>> >>
>> >> Could anybody please review this patch?
>> > I don't see that the original of this ever came through netdev.
>>
>> Oh, that' bad. I sent this to netdev@vger.kernel.org. The mail address
>> is wrong?
>>
>> >> thanks,
>> >> wengang
>> >>
>> >> 于 2014年11月13日 10:19, Wengang Wang 写道:
>> >>> When last slave of a bonding master is removed, the bonding then does not work.
>> >>> When packet_snd is called against with a master net_device, it accesses
>> >>> header_ops. In case the header_ops is not valid any longer(say module unloaded)
>> >>> it will then access an invalid memory address.
>> >>> This patch try to fix this issue by clearing header_ops when last slave
>> >>> detached.
>> > Am I correct in presuming that this behavior is limited to ipoib
>> > slaves only? I don't see that this could occur with ethernet slaves, as
>> > eth_header_ops isn't part of a module. This needs to be mentioned in
>> > the commit log.
>> Yes, the problem is found with ipoib slaves.
>> >>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>> >>> ---
>> >>> drivers/net/bonding/bond_main.c | 2 ++
>> >>> 1 file changed, 2 insertions(+)
>> >>>
>> >>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >>> index c9ac06c..84a34fc 100644
>> >>> --- a/drivers/net/bonding/bond_main.c
>> >>> +++ b/drivers/net/bonding/bond_main.c
>> >>> @@ -1728,6 +1728,8 @@ static int __bond_release_one(struct net_device *bond_dev,
>> >>> unblock_netpoll_tx();
>> >>> synchronize_rcu();
>> >>> bond->slave_cnt--;
>> >>> + if (!bond->slave_cnt)
>> >>> + bond->dev->header_ops = NULL;
>> >>> if (!bond_has_slaves(bond)) {
>> >>> call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
>> > I believe your addition could be moved into the block for the
>> > next if, as "!bond->slave_cnt" is essentially "!bond_has_slaves()".
>>
>> Yes, Agree.
>> I will send the second prompt soon with commit message mentioning ipoib.
>
>I really don't like this patch. Its quite racy.
>
>bond_setup_by_slave() kind of assume slave_dev->header_ops is always
>present.
Isn't the ipoib header_ops implicitly gated by the presence or
absence of the module itself? An ipoib device can't be enslaved unless
ipoib is loaded, and if ipoib is loaded, the ops are present. And ipoib
can't be removed while there are interfaces enslaved to bonding.
I'm not saying it's not ugly, but I'm not seeing why it won't
work or what the race would be.
>No rcu protection, no module refcount protection for struct header_ops
>
>Considering ipoib_hard_header() is quite small, you might instead move
>ipoib_hard_header() and ipoib_header_ops in static vmlinux, like we do
>for eth_header_ops.
Won't this require including all of the functions referenced by
the ops? The problem here is that packet_snd will call dev_hard_header,
which wants to call header_ops->create.
Ok, now that I check, there's only one op in ipoib_header_ops,
->create, and it's fairly simple.
There was a similar chicken and egg problem with bonding and
ipoib a while back related to the master device having a dangling
pointer into ipoib somewhere; that might have been the header_ops as
well, so there may be a hack or two that could be removed if the ops
cannot disappear.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox