Netdev List
 help / color / mirror / Atom feed
* [PATCH] sunrpc: fix rpcb_clnt.c kernel-doc notation
From: Randy Dunlap @ 2012-04-18  0:03 UTC (permalink / raw)
  To: netdev, davem

From: Randy Dunlap <rdunlap@xenotime.net>

Fix kernel-doc warnings in sunrpc/rpcb_clnt.c:

Warning(net/sunrpc/rpcb_clnt.c:428): No description found for parameter 'net'
Warning(net/sunrpc/rpcb_clnt.c:567): No description found for parameter 'net'

Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
---
 net/sunrpc/rpcb_clnt.c |    2 ++
 1 file changed, 2 insertions(+)

--- lnx-34-rc3.orig/net/sunrpc/rpcb_clnt.c
+++ lnx-34-rc3/net/sunrpc/rpcb_clnt.c
@@ -394,6 +394,7 @@ static int rpcb_register_call(struct rpc
 
 /**
  * rpcb_register - set or unset a port registration with the local rpcbind svc
+ * @net: target network namespace
  * @prog: RPC program number to bind
  * @vers: RPC version number to bind
  * @prot: transport protocol to register
@@ -521,6 +522,7 @@ static int rpcb_unregister_all_protofami
 
 /**
  * rpcb_v4_register - set or unset a port registration with the local rpcbind
+ * @net: target network namespace
  * @program: RPC program number of service to (un)register
  * @version: RPC version number of service to (un)register
  * @address: address family, IP address, and port to (un)register

^ permalink raw reply

* [PATCH] net/sock.h: fix sk_peek_off kernel-doc warning
From: Randy Dunlap @ 2012-04-18  0:03 UTC (permalink / raw)
  To: netdev, davem

From: Randy Dunlap <rdunlap@xenotime.net>

Fix kernel-doc warning in net/sock.h:

Warning(include/net/sock.h:377): No description found for parameter 'sk_peek_off'

Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
---
 include/net/sock.h |    1 +
 1 file changed, 1 insertion(+)

--- lnx-34-rc3.orig/include/net/sock.h
+++ lnx-34-rc3/include/net/sock.h
@@ -246,6 +246,7 @@ struct cg_proto;
   *	@sk_user_data: RPC layer private data
   *	@sk_sndmsg_page: cached page for sendmsg
   *	@sk_sndmsg_off: cached offset for sendmsg
+  *	@sk_peek_off: current peek_offset value
   *	@sk_send_head: front of stuff to transmit
   *	@sk_security: used by security modules
   *	@sk_mark: generic packet mark

^ permalink raw reply

* [net 2/2] ixgbe: Identify FCoE rings earlier to resolve memory corruption w/ FCoE
From: Jeff Kirsher @ 2012-04-17 22:48 UTC (permalink / raw)
  To: davem; +Cc: Alexander Duyck, netdev, gospo, sassmann, Jeff Kirsher
In-Reply-To: <1334702885-4411-1-git-send-email-jeffrey.t.kirsher@intel.com>

From: Alexander Duyck <alexander.h.duyck@intel.com>

This patch makes it so that we identify FCoE rings earlier than
ixgbe_set_rx_buffer_len.  Instead we identify the Rx FCoE rings at
allocation time in ixgbe_alloc_q_vector.

The motivation behind this change is to avoid memory corruption when FCoE
is enabled.  Without this change we were initializing the rings at 0, and
2K on systems with 4K pages, then when we bumped the buffer size to 4K with
order 1 pages we were accessing offsets 2K and 6K instead of 0 and 4K.
This was resulting in memory corruptions.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Acked-by: Yi Zou <yi.zou@intel.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c  |   10 ++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    8 --------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
index 027d7a7..ed1b47d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
@@ -622,6 +622,16 @@ static int ixgbe_alloc_q_vector(struct ixgbe_adapter *adapter, int v_idx,
 		if (adapter->hw.mac.type == ixgbe_mac_82599EB)
 			set_bit(__IXGBE_RX_CSUM_UDP_ZERO_ERR, &ring->state);
 
+#ifdef IXGBE_FCOE
+		if (adapter->netdev->features & NETIF_F_FCOE_MTU) {
+			struct ixgbe_ring_feature *f;
+			f = &adapter->ring_feature[RING_F_FCOE];
+			if ((rxr_idx >= f->mask) &&
+			    (rxr_idx < f->mask + f->indices))
+				set_bit(__IXGBE_RX_FCOE_BUFSZ, &ring->state);
+		}
+
+#endif /* IXGBE_FCOE */
 		/* apply Rx specific ring traits */
 		ring->count = adapter->rx_ring_count;
 		ring->queue_index = rxr_idx;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 9e2be8c..a7f3cd8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3154,14 +3154,6 @@ static void ixgbe_set_rx_buffer_len(struct ixgbe_adapter *adapter)
 			set_ring_rsc_enabled(rx_ring);
 		else
 			clear_ring_rsc_enabled(rx_ring);
-#ifdef IXGBE_FCOE
-		if (netdev->features & NETIF_F_FCOE_MTU) {
-			struct ixgbe_ring_feature *f;
-			f = &adapter->ring_feature[RING_F_FCOE];
-			if ((i >= f->mask) && (i < f->mask + f->indices))
-				set_bit(__IXGBE_RX_FCOE_BUFSZ, &rx_ring->state);
-		}
-#endif /* IXGBE_FCOE */
 	}
 }
 
-- 
1.7.7.6

^ permalink raw reply related

* [net 1/2] ixgbe: add missing rtnl_lock in PM resume path
From: Jeff Kirsher @ 2012-04-17 22:48 UTC (permalink / raw)
  To: davem; +Cc: Benjamin Poirier, netdev, gospo, sassmann, Jeff Kirsher
In-Reply-To: <1334702885-4411-1-git-send-email-jeffrey.t.kirsher@intel.com>

From: Benjamin Poirier <bpoirier@suse.de>

Upon resume from standby, ixgbe may trigger the ASSERT_RTNL() in
netif_set_real_num_tx_queues(). The call stack is:
	netif_set_real_num_tx_queues
	ixgbe_set_num_queues
	ixgbe_init_interrupt_scheme
	ixgbe_resume

Signed-off-by: Benjamin Poirier <bpoirier@suse.de>
Tested-by: Stephen Ko <stephen.s.ko@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index dac7c01..9e2be8c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -4836,7 +4836,9 @@ static int ixgbe_resume(struct pci_dev *pdev)
 
 	pci_wake_from_d3(pdev, false);
 
+	rtnl_lock();
 	err = ixgbe_init_interrupt_scheme(adapter);
+	rtnl_unlock();
 	if (err) {
 		e_dev_err("Cannot initialize interrupts for device\n");
 		return err;
-- 
1.7.7.6

^ permalink raw reply related

* [net 0/2][pull request] Intel Wired LAN Driver Updates
From: Jeff Kirsher @ 2012-04-17 22:48 UTC (permalink / raw)
  To: davem; +Cc: Jeff Kirsher, netdev, gospo, sassmann

This series of patches contains fixes for ixgbe only.

The following are changes since commit a99ff7d0123b19ecad3b589480b6542716ab6b52:
  net: usb: smsc75xx: fix mtu
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net master

Alexander Duyck (1):
  ixgbe: Identify FCoE rings earlier to resolve memory corruption w/
    FCoE

Benjamin Poirier (1):
  ixgbe: add missing rtnl_lock in PM resume path

 drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c  |   10 ++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   10 ++--------
 2 files changed, 12 insertions(+), 8 deletions(-)

-- 
1.7.7.6

^ permalink raw reply

* Re: TSO not 10G friendly if peer is close enough
From: David Miller @ 2012-04-17 21:47 UTC (permalink / raw)
  To: eric.dumazet
  Cc: alexander.h.duyck, jeffrey.t.kirsher, donald.c.skidmore,
	gregory.v.rose, john.r.fastabend, jesse.brandeburg, netdev
In-Reply-To: <1334698722.2472.71.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 17 Apr 2012 23:38:42 +0200

> Hmm... maybe tcp_trim_head should not trim but only update an offset in
> skb... With some luck, offset can reach skb->len when all data is
> ACKnowledged...

This is definitely the way to fix this.  Just essentially defer all
the page operations until later when the entire SKB is consumed.

^ permalink raw reply

* Re: [PATCH net-next] dmfe: enforce consistent timing delay.
From: Grant Grundler @ 2012-04-17 21:45 UTC (permalink / raw)
  To: Francois Romieu; +Cc: netdev, David Miller
In-Reply-To: <20120417211140.GA12909@electric-eye.fr.zoreil.com>

On Tue, Apr 17, 2012 at 2:11 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> The driver does not always use the same timing for what looks like
> the same operations.
>
> - DCR0
>  Use the same udelay everywhere for reset. Upper bound is 100 us.
> - DCR9
>  Use 5us delay for srom clock. 1us delay for phy_write_1bit (writes
>  PHY_DATA_[01]) are not changed as they stay withing a 2,5MHz MDIO
>  clock range.
>
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>


Reviewed-by: Grant Grundler <grundler@parisc-linux.org>

> ---
>
> I noticed those while staring at the driver.
>
> Grant, regarding your previous remarks, I do not see where the problem
> could be with posted writes MMIO accesses:
> - the driver has always used the first (#0) bar register. It's hardcoded.
>  The driver still uses this same bar register.
> - the driver has never checked whether bar #0 was related to I/O or
>  memory space. It assumed an I/O decoder and issued out* instructions.
> - the DM9102 datasheet states BAR #0 is an IO only BAR. No idea for the
>  DM9132, 9100 and 9009 though.

I didn't realize BAR0 is hard coded in the driver. BAR0 unlikely to
become MMIO for other devices. I think you are right - there is no
problem.

> My changes have replaced in/out* accesses with ioread/write* - mostly
> to help detecting places where type checking would have uncloaked a
> forgotten net_device.base_addr.
>
> Thus, if someone has a dmfe device including a MMIO space decoder behind
> bar #0, iowrite* will now emit memory write instructions where the driver
> previously used I/O operations. It may not work very well due to posted
> writes issues, sure.

unlikely. I'll worry about it when someone does in fact find this configuration.

> However I wonder on which platform - if any - the
> former MMIO space + I/O accesses (!) combo would have behaved correctly.
> ?

MMIO and I/O can interoperate to the same device since the device
registers are just having transactions sent to them via different
address decoders. Drivers typically only use one or the other though
if both are available.

thanks,
grant

>
>  drivers/net/ethernet/dec/tulip/dmfe.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ethernet/dec/tulip/dmfe.c b/drivers/net/ethernet/dec/tulip/dmfe.c
> index 0ef5b68..4d6fe60 100644
> --- a/drivers/net/ethernet/dec/tulip/dmfe.c
> +++ b/drivers/net/ethernet/dec/tulip/dmfe.c
> @@ -767,7 +767,7 @@ static int dmfe_stop(struct DEVICE *dev)
>
>        /* Reset & stop DM910X board */
>        dw32(DCR0, DM910X_RESET);
> -       udelay(5);
> +       udelay(100);
>        phy_write(ioaddr, db->phy_addr, 0, 0x8000, db->chip_id);
>
>        /* free interrupt */
> @@ -1601,7 +1601,9 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
>        int i;
>
>        dw32(DCR9, CR9_SROM_READ);
> +       udelay(5);
>        dw32(DCR9, CR9_SROM_READ | CR9_SRCS);
> +       udelay(5);
>
>        /* Send the Read Command 110b */
>        srom_clk_write(ioaddr, SROM_DATA_1);
> @@ -1615,6 +1617,7 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
>        }
>
>        dw32(DCR9, CR9_SROM_READ | CR9_SRCS);
> +       udelay(5);
>
>        for (i = 16; i > 0; i--) {
>                dw32(DCR9, CR9_SROM_READ | CR9_SRCS | CR9_SRCLK);
> @@ -1626,6 +1629,7 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
>        }
>
>        dw32(DCR9, CR9_SROM_READ);
> +       udelay(5);
>        return srom_data;
>  }
>
> --
> 1.7.7.6
>

^ permalink raw reply

* TSO not 10G friendly if peer is close enough
From: Eric Dumazet @ 2012-04-17 21:38 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: jeffrey.t.kirsher, Skidmore, Donald C, Greg Rose, John Fastabend,
	Jesse Brandeburg, netdev
In-Reply-To: <1334681204.2472.41.camel@edumazet-glaptop>

After further analysis, I found we hit badly page refcounts games,
because when we transmit full size skb (64 KB), we can receive ACK for
the first MSS of the frame while skb was not completely sent by NIC.

(Needs 52 us to send a full TSO frame at 10Gb, and maybe NIC delays
interrupt to trigger TX completion ?)

In this case, tcp_trim_head() has to call pskb_expand_head(), because
skb clone is still alive in TX ring buffer.

pskb_expand_head() is really expensive, it has to make about 32 atomic
operations on page refcounts.

Hmm... maybe tcp_trim_head should not trim but only update an offset in
skb... With some luck, offset can reach skb->len when all data is
ACKnowledged...

Only in case of retransmit we would need to really trim the skb, and by
this time, clone would had been freed to : No more pskb_expand_head()
calls.

^ permalink raw reply

* Re: [PATCH 2/2] ath9k: Gather and report IRQ sync_cause errors.
From: Gabor Juhos @ 2012-04-17 21:29 UTC (permalink / raw)
  To: Ben Greear
  Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4F8DDCF1.1080304-my8/4N5VtI7c+919tysfdA@public.gmane.org>

2012.04.17. 23:13 keltezéssel, Ben Greear írta:
> On 04/17/2012 02:07 PM, Gabor Juhos wrote:
>>> From: Ben Greear<greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
>>>
>>> Report all defined sync_cause errors in debugfs
>>> to aid with debugging.
>>>
>>> Use a macro to print out the interrupts file contents
>>> to decrease code duplication.
>>
>> Now, that the patch is in wireless-testing, I have noticed a minor problem
>> with it.
> 
> Thanks for the report.

Np.

> I'll fix this shortly.

Thanks,
Gabor

> 
> Thanks,
> Ben
> 
>>> +#ifdef CONFIG_ATH9K_DEBUGFS
>>> +void ath9k_debug_sync_cause(struct ath_common *common, u32 sync_cause);
>>> +#else
>>> +static void ath9k_debug_sync_cause(struct ath_common *common, u32
>>> sync_cause) {}
>>
>> Due to a missing 'inline' keyword, this causes a bunch of warnings if
>> CONFIG_ATH9K_DEBUGFS is not enabled:
>>
>>     CC [M]  drivers/net/wireless/ath/main.o
>>     CC [M]  drivers/net/wireless/ath/regd.o
>>     CC [M]  drivers/net/wireless/ath/hw.o
>>     CC [M]  drivers/net/wireless/ath/key.o
>>     LD [M]  drivers/net/wireless/ath/ath.o
>>     CC [M]  drivers/net/wireless/ath/ath9k/beacon.o
>>   drivers/net/wireless/ath/ath9k/hw.h:962: warning: ‘ath9k_debug_sync_cause’
>> defined but not used
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH net-next] dmfe: enforce consistent timing delay.
From: Francois Romieu @ 2012-04-17 21:11 UTC (permalink / raw)
  To: Grant Grundler; +Cc: netdev, David Miller

The driver does not always use the same timing for what looks like
the same operations.

- DCR0
  Use the same udelay everywhere for reset. Upper bound is 100 us.
- DCR9
  Use 5us delay for srom clock. 1us delay for phy_write_1bit (writes
  PHY_DATA_[01]) are not changed as they stay withing a 2,5MHz MDIO
  clock range.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---

I noticed those while staring at the driver.

Grant, regarding your previous remarks, I do not see where the problem
could be with posted writes MMIO accesses:
- the driver has always used the first (#0) bar register. It's hardcoded.
  The driver still uses this same bar register.
- the driver has never checked whether bar #0 was related to I/O or
  memory space. It assumed an I/O decoder and issued out* instructions.
- the DM9102 datasheet states BAR #0 is an IO only BAR. No idea for the
  DM9132, 9100 and 9009 though.

My changes have replaced in/out* accesses with ioread/write* - mostly
to help detecting places where type checking would have uncloaked a
forgotten net_device.base_addr.

Thus, if someone has a dmfe device including a MMIO space decoder behind
bar #0, iowrite* will now emit memory write instructions where the driver
previously used I/O operations. It may not work very well due to posted
writes issues, sure. However I wonder on which platform - if any - the
former MMIO space + I/O accesses (!) combo would have behaved correctly.

?

 drivers/net/ethernet/dec/tulip/dmfe.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/dec/tulip/dmfe.c b/drivers/net/ethernet/dec/tulip/dmfe.c
index 0ef5b68..4d6fe60 100644
--- a/drivers/net/ethernet/dec/tulip/dmfe.c
+++ b/drivers/net/ethernet/dec/tulip/dmfe.c
@@ -767,7 +767,7 @@ static int dmfe_stop(struct DEVICE *dev)
 
 	/* Reset & stop DM910X board */
 	dw32(DCR0, DM910X_RESET);
-	udelay(5);
+	udelay(100);
 	phy_write(ioaddr, db->phy_addr, 0, 0x8000, db->chip_id);
 
 	/* free interrupt */
@@ -1601,7 +1601,9 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
 	int i;
 
 	dw32(DCR9, CR9_SROM_READ);
+	udelay(5);
 	dw32(DCR9, CR9_SROM_READ | CR9_SRCS);
+	udelay(5);
 
 	/* Send the Read Command 110b */
 	srom_clk_write(ioaddr, SROM_DATA_1);
@@ -1615,6 +1617,7 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
 	}
 
 	dw32(DCR9, CR9_SROM_READ | CR9_SRCS);
+	udelay(5);
 
 	for (i = 16; i > 0; i--) {
 		dw32(DCR9, CR9_SROM_READ | CR9_SRCS | CR9_SRCLK);
@@ -1626,6 +1629,7 @@ static u16 read_srom_word(void __iomem *ioaddr, int offset)
 	}
 
 	dw32(DCR9, CR9_SROM_READ);
+	udelay(5);
 	return srom_data;
 }
 
-- 
1.7.7.6

^ permalink raw reply related

* Re: [PATCH 2/2] ath9k: Gather and report IRQ sync_cause errors.
From: Ben Greear @ 2012-04-17 21:13 UTC (permalink / raw)
  To: Gabor Juhos; +Cc: linux-wireless, netdev
In-Reply-To: <4F8DDB9F.8030302@openwrt.org>

On 04/17/2012 02:07 PM, Gabor Juhos wrote:
>> From: Ben Greear<greearb@candelatech.com>
>>
>> Report all defined sync_cause errors in debugfs
>> to aid with debugging.
>>
>> Use a macro to print out the interrupts file contents
>> to decrease code duplication.
>
> Now, that the patch is in wireless-testing, I have noticed a minor problem with it.

Thanks for the report.

I'll fix this shortly.

Thanks,
Ben

>> +#ifdef CONFIG_ATH9K_DEBUGFS
>> +void ath9k_debug_sync_cause(struct ath_common *common, u32 sync_cause);
>> +#else
>> +static void ath9k_debug_sync_cause(struct ath_common *common, u32 sync_cause) {}
>
> Due to a missing 'inline' keyword, this causes a bunch of warnings if
> CONFIG_ATH9K_DEBUGFS is not enabled:
>
>     CC [M]  drivers/net/wireless/ath/main.o
>     CC [M]  drivers/net/wireless/ath/regd.o
>     CC [M]  drivers/net/wireless/ath/hw.o
>     CC [M]  drivers/net/wireless/ath/key.o
>     LD [M]  drivers/net/wireless/ath/ath.o
>     CC [M]  drivers/net/wireless/ath/ath9k/beacon.o
>   drivers/net/wireless/ath/ath9k/hw.h:962: warning: ‘ath9k_debug_sync_cause’
> defined but not used

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

^ permalink raw reply

* Re: [PATCH 2/2] ath9k: Gather and report IRQ sync_cause errors.
From: Gabor Juhos @ 2012-04-17 21:07 UTC (permalink / raw)
  To: greearb-my8/4N5VtI7c+919tysfdA
  Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1334250240-21038-2-git-send-email-greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>

> From: Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
> 
> Report all defined sync_cause errors in debugfs
> to aid with debugging.
> 
> Use a macro to print out the interrupts file contents
> to decrease code duplication.

Now, that the patch is in wireless-testing, I have noticed a minor problem with it.

> 
> Signed-off-by: Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
> ---


<...>

> diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
> index aa1680a..fba99ef 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.h
> +++ b/drivers/net/wireless/ath/ath9k/hw.h
> @@ -965,6 +965,12 @@ bool ath9k_hw_check_alive(struct ath_hw *ah);
>  
>  bool ath9k_hw_setpower(struct ath_hw *ah, enum ath9k_power_mode mode);
>  
> +#ifdef CONFIG_ATH9K_DEBUGFS
> +void ath9k_debug_sync_cause(struct ath_common *common, u32 sync_cause);
> +#else
> +static void ath9k_debug_sync_cause(struct ath_common *common, u32 sync_cause) {}

Due to a missing 'inline' keyword, this causes a bunch of warnings if
CONFIG_ATH9K_DEBUGFS is not enabled:

   CC [M]  drivers/net/wireless/ath/main.o
   CC [M]  drivers/net/wireless/ath/regd.o
   CC [M]  drivers/net/wireless/ath/hw.o
   CC [M]  drivers/net/wireless/ath/key.o
   LD [M]  drivers/net/wireless/ath/ath.o
   CC [M]  drivers/net/wireless/ath/ath9k/beacon.o
 drivers/net/wireless/ath/ath9k/hw.h:962: warning: ‘ath9k_debug_sync_cause’
defined but not used
   CC [M]  drivers/net/wireless/ath/ath9k/gpio.o
 drivers/net/wireless/ath/ath9k/hw.h:962: warning: ‘ath9k_debug_sync_cause’
defined but not used
   CC [M]  drivers/net/wireless/ath/ath9k/init.o
 drivers/net/wireless/ath/ath9k/hw.h:962: warning: ‘ath9k_debug_sync_cause’
defined but not used
   CC [M]  drivers/net/wireless/ath/ath9k/main.o
 drivers/net/wireless/ath/ath9k/hw.h:962: warning: ‘ath9k_debug_sync_cause’
defined but not used
   CC [M]  drivers/net/wireless/ath/ath9k/recv.o

-Gabor

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: tg3 'No PHY devices' loading issue
From: Josh Boyer @ 2012-04-17 21:07 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Matt Carlson, Michael Chan, netdev, linux-kernel
In-Reply-To: <20120417205955.GC3932@decadent.org.uk>

On Tue, Apr 17, 2012 at 09:59:55PM +0100, Ben Hutchings wrote:
> On Tue, Apr 17, 2012 at 03:40:30PM -0400, Josh Boyer wrote:
> > On Tue, Apr 17, 2012 at 08:10:42PM +0100, Ben Hutchings wrote:
> > > On Tue, Apr 17, 2012 at 11:50:07AM -0700, Matt Carlson wrote:
> > > > On Tue, Apr 17, 2012 at 10:18:57AM -0400, Josh Boyer wrote:
> > > > > Hi Matt and Michael,
> > > > > 
> > > > > I'm seeing an odd issue with the tg3 driver on one of my development
> > > > > machines.  I've tried kernels 3.2.10, 3.3.0, 3.3.1, 3.3.2 and 3.4-rc3
> > > > > and they all seem to exhibit this issue now.  When the machine boots
> > > > > and the tg3 driver is loaded, it fails to find a PHY and then reports
> > > > > 'Problem fetching invariants of chip'.  If I do a rmmod/modprobe of
> > > > > tg3 after login, the probe seems to work fine and ethernet works as
> > > > > expected.  You can see this in the dmesg below:
> > > [...]
> > > > The 57788 uses the broadcom phylib module.  For some reason, it isn't
> > > > available the first module load attempt.  A while ago, code was added to
> > > > phylib to request modules from userspace if the particular phy support
> > > > code wasn't already loaded.  It looks like this mechanism isn't working
> > > > too well the first time through.
> > > [...]
> > > 
> > > At a guess, tg3 is in the initramfs but broadcom isn't.
> > 
> > Good guess.  At least on one of the initramfs' I checked.  I'll verify
> > the others, but it seems likely they match.  As I just said in my other
> > reply, I would have expected tg3 to have a module dep on it to get it
> > included in the initramfs built, but it seems that isn't the case.
>  
> tg3 doesn't use any symbols from broadcom, which is the only way
> depmod can find dependencies.
> 
> In initramfs-tools (used in Debian and its derivatives) we just copy
> all PHY drivers into the initramfs by default.  dracut should probably
> do the same, at least when phylib is in the initramfs (or built into
> the kernel).  (tg3 does use symbols from phylib.)

Heh.  Yep, that's basically what I just concluded as well in my other
reply.  I'll work with Harald and get that included or something similar
to it.

josh

^ permalink raw reply

* Re: tg3 'No PHY devices' loading issue
From: Josh Boyer @ 2012-04-17 21:05 UTC (permalink / raw)
  To: Matt Carlson; +Cc: Michael Chan, netdev, linux-kernel, Ben Hutchings, harald
In-Reply-To: <20120417193908.GG19507@zod.bos.redhat.com>

On Tue, Apr 17, 2012 at 03:39:08PM -0400, Josh Boyer wrote:
> On Tue, Apr 17, 2012 at 11:50:07AM -0700, Matt Carlson wrote:
> > On Tue, Apr 17, 2012 at 10:18:57AM -0400, Josh Boyer wrote:
> > > It has worked on some of the older kernels without the need for the
> > > manual rmmod/modprobe step, so it seems to be somewhat timing related.
> > > I'm not sure if there is a module load ordering issue, but that doesn't
> > > seem to be the case.  I can't explain why a later modprobe would work
> > > just fine though.
> > > 
> > > Do you have any thoughts on how to go about debugging/fixing this?  I'd
> > > be happy to test and provide whatever information you need.
> > 
> > The 57788 uses the broadcom phylib module.  For some reason, it isn't
> > available the first module load attempt.  A while ago, code was added to
> > phylib to request modules from userspace if the particular phy support
> > code wasn't already loaded.  It looks like this mechanism isn't working
> > too well the first time through.
> > 
> > Can you reproduce the problem if you run 'rmmod broadcom' and then
> > reload tg3?
> 
> I'll give it a shot, but that sounds like a quite plausible scenario to
> me.  Oddly, even after the manual modprobe, the broadcom phy module
> doesn't show an increased use count.  There's also no dependency on
> 'broadcom' listed for tg3 by modinfo.

If I rmmod both tg3 and broadcom and then just load tg3 (either via
insmod or modprobe), it works fine.  It does so because whatever you did
to make it request the phylib module from userspace also worked because
the broadcom module is loaded as well.

So it seems my issue is basically because the initramfs isn't built with
the broadcom module included.  I'll have to figure out a way to teach
dracut what to do here, since it doesn't seem to be automatically
picking it up (Harald CC'd).

Thanks much for the very quick response from everyone.

josh

^ permalink raw reply

* Re: tg3 'No PHY devices' loading issue
From: Ben Hutchings @ 2012-04-17 20:59 UTC (permalink / raw)
  To: Josh Boyer; +Cc: Matt Carlson, Michael Chan, netdev, linux-kernel
In-Reply-To: <20120417194029.GH19507@zod.bos.redhat.com>

On Tue, Apr 17, 2012 at 03:40:30PM -0400, Josh Boyer wrote:
> On Tue, Apr 17, 2012 at 08:10:42PM +0100, Ben Hutchings wrote:
> > On Tue, Apr 17, 2012 at 11:50:07AM -0700, Matt Carlson wrote:
> > > On Tue, Apr 17, 2012 at 10:18:57AM -0400, Josh Boyer wrote:
> > > > Hi Matt and Michael,
> > > > 
> > > > I'm seeing an odd issue with the tg3 driver on one of my development
> > > > machines.  I've tried kernels 3.2.10, 3.3.0, 3.3.1, 3.3.2 and 3.4-rc3
> > > > and they all seem to exhibit this issue now.  When the machine boots
> > > > and the tg3 driver is loaded, it fails to find a PHY and then reports
> > > > 'Problem fetching invariants of chip'.  If I do a rmmod/modprobe of
> > > > tg3 after login, the probe seems to work fine and ethernet works as
> > > > expected.  You can see this in the dmesg below:
> > [...]
> > > The 57788 uses the broadcom phylib module.  For some reason, it isn't
> > > available the first module load attempt.  A while ago, code was added to
> > > phylib to request modules from userspace if the particular phy support
> > > code wasn't already loaded.  It looks like this mechanism isn't working
> > > too well the first time through.
> > [...]
> > 
> > At a guess, tg3 is in the initramfs but broadcom isn't.
> 
> Good guess.  At least on one of the initramfs' I checked.  I'll verify
> the others, but it seems likely they match.  As I just said in my other
> reply, I would have expected tg3 to have a module dep on it to get it
> included in the initramfs built, but it seems that isn't the case.
 
tg3 doesn't use any symbols from broadcom, which is the only way
depmod can find dependencies.

In initramfs-tools (used in Debian and its derivatives) we just copy
all PHY drivers into the initramfs by default.  dracut should probably
do the same, at least when phylib is in the initramfs (or built into
the kernel).  (tg3 does use symbols from phylib.)

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

^ permalink raw reply

* Re: [PATCH 2/2] IPVS: make failure of netns init more stable
From: Julian Anastasov @ 2012-04-17 20:57 UTC (permalink / raw)
  To: Hans Schillstrom
  Cc: horms@verge.net.au, wensong@linux-vs.org,
	lvs-devel@vger.kernel.org, netdev@vger.kernel.org,
	netfilter-devel@vger.kernel.org, hans@schillstrom.com
In-Reply-To: <201204171315.12686.hans.schillstrom@ericsson.com>


	Hello,

On Tue, 17 Apr 2012, Hans Schillstrom wrote:

> I wonder if we are chasing ghosts...
> 
> With proper fault handling I can't even see a case when it (net->ipvs) can be used.
> Can you see a case when it could happen?
> Still we can set it to NULL on error exit and cleanup as you suggested, that doesn't harm I think.
> 
> A. If you add a netns and it fails the entire ns will be rolled back, 
>    and no access to that ns can occur.
>    That ns does not exist

	Agreed

> B. If you insert ip_vs.ko when having one or more name spaces and 
>    __ip_vs_init() returns an error the module will be unloaded.
>    All ready loaded ns will not be affected.

	Yes, ip_vs_init fails.

> C. insmod of ex. ip_vs_ftp only affects loaded name spaces
>    and if the load of ip_vs_ftp fails it will be unloaded without affecting ip_vs(.ko)
>    (If ip_vs.ko is not loaded then it has to be loaded first case B...)
> 
> With a "compiled in" ip_vs case B doesn't exist.

	It is this case that can happen, we can only guess how
difficult is to get ENOMEM here. IIRC, we can generate only
ENOMEM error on IPVS core load.

	I assume Simon has such setup and changes code to
trigger load error. When I generate ENOMEM on IPVS core init
for such case I get ENOENT from register_ip_vs_app when
patch 1 and 2 for apps are applied, i.e. net->ipvs is NULL.
You can check it with NF_CONNTRACK=y, IP_VS=y and
IP_VS_FTP=m. You only need to trigger ENOMEM in __ip_vs_init.

> With proper fault handling i.e. all ways returning fault codes to the netns init,
> there is no need for checking for  "if (!net->ipvs)" or any other action.

	Probably but one check on load does not hurt much.

Regards

--
Julian Anastasov <ja@ssi.bg>

^ permalink raw reply

* Re: 3.4.0-rc2: skb_put() -> skb_over_panic
From: Eric Dumazet @ 2012-04-17 20:53 UTC (permalink / raw)
  To: David Miller; +Cc: a.beregalov, netdev, linux-kernel
In-Reply-To: <20120417.164748.432392687904063236.davem@davemloft.net>

On Tue, 2012-04-17 at 16:47 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 17 Apr 2012 22:45:06 +0200
> 
> > Could you check if commit a21d45726acacc963d8baddf74607d9b74e2b723
> > (tcp: avoid order-1 allocations on wifi and tx path)
> > was in your tree ?
> 
> I was about to say that I think this is the guilty commit too.
> 
> Good thing I held off the -stable submission of that change
> for a bit :-)

Fix should be easy I think, but yes you can hold stable submission of
course.

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 376b2cf..7ac6423 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1096,6 +1096,7 @@ static void __pskb_trim_head(struct sk_buff *skb, int len)
 	eat = min_t(int, len, skb_headlen(skb));
 	if (eat) {
 		__skb_pull(skb, eat);
+		skb->avail_size -= eat;
 		len -= eat;
 		if (!len)
 			return;

^ permalink raw reply related

* Re: 3.4.0-rc2: skb_put() -> skb_over_panic
From: David Miller @ 2012-04-17 20:47 UTC (permalink / raw)
  To: eric.dumazet; +Cc: a.beregalov, netdev, linux-kernel
In-Reply-To: <1334695506.2472.46.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 17 Apr 2012 22:45:06 +0200

> Could you check if commit a21d45726acacc963d8baddf74607d9b74e2b723
> (tcp: avoid order-1 allocations on wifi and tx path)
> was in your tree ?

I was about to say that I think this is the guilty commit too.

Good thing I held off the -stable submission of that change
for a bit :-)

^ permalink raw reply

* Re: 3.4.0-rc2: skb_put() -> skb_over_panic
From: Eric Dumazet @ 2012-04-17 20:45 UTC (permalink / raw)
  To: Alexander Beregalov; +Cc: netdev, Linux Kernel Mailing List
In-Reply-To: <CAA1sL1SJyY7fYJn7OKqo5Pa=b4-r9DW90-5WVGBf==tFjVEBpg@mail.gmail.com>

On Wed, 2012-04-18 at 00:15 +0400, Alexander Beregalov wrote:
> On 15 April 2012 19:16, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Sun, 2012-04-15 at 16:24 +0400, Alexander Beregalov wrote:
> >> Hi
> >>
> >> kernel 3.4.0-rc2-00333-g668ce0a
> >>
> >> calltrace is lost on netconsole, sorry.
> >>
> >> ethernet is realtek/r8169
> >>
> >> It happened already two times with rtorrent, perhaps I can reproduce
> >> it, what else can I provide to you?
> >>
> > full stack trace needed please.
> >
> 
> This time calltrace is different (I saw few last lines of calltrace on
> a display, but not enough) and netconsole transmitted complete
> message, but perhaps it is the same problem. At least 'end' is the
> same.
> 
> 
> skb_over_panic: text:ffffffff8136d919 len:1248 put:932
> head:ffff8800babcf800 data:ffff8800babcfd10 tail:0x9f0 end:0x6c0
> dev:<NULL>
> ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:127!
> invalid opcode: 0000 [#1] SMP
> CPU 3
> Modules linked in:
> 
> Pid: 1926, comm: rtorrent Not tainted 3.4.0-rc2-00333-g668ce0a #1
>             /D525MW
> RIP: 0010:[<ffffffff8132414a>]  [<ffffffff8132414a>] skb_put+0x7c/0x86
> RSP: 0018:ffff8800bed83d60  EFLAGS: 00010246
> RAX: 0000000000000098 RBX: ffff8800b0b244c0 RCX: 000000000000003d
> RDX: 000000000000000d RSI: 0000000000000046 RDI: ffffffff8162a0b0
> RBP: ffff8800bed83d80 R08: 0000000000000001 R09: 0000000000000000
> R10: ffff88002ed840c0 R11: 00000000007e28f6 R12: ffff8800b9cb9180
> R13: ffff8800b9cb96c0 R14: ffff8800b9cb91a8 R15: ffff8800b0b245a0
> FS:  00007fe639e75720(0000) GS:ffff8800bed80000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fe63571a000 CR3: 00000000b53fa000 CR4: 00000000000007e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process rtorrent (pid: 1926, threadinfo ffff8800bb0a0000, task ffff8800bb1dddc0)
> Stack:
>  00000000000009f0 00000000000006c0 ffffffff814d06e7 ffff8800b9cb9180
>  ffff8800bed83de0 ffffffff8136d919 ffff8800000003a4 0000013c81536640
>  ffff8800b0b245a0 000000cc00000006 ffff8800b0b244c0 ffff8800b0b244c0
> Call Trace:
>  <IRQ>
>  [<ffffffff8136d919>] tcp_retransmit_skb+0x29a/0x529
>  [<ffffffff8136eebd>] tcp_retransmit_timer+0x358/0x4e7
>  [<ffffffff8136f0e8>] tcp_write_timer+0x9c/0x17d
>  [<ffffffff81035bd0>] run_timer_softirq+0x1eb/0x2bf
>  [<ffffffff8136f04c>] ? tcp_retransmit_timer+0x4e7/0x4e7
>  [<ffffffff810184e9>] ? native_smp_send_reschedule+0x4f/0x51
>  [<ffffffff8102fb52>] __do_softirq+0xbf/0x17d
>  [<ffffffff810188ec>] ? lapic_next_event+0x18/0x1c
>  [<ffffffff813c05cc>] call_softirq+0x1c/0x30
>  [<ffffffff810033d6>] do_softirq+0x33/0x69
>  [<ffffffff8102fdc6>] irq_exit+0x44/0x9c
>  [<ffffffff81018c6f>] smp_apic_timer_interrupt+0x86/0x94
>  [<ffffffff813bfe47>] apic_timer_interrupt+0x67/0x70
>  <EOI>
>  [<ffffffff8131d87e>] ? sock_sendmsg+0xe6/0x106
>  [<ffffffff8135fb7b>] ? tcp_poll+0xaf/0x168
>  [<ffffffff810ff814>] ? ep_send_events_proc+0x67/0x116
>  [<ffffffff8131b8ef>] sock_poll+0x15/0x17
>  [<ffffffff810ff823>] ep_send_events_proc+0x76/0x116
>  [<ffffffff810ff7ad>] ? ep_read_events_proc+0x99/0x99
>  [<ffffffff810fff24>] ep_scan_ready_list.clone.6+0x8f/0x16f
>  [<ffffffff81100277>] ep_poll+0x25f/0x2e2
>  [<ffffffff8131e886>] ? sys_accept4+0x133/0x15f
>  [<ffffffff81100d8e>] sys_epoll_wait+0x90/0xae
>  [<ffffffff813bf2e6>] system_call_fastpath+0x1a/0x1f
> Code: 8b 57 60 48 89 44 24 10 8b 87 ac 00 00 00 48 89 44 24 08 31 c0
> 8b bf a8 00 00 00 48 89 3c 24 48 c7 c7 43 07 4d 81 e8 2c 27 09 00 <0f>
> 0b 89 c0 49 8d 04 00 c9 c3 55 48 89 e5 41 57 41 56 41 55 41
> RIP  [<ffffffff8132414a>] skb_put+0x7c/0x86
>  RSP <ffff8800bed83d60>
> ---[ end trace a721715cd86be064 ]---

Thanks a lot, I belive I know where the problem is.

Could you check if commit a21d45726acacc963d8baddf74607d9b74e2b723
(tcp: avoid order-1 allocations on wifi and tx path)
was in your tree ?

^ permalink raw reply

* Re: [PATCH] net: filter: Just In Time compiler for sparc
From: David Miller @ 2012-04-17 20:44 UTC (permalink / raw)
  To: sam; +Cc: netdev, sparclinux
In-Reply-To: <20120417195716.GA26750@merkur.ravnborg.org>

From: Sam Ravnborg <sam@ravnborg.org>
Date: Tue, 17 Apr 2012 21:57:16 +0200

> I hope you had some fun doing this work - it does not look simple!

Like most programming tasks, it was just like solving a sudoku
puzzle. :-)

>> +	select HAVE_BPF_JIT
> If we sorted this block of select then the chances
> for merge conflict would be smaller.
> But this is not this patch to do so.

I also think we should create an arch/sparc/Kbuild for sparc
too, just like x86 has.

> Nice helpers - they made it easier for me to follow the assembler.
> r_SKB is not used in assembler?

It is used, but indirectly.  When we call the super slow paths
we have to pass r_SKB in, but we first temporarily allocate a
register window for the function call.  As a side effect
r_SKB (%o0) becomes %i0 so we can't just use r_SKB in this case.

>> +#ifdef CONFIG_SPARC64
>> +#define SAVE_SZ		176
>> +#define SCRATCH_OFF	STACK_BIAS + 128
>> +#define BE_PTR(label)	be,pn %xcc, label
>> +#else
>> +#define SAVE_SZ		96
>> +#define SCRATCH_OFF	72
>> +#define BE_PTR(label)	be label
>> +#endif
> 
> Is it coincidentally that SAVE_SZ has same value as BASE_STACKFRAME?
> From my quick browse of the code I think this is two distinct things,
> but if not we should move the definition to the header file and use the same.

They are identical values but used in two different situations and
thus best to keep them different in case we want to adjust one in the
future.

BASE_STACKFRAME is used to compute the stack space to allocate for the
whole JIT program if it makes use of the scratch memory area.

SAVE_SZ is used for the register window allocate we make in the stubs
when calling the slow path SKB helper functions.

>> +bpf_error:
>> +	jmpl	r_saved_O7 + 8, %g0
>> +	 clr	%o0
> 
> I wondered about this - because this is the only reference to %03 aka r_saved_O7
> And then the + 8 also puzzeled me.
> 
> A small comment would be nice.

I'll add a comment, but not a small one :-)

>> +/* assembly code in arch/sparc/net/bpf_jit_asm.S */
>> +extern u32 bpf_jit_load_word[];
>> +extern u32 bpf_jit_load_half[];
>> +extern u32 bpf_jit_load_byte[];
>> +extern u32 bpf_jit_load_byte_msh[];
>> +extern u32 bpf_jit_load_word_positive_offset[];
>> +extern u32 bpf_jit_load_half_positive_offset[];
>> +extern u32 bpf_jit_load_byte_positive_offset[];
>> +extern u32 bpf_jit_load_byte_msh_positive_offset[];
>> +extern u32 bpf_jit_load_word_negative_offset[];
>> +extern u32 bpf_jit_load_half_negative_offset[];
>> +extern u32 bpf_jit_load_byte_negative_offset[];
>> +extern u32 bpf_jit_load_byte_msh_negative_offset[];
> 
> I know this is from assembler files - but I hate externs in .c files.
> sparse did not complain though.

I'll put these into bpf_jit.h

>> +#define CONDN		COND (0x0)
>> +#define CONDE		COND (0x1)
>> +#define CONDLE		COND (0x2)
>> +#define CONDL		COND (0x3)
>> +#define CONDLEU		COND (0x4)
>> +#define CONDCS		COND (0x5)
>> +#define CONDNEG		COND (0x6)
>> +#define CONDVC		COND (0x7)
>> +#define CONDA		COND (0x8)
>> +#define CONDNE		COND (0x9)
>> +#define CONDG		COND (0xa)
>> +#define CONDGE		COND (0xb)
>> +#define CONDGU		COND (0xc)
>> +#define CONDCC		COND (0xd)
>> +#define CONDPOS		COND (0xe)
>> +#define CONDVS		COND (0xf)
> 
> The added space between COND and (0x..) looks strange to me.

Sorry, too must binutils hacking lately, I'll fix this.

>> +} while (0)
>> +
>> +	/* Emit
>> +	 *
>> +	 * 	OP	r_A, r_X, r_A
>> +	 */
> My vim marks the mixed spaces/tabs before OP with red.

I've fixed up all of these cases, thanks.

>> +} while(0)
>           ^
> Missing space. Repeats in the following macros.

And I've fixed these too.

>> #define emit_read_y(REG)        *prog++ = RD_Y | RD(REG);
>> #define emit_write_y(REG)       *prog++ = WR_Y | IMMED | RS1(REG) | S13(0);
> Mixed spaces and tabs, (The above is pasted).

Fixed, and erroneous trailing semicolon removed.

>> +#ifdef CONFIG_SPARC32
>> +					emit_branch(BE, t_offset + 20);
>> +#else
>> +					emit_branch(BE, t_offset + 8);
>> +#endif
> 
> What are these magic 8 and 20?

I've added a huge comment explaining the branch offset calculations.

Actually this was the hardest area to understand in the x86 JIT :)

--------------------
net: filter: Fix some more small issues in sparc JIT.

Fix mixed space and tabs.

Put bpf_jit_load_*[] externs into bpf_jit.h

"while(0)" --> "while (0)"
"COND (X)" --> "COND(X)"
Document branch offset calculations, and bpf_error's return
sequence.

Document the reason we need to emit three nops between the
%y register write and the divide instruction.

Remove erroneous trailing semicolons from emit_read_y() and
emit_write_y().

Based upon feedback from Sam Ravnborg.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/sparc/net/bpf_jit.h      |   15 ++++++
 arch/sparc/net/bpf_jit_asm.S  |    6 +++
 arch/sparc/net/bpf_jit_comp.c |  107 ++++++++++++++++++++++++-----------------
 3 files changed, 84 insertions(+), 44 deletions(-)

diff --git a/arch/sparc/net/bpf_jit.h b/arch/sparc/net/bpf_jit.h
index 05175be..33d6b37 100644
--- a/arch/sparc/net/bpf_jit.h
+++ b/arch/sparc/net/bpf_jit.h
@@ -38,6 +38,21 @@
 #define r_TMP		G1
 #define r_TMP2		G2
 #define r_OFF		G3
+
+/* assembly code in arch/sparc/net/bpf_jit_asm.S */
+extern u32 bpf_jit_load_word[];
+extern u32 bpf_jit_load_half[];
+extern u32 bpf_jit_load_byte[];
+extern u32 bpf_jit_load_byte_msh[];
+extern u32 bpf_jit_load_word_positive_offset[];
+extern u32 bpf_jit_load_half_positive_offset[];
+extern u32 bpf_jit_load_byte_positive_offset[];
+extern u32 bpf_jit_load_byte_msh_positive_offset[];
+extern u32 bpf_jit_load_word_negative_offset[];
+extern u32 bpf_jit_load_half_negative_offset[];
+extern u32 bpf_jit_load_byte_negative_offset[];
+extern u32 bpf_jit_load_byte_msh_negative_offset[];
+
 #else
 #define r_SKB		%o0
 #define r_A		%o1
diff --git a/arch/sparc/net/bpf_jit_asm.S b/arch/sparc/net/bpf_jit_asm.S
index 46d8f59..9d016c7 100644
--- a/arch/sparc/net/bpf_jit_asm.S
+++ b/arch/sparc/net/bpf_jit_asm.S
@@ -195,5 +195,11 @@ bpf_jit_load_byte_msh_negative_offset:
 	 sll	r_OFF, 2, r_X
 
 bpf_error:
+	/* Make the JIT program return zero.  The JIT epilogue
+	 * stores away the original %o7 into r_saved_O7.  The
+	 * normal leaf function return is to use "retl" which
+	 * would evalute to "jmpl %o7 + 8, %g0" but we want to
+	 * use the saved value thus the sequence you see here.
+	 */
 	jmpl	r_saved_O7 + 8, %g0
 	 clr	%o0
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
index ebc8980..2314eeb 100644
--- a/arch/sparc/net/bpf_jit_comp.c
+++ b/arch/sparc/net/bpf_jit_comp.c
@@ -11,20 +11,6 @@
 
 int bpf_jit_enable __read_mostly;
 
-/* assembly code in arch/sparc/net/bpf_jit_asm.S */
-extern u32 bpf_jit_load_word[];
-extern u32 bpf_jit_load_half[];
-extern u32 bpf_jit_load_byte[];
-extern u32 bpf_jit_load_byte_msh[];
-extern u32 bpf_jit_load_word_positive_offset[];
-extern u32 bpf_jit_load_half_positive_offset[];
-extern u32 bpf_jit_load_byte_positive_offset[];
-extern u32 bpf_jit_load_byte_msh_positive_offset[];
-extern u32 bpf_jit_load_word_negative_offset[];
-extern u32 bpf_jit_load_half_negative_offset[];
-extern u32 bpf_jit_load_byte_negative_offset[];
-extern u32 bpf_jit_load_byte_msh_negative_offset[];
-
 static inline bool is_simm13(unsigned int value)
 {
 	return value + 0x1000 < 0x2000;
@@ -65,22 +51,22 @@ static void bpf_flush_icache(void *start_, void *end_)
 #define F2(X, Y)	(OP(X) | OP2(Y))
 #define F3(X, Y)	(OP(X) | OP3(Y))
 
-#define CONDN		COND (0x0)
-#define CONDE		COND (0x1)
-#define CONDLE		COND (0x2)
-#define CONDL		COND (0x3)
-#define CONDLEU		COND (0x4)
-#define CONDCS		COND (0x5)
-#define CONDNEG		COND (0x6)
-#define CONDVC		COND (0x7)
-#define CONDA		COND (0x8)
-#define CONDNE		COND (0x9)
-#define CONDG		COND (0xa)
-#define CONDGE		COND (0xb)
-#define CONDGU		COND (0xc)
-#define CONDCC		COND (0xd)
-#define CONDPOS		COND (0xe)
-#define CONDVS		COND (0xf)
+#define CONDN		COND(0x0)
+#define CONDE		COND(0x1)
+#define CONDLE		COND(0x2)
+#define CONDL		COND(0x3)
+#define CONDLEU		COND(0x4)
+#define CONDCS		COND(0x5)
+#define CONDNEG		COND(0x6)
+#define CONDVC		COND(0x7)
+#define CONDA		COND(0x8)
+#define CONDNE		COND(0x9)
+#define CONDG		COND(0xa)
+#define CONDGE		COND(0xb)
+#define CONDGU		COND(0xc)
+#define CONDCC		COND(0xd)
+#define CONDPOS		COND(0xe)
+#define CONDVS		COND(0xf)
 
 #define CONDGEU		CONDCC
 #define CONDLU		CONDCS
@@ -172,7 +158,7 @@ do {	/* sethi %hi(K), REG */					\
 
 	/* Emit
 	 *
-	 * 	OP	r_A, r_X, r_A
+	 *	OP	r_A, r_X, r_A
 	 */
 #define emit_alu_X(OPCODE)					\
 do {								\
@@ -195,7 +181,7 @@ do {								\
 	 * is zero.
 	 */
 #define emit_alu_K(OPCODE, K)					\
-do {			   					\
+do {								\
 	if (K) {						\
 		unsigned int _insn = OPCODE;			\
 		_insn |= RS1(r_A) | RD(r_A);			\
@@ -204,7 +190,7 @@ do {			   					\
 		} else {					\
 			emit_set_const(K, r_TMP);		\
 			*prog++ = _insn | RS2(r_TMP);		\
-		}		  	  			\
+		}						\
 	}							\
 } while (0)
 
@@ -222,37 +208,37 @@ do {									\
 do {	unsigned int _off = offsetof(STRUCT, FIELD);			\
 	BUILD_BUG_ON(FIELD_SIZEOF(STRUCT, FIELD) != sizeof(void *));	\
 	*prog++ = LDPTRI | RS1(BASE) | S13(_off) | RD(DEST);		\
-} while(0)
+} while (0)
 
 #define emit_load32(BASE, STRUCT, FIELD, DEST)				\
 do {	unsigned int _off = offsetof(STRUCT, FIELD);			\
 	BUILD_BUG_ON(FIELD_SIZEOF(STRUCT, FIELD) != sizeof(u32));	\
 	*prog++ = LD32I | RS1(BASE) | S13(_off) | RD(DEST);		\
-} while(0)
+} while (0)
 
 #define emit_load16(BASE, STRUCT, FIELD, DEST)				\
 do {	unsigned int _off = offsetof(STRUCT, FIELD);			\
 	BUILD_BUG_ON(FIELD_SIZEOF(STRUCT, FIELD) != sizeof(u16));	\
 	*prog++ = LD16I | RS1(BASE) | S13(_off) | RD(DEST);		\
-} while(0)
+} while (0)
 
 #define __emit_load8(BASE, STRUCT, FIELD, DEST)				\
 do {	unsigned int _off = offsetof(STRUCT, FIELD);			\
 	*prog++ = LD8I | RS1(BASE) | S13(_off) | RD(DEST);		\
-} while(0)
+} while (0)
 
 #define emit_load8(BASE, STRUCT, FIELD, DEST)				\
 do {	BUILD_BUG_ON(FIELD_SIZEOF(STRUCT, FIELD) != sizeof(u8));	\
 	__emit_load8(BASE, STRUCT, FIELD, DEST);			\
-} while(0)
+} while (0)
 
 #define emit_ldmem(OFF, DEST)					\
 do {	*prog++ = LD32I | RS1(FP) | S13(-(OFF)) | RD(DEST);	\
-} while(0)
+} while (0)
 
 #define emit_stmem(OFF, SRC)					\
 do {	*prog++ = LD32I | RS1(FP) | S13(-(OFF)) | RD(SRC);	\
-} while(0)
+} while (0)
 
 #define cpu_off		offsetof(struct thread_info, cpu)
 
@@ -292,16 +278,16 @@ do {	void *_here = image + addrs[i] - 8;		\
 #define emit_branch(BR_OPC, DEST)			\
 do {	unsigned int _here = addrs[i] - 8;		\
 	*prog++ = BR_OPC | WDISP22((DEST) - _here);	\
-} while(0)
+} while (0)
 
 #define emit_branch_off(BR_OPC, OFF)			\
 do {	*prog++ = BR_OPC | WDISP22(OFF);		\
-} while(0)
+} while (0)
 
 #define emit_jump(DEST)		emit_branch(BA, DEST)
 
-#define emit_read_y(REG) 	*prog++ = RD_Y | RD(REG);
-#define emit_write_y(REG) 	*prog++ = WR_Y | IMMED | RS1(REG) | S13(0);
+#define emit_read_y(REG)	*prog++ = RD_Y | RD(REG)
+#define emit_write_y(REG)	*prog++ = WR_Y | IMMED | RS1(REG) | S13(0)
 
 #define emit_cmp(R1, R2) \
 	*prog++ = (SUBCC | RS1(R1) | RS2(R2) | RD(G0))
@@ -333,6 +319,35 @@ do {	*prog++ = BR_OPC | WDISP22(OFF);		\
 #define emit_release_stack(SZ) \
 	*prog++ = (ADD | IMMED | RS1(SP) | S13(SZ) | RD(SP))
 
+/* A note about branch offset calculations.  The addrs[] array,
+ * indexed by BPF instruction, records the address after all the
+ * sparc instructions emitted for that BPF instruction.
+ *
+ * The most common case is to emit a branch at the end of such
+ * a code sequence.  So this would be two instructions, the
+ * branch and it's delay slot.
+ *
+ * Therefore by default the branch emitters calculate the branch
+ * offset field as:
+ *
+ *	destination - (addrs[i] - 8)
+ *
+ * This "addrs[i] - 8" is the address of the branch itself or
+ * what "." would be in assembler notation.  The "8" part is
+ * how we take into consideration the branch and it's delay
+ * slot mentioned above.
+ *
+ * Sometimes we need to emit a branch earlier in the code
+ * sequence.  And in these situations we adjust "destination"
+ * to accomodate this difference.  For example, if we needed
+ * to emit a branch (and it's delay slot) right before the
+ * final instruction emitted for a BPF opcode, we'd use
+ * "destination + 4" instead of just plain "destination" above.
+ *
+ * This is why you see all of these funny emit_branch() and
+ * emit_jump() calls with adjusted offsets.
+ */
+
 void bpf_jit_compile(struct sk_filter *fp)
 {
 	unsigned int cleanup_addr, proglen, oldproglen = 0;
@@ -493,6 +508,10 @@ void bpf_jit_compile(struct sk_filter *fp)
 				}
 				emit_write_y(G0);
 #ifdef CONFIG_SPARC32
+				/* The Sparc v8 architecture requires
+				 * three instructions between a %y
+				 * register write and the first use.
+				 */
 				emit_nop();
 				emit_nop();
 				emit_nop();
-- 
1.7.10

^ permalink raw reply related

* Re: [PATCH] net: filter: Just In Time compiler for sparc
From: David Miller @ 2012-04-17 20:15 UTC (permalink / raw)
  To: richm; +Cc: netdev, sparclinux
In-Reply-To: <4F8DBDAD.50507@oldelvet.org.uk>

From: Richard Mortimer <richm@oldelvet.org.uk>
Date: Tue, 17 Apr 2012 19:59:57 +0100

> A few points below. Mainly a couple of comments that do not match the
> code itself. I didn't spot any errors in the assembler generation
> itself.

Thanks Richard, I've committed (after testing) the following based
upon your feedback.

--------------------
net: filter: Fix some minor issues in sparc JIT.

Correct conventions comments.  %o4 and %o5 were swapped,
%g3 was not documented.

Use r_TMP instead of r_SKB_DATA + r_OFF where possible in
assembler stubs.

Correct discussion of %o4 and %o5 in one of bpf_jit_compile()'s
comments.

Based upon feedback from Richard Mortimer.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/sparc/net/bpf_jit.h      |    5 +++--
 arch/sparc/net/bpf_jit_asm.S  |    4 ++--
 arch/sparc/net/bpf_jit_comp.c |    2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/sparc/net/bpf_jit.h b/arch/sparc/net/bpf_jit.h
index 74f048b..05175be 100644
--- a/arch/sparc/net/bpf_jit.h
+++ b/arch/sparc/net/bpf_jit.h
@@ -4,13 +4,14 @@
 /* Conventions:
  *  %g1 : temporary
  *  %g2 : Secondary temporary used by SKB data helper stubs.
+ *  %g3 : packet offset passed into SKB data helper stubs.
  *  %o0 : pointer to skb (first argument given to JIT function)
  *  %o1 : BPF A accumulator
  *  %o2 : BPF X accumulator
  *  %o3 : Holds saved %o7 so we can call helper functions without needing
  *        to allocate a register window.
- *  %o4 : skb->data
- *  %o5 : skb->len - skb->data_len
+ *  %o4 : skb->len - skb->data_len
+ *  %o5 : skb->data
  */
 
 #ifndef __ASSEMBLER__
diff --git a/arch/sparc/net/bpf_jit_asm.S b/arch/sparc/net/bpf_jit_asm.S
index fdc6932..46d8f59 100644
--- a/arch/sparc/net/bpf_jit_asm.S
+++ b/arch/sparc/net/bpf_jit_asm.S
@@ -30,7 +30,7 @@ bpf_jit_load_word_positive_offset:
 	bne	load_word_unaligned
 	 nop
 	retl
-	 ld	[r_SKB_DATA + r_OFF], r_A
+	 ld	[r_TMP], r_A
 load_word_unaligned:
 	ldub	[r_TMP + 0x0], r_OFF
 	ldub	[r_TMP + 0x1], r_TMP2
@@ -59,7 +59,7 @@ bpf_jit_load_half_positive_offset:
 	bne	load_half_unaligned
 	 nop
 	retl
-	 lduh	[r_SKB_DATA + r_OFF], r_A
+	 lduh	[r_TMP], r_A
 load_half_unaligned:
 	ldub	[r_TMP + 0x0], r_OFF
 	ldub	[r_TMP + 0x1], r_TMP2
diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
index 86349ca..ebc8980 100644
--- a/arch/sparc/net/bpf_jit_comp.c
+++ b/arch/sparc/net/bpf_jit_comp.c
@@ -378,7 +378,7 @@ void bpf_jit_compile(struct sk_filter *fp)
 				emit_clear(r_X);
 
 			/* If this filter needs to access skb data,
-			 * load %o4 and %o4 with:
+			 * load %o4 and %o5 with:
 			 *  %o4 = skb->len - skb->data_len
 			 *  %o5 = skb->data
 			 * And also back up %o7 into r_saved_O7 so we can
-- 
1.7.10


^ permalink raw reply related

* Re: 3.4.0-rc2: skb_put() -> skb_over_panic
From: Alexander Beregalov @ 2012-04-17 20:15 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, Linux Kernel Mailing List
In-Reply-To: <1334502964.28012.1.camel@edumazet-glaptop>

On 15 April 2012 19:16, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2012-04-15 at 16:24 +0400, Alexander Beregalov wrote:
>> Hi
>>
>> kernel 3.4.0-rc2-00333-g668ce0a
>>
>> calltrace is lost on netconsole, sorry.
>>
>> ethernet is realtek/r8169
>>
>> It happened already two times with rtorrent, perhaps I can reproduce
>> it, what else can I provide to you?
>>
> full stack trace needed please.
>

This time calltrace is different (I saw few last lines of calltrace on
a display, but not enough) and netconsole transmitted complete
message, but perhaps it is the same problem. At least 'end' is the
same.


skb_over_panic: text:ffffffff8136d919 len:1248 put:932
head:ffff8800babcf800 data:ffff8800babcfd10 tail:0x9f0 end:0x6c0
dev:<NULL>
------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:127!
invalid opcode: 0000 [#1] SMP
CPU 3
Modules linked in:

Pid: 1926, comm: rtorrent Not tainted 3.4.0-rc2-00333-g668ce0a #1
            /D525MW
RIP: 0010:[<ffffffff8132414a>]  [<ffffffff8132414a>] skb_put+0x7c/0x86
RSP: 0018:ffff8800bed83d60  EFLAGS: 00010246
RAX: 0000000000000098 RBX: ffff8800b0b244c0 RCX: 000000000000003d
RDX: 000000000000000d RSI: 0000000000000046 RDI: ffffffff8162a0b0
RBP: ffff8800bed83d80 R08: 0000000000000001 R09: 0000000000000000
R10: ffff88002ed840c0 R11: 00000000007e28f6 R12: ffff8800b9cb9180
R13: ffff8800b9cb96c0 R14: ffff8800b9cb91a8 R15: ffff8800b0b245a0
FS:  00007fe639e75720(0000) GS:ffff8800bed80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fe63571a000 CR3: 00000000b53fa000 CR4: 00000000000007e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process rtorrent (pid: 1926, threadinfo ffff8800bb0a0000, task ffff8800bb1dddc0)
Stack:
 00000000000009f0 00000000000006c0 ffffffff814d06e7 ffff8800b9cb9180
 ffff8800bed83de0 ffffffff8136d919 ffff8800000003a4 0000013c81536640
 ffff8800b0b245a0 000000cc00000006 ffff8800b0b244c0 ffff8800b0b244c0
Call Trace:
 <IRQ>
 [<ffffffff8136d919>] tcp_retransmit_skb+0x29a/0x529
 [<ffffffff8136eebd>] tcp_retransmit_timer+0x358/0x4e7
 [<ffffffff8136f0e8>] tcp_write_timer+0x9c/0x17d
 [<ffffffff81035bd0>] run_timer_softirq+0x1eb/0x2bf
 [<ffffffff8136f04c>] ? tcp_retransmit_timer+0x4e7/0x4e7
 [<ffffffff810184e9>] ? native_smp_send_reschedule+0x4f/0x51
 [<ffffffff8102fb52>] __do_softirq+0xbf/0x17d
 [<ffffffff810188ec>] ? lapic_next_event+0x18/0x1c
 [<ffffffff813c05cc>] call_softirq+0x1c/0x30
 [<ffffffff810033d6>] do_softirq+0x33/0x69
 [<ffffffff8102fdc6>] irq_exit+0x44/0x9c
 [<ffffffff81018c6f>] smp_apic_timer_interrupt+0x86/0x94
 [<ffffffff813bfe47>] apic_timer_interrupt+0x67/0x70
 <EOI>
 [<ffffffff8131d87e>] ? sock_sendmsg+0xe6/0x106
 [<ffffffff8135fb7b>] ? tcp_poll+0xaf/0x168
 [<ffffffff810ff814>] ? ep_send_events_proc+0x67/0x116
 [<ffffffff8131b8ef>] sock_poll+0x15/0x17
 [<ffffffff810ff823>] ep_send_events_proc+0x76/0x116
 [<ffffffff810ff7ad>] ? ep_read_events_proc+0x99/0x99
 [<ffffffff810fff24>] ep_scan_ready_list.clone.6+0x8f/0x16f
 [<ffffffff81100277>] ep_poll+0x25f/0x2e2
 [<ffffffff8131e886>] ? sys_accept4+0x133/0x15f
 [<ffffffff81100d8e>] sys_epoll_wait+0x90/0xae
 [<ffffffff813bf2e6>] system_call_fastpath+0x1a/0x1f
Code: 8b 57 60 48 89 44 24 10 8b 87 ac 00 00 00 48 89 44 24 08 31 c0
8b bf a8 00 00 00 48 89 3c 24 48 c7 c7 43 07 4d 81 e8 2c 27 09 00 <0f>
0b 89 c0 49 8d 04 00 c9 c3 55 48 89 e5 41 57 41 56 41 55 41
RIP  [<ffffffff8132414a>] skb_put+0x7c/0x86
 RSP <ffff8800bed83d60>
---[ end trace a721715cd86be064 ]---

^ permalink raw reply

* Re: [PATCH] net: filter: Just In Time compiler for sparc
From: Sam Ravnborg @ 2012-04-17 19:57 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, sparclinux
In-Reply-To: <20120416.225823.1395194623649559124.davem@davemloft.net>

Hi David.

I am glad to see sparc32 covered as well as sparc64 - but you knew that.

A few nits below.

I did not find the time to walk through it so I understood
the functionality. But I guess netdev's will do so.

I hope you had some fun doing this work - it does not look simple!

	Sam

> diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
> index db4e821..9f9afd9 100644
> --- a/arch/sparc/Kconfig
> +++ b/arch/sparc/Kconfig
> @@ -30,6 +30,7 @@ config SPARC
>  	select USE_GENERIC_SMP_HELPERS if SMP
>  	select GENERIC_PCI_IOMAP
>  	select HAVE_NMI_WATCHDOG if SPARC64
> +	select HAVE_BPF_JIT
If we sorted this block of select then the chances
for merge conflict would be smaller.
But this is not this patch to do so.

> +#define r_TMP		G1
> +#define r_TMP2		G2
> +#define r_OFF		G3
> +#else
> +#define r_SKB		%o0
> +#define r_A		%o1
> +#define r_X		%o2
> +#define r_saved_O7	%o3
> +#define r_HEADLEN	%o4
> +#define r_SKB_DATA	%o5
> +#define r_TMP		%g1
> +#define r_TMP2		%g2
> +#define r_OFF		%g3
> +#endif
Nice helpers - they made it easier for me to follow the assembler.
r_SKB is not used in assembler?

> +++ b/arch/sparc/net/bpf_jit_asm.S
> @@ -0,0 +1,199 @@
> +#include <asm/ptrace.h>
> +
> +#include "bpf_jit.h"
> +
> +#ifdef CONFIG_SPARC64
> +#define SAVE_SZ		176
> +#define SCRATCH_OFF	STACK_BIAS + 128
> +#define BE_PTR(label)	be,pn %xcc, label
> +#else
> +#define SAVE_SZ		96
> +#define SCRATCH_OFF	72
> +#define BE_PTR(label)	be label
> +#endif

Is it coincidentally that SAVE_SZ has same value as BASE_STACKFRAME?
>From my quick browse of the code I think this is two distinct things,
but if not we should move the definition to the header file and use the same.


> +bpf_error:
> +	jmpl	r_saved_O7 + 8, %g0
> +	 clr	%o0

I wondered about this - because this is the only reference to %03 aka r_saved_O7
And then the + 8 also puzzeled me.

A small comment would be nice.


> diff --git a/arch/sparc/net/bpf_jit_comp.c b/arch/sparc/net/bpf_jit_comp.c
> new file mode 100644
> index 0000000..86349ca
> --- /dev/null
> +++ b/arch/sparc/net/bpf_jit_comp.c
> @@ -0,0 +1,785 @@
> +#include <linux/moduleloader.h>
> +#include <linux/workqueue.h>
> +#include <linux/netdevice.h>
> +#include <linux/filter.h>
> +#include <linux/cache.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/ptrace.h>
> +
> +#include "bpf_jit.h"
> +
> +int bpf_jit_enable __read_mostly;
> +
> +/* assembly code in arch/sparc/net/bpf_jit_asm.S */
> +extern u32 bpf_jit_load_word[];
> +extern u32 bpf_jit_load_half[];
> +extern u32 bpf_jit_load_byte[];
> +extern u32 bpf_jit_load_byte_msh[];
> +extern u32 bpf_jit_load_word_positive_offset[];
> +extern u32 bpf_jit_load_half_positive_offset[];
> +extern u32 bpf_jit_load_byte_positive_offset[];
> +extern u32 bpf_jit_load_byte_msh_positive_offset[];
> +extern u32 bpf_jit_load_word_negative_offset[];
> +extern u32 bpf_jit_load_half_negative_offset[];
> +extern u32 bpf_jit_load_byte_negative_offset[];
> +extern u32 bpf_jit_load_byte_msh_negative_offset[];

I know this is from assembler files - but I hate externs in .c files.
sparse did not complain though.

> +#define CONDN		COND (0x0)
> +#define CONDE		COND (0x1)
> +#define CONDLE		COND (0x2)
> +#define CONDL		COND (0x3)
> +#define CONDLEU		COND (0x4)
> +#define CONDCS		COND (0x5)
> +#define CONDNEG		COND (0x6)
> +#define CONDVC		COND (0x7)
> +#define CONDA		COND (0x8)
> +#define CONDNE		COND (0x9)
> +#define CONDG		COND (0xa)
> +#define CONDGE		COND (0xb)
> +#define CONDGU		COND (0xc)
> +#define CONDCC		COND (0xd)
> +#define CONDPOS		COND (0xe)
> +#define CONDVS		COND (0xf)

The added space between COND and (0x..) looks strange to me.

> +} while (0)
> +
> +	/* Emit
> +	 *
> +	 * 	OP	r_A, r_X, r_A
> +	 */
My vim marks the mixed spaces/tabs before OP with red.

> +do {			   					\
Mixed spaces and tabs (vim is again red here)

> +	if (K) {						\
> +		unsigned int _insn = OPCODE;			\
> +		_insn |= RS1(r_A) | RD(r_A);			\
> +		if (is_simm13(K)) {				\
> +			*prog++ = _insn | IMMED | S13(K);	\
> +		} else {					\
> +			emit_set_const(K, r_TMP);		\
> +			*prog++ = _insn | RS2(r_TMP);		\
> +		}		  	  			\
Mixed spaces and tabs.

> +#define emit_loadptr(BASE, STRUCT, FIELD, DEST)				\
> +do {	unsigned int _off = offsetof(STRUCT, FIELD);			\
> +	BUILD_BUG_ON(FIELD_SIZEOF(STRUCT, FIELD) != sizeof(void *));	\
> +	*prog++ = LDPTRI | RS1(BASE) | S13(_off) | RD(DEST);		\
> +} while(0)
          ^
Missing space. Repeats in the following macros.

> #define emit_read_y(REG)        *prog++ = RD_Y | RD(REG);
> #define emit_write_y(REG)       *prog++ = WR_Y | IMMED | RS1(REG) | S13(0);
Mixed spaces and tabs, (The above is pasted).


> +		proglen = 0;
> +		prog = temp;
> +
> +		/* Prologue */
> +		if (seen_or_pass0) {
> +			if (seen_or_pass0 & SEEN_MEM) {
> +				unsigned int sz = BASE_STACKFRAME;
> +				sz += BPF_MEMWORDS * sizeof(u32);
> +				emit_alloc_stack(sz);
> +			}
> +
> +			/* Make sure we dont leek kernel memory. */
> +			if (seen_or_pass0 & SEEN_XREG)
> +				emit_clear(r_X);
> +
> +			/* If this filter needs to access skb data,
> +			 * load %o4 and %o4 with:
> +			 *  %o4 = skb->len - skb->data_len
> +			 *  %o5 = skb->data
We have nice menonics for o4 and o5 (r_HEADLEN, r_SKB_DATA),
but I assume you follow the registers easier.


> +			 * And also back up %o7 into r_saved_O7 so we can
> +			 * invoke the stubs using 'call'.
> +			 */
> +			if (seen_or_pass0 & SEEN_DATAREF) {
> +				emit_load32(r_SKB, struct sk_buff, len, r_HEADLEN);
> +				emit_load32(r_SKB, struct sk_buff, data_len, r_TMP);
> +				emit_sub(r_HEADLEN, r_TMP, r_HEADLEN);
> +				emit_loadptr(r_SKB, struct sk_buff, data, r_SKB_DATA);
> +			}
> +		}
> +		emit_reg_move(O7, r_saved_O7);
I want to say that I have seen this use of saved_O7 - but it did not help me.

> +#ifdef CONFIG_SPARC32
> +					emit_branch(BE, t_offset + 20);
> +#else
> +					emit_branch(BE, t_offset + 8);
> +#endif

What are these magic 8 and 20?


> +					emit_nop(); /* delay slot */
> +				} else {
> +					emit_branch_off(BNE, 16);
> +					emit_nop();
> +#ifdef CONFIG_SPARC32
> +					emit_jump(cleanup_addr + 20);
> +#else
> +					emit_jump(cleanup_addr + 8);
> +#endif
Here they are again.

> +					emit_clear(r_A);
> +				}
> +				emit_write_y(G0);
> +#ifdef CONFIG_SPARC32
> +				emit_nop();
> +				emit_nop();
> +				emit_nop();
> +#endif
Why these nops? Comment?


^ permalink raw reply

* Re: [PATCH] net: filter: Just In Time compiler for sparc
From: David Miller @ 2012-04-17 19:42 UTC (permalink / raw)
  To: jan.ceuleers; +Cc: netdev, sparclinux
In-Reply-To: <4F8DA21E.9020809@computer.org>

From: Jan Ceuleers <jan.ceuleers@computer.org>
Date: Tue, 17 Apr 2012 19:02:22 +0200

> There is inconsistent spacing in multiple places; a few examples shown
> above.

You've never written sparc assembler before, obviously.

The extra space after the tabs is intentional, and indicates
a branch delay slot instruction.

^ permalink raw reply

* Re: tg3 'No PHY devices' loading issue
From: Josh Boyer @ 2012-04-17 19:40 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Matt Carlson, Michael Chan, netdev, linux-kernel
In-Reply-To: <20120417191042.GB3932@decadent.org.uk>

On Tue, Apr 17, 2012 at 08:10:42PM +0100, Ben Hutchings wrote:
> On Tue, Apr 17, 2012 at 11:50:07AM -0700, Matt Carlson wrote:
> > On Tue, Apr 17, 2012 at 10:18:57AM -0400, Josh Boyer wrote:
> > > Hi Matt and Michael,
> > > 
> > > I'm seeing an odd issue with the tg3 driver on one of my development
> > > machines.  I've tried kernels 3.2.10, 3.3.0, 3.3.1, 3.3.2 and 3.4-rc3
> > > and they all seem to exhibit this issue now.  When the machine boots
> > > and the tg3 driver is loaded, it fails to find a PHY and then reports
> > > 'Problem fetching invariants of chip'.  If I do a rmmod/modprobe of
> > > tg3 after login, the probe seems to work fine and ethernet works as
> > > expected.  You can see this in the dmesg below:
> [...]
> > The 57788 uses the broadcom phylib module.  For some reason, it isn't
> > available the first module load attempt.  A while ago, code was added to
> > phylib to request modules from userspace if the particular phy support
> > code wasn't already loaded.  It looks like this mechanism isn't working
> > too well the first time through.
> [...]
> 
> At a guess, tg3 is in the initramfs but broadcom isn't.

Good guess.  At least on one of the initramfs' I checked.  I'll verify
the others, but it seems likely they match.  As I just said in my other
reply, I would have expected tg3 to have a module dep on it to get it
included in the initramfs built, but it seems that isn't the case.

josh

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox