Netdev List
 help / color / mirror / Atom feed
* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
From: Jason Gunthorpe @ 2010-02-27  6:01 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Or Gerlitz, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20100225084915.GA3171-YzwxZg+R7et1/kRsl7OVgNvLeJWuRmrY@public.gmane.org>

On Thu, Feb 25, 2010 at 09:49:15AM +0100, Jiri Pirko wrote:
> Thu, Feb 25, 2010 at 09:00:07AM CET, ogerlitz-smomgflXvOZWk0Htik3J/w@public.gmane.org wrote:
> >Jiri Pirko wrote:
> >> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> >> @@ -767,11 +767,8 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
> >> -static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen,
> >> -				     const u8 *broadcast)
> >> +static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast)
> >>  {
> >> -	if (addrlen != INFINIBAND_ALEN)
> >> -		return 0;
> >
> >This check was added by commit 5e47596b "IPoIB: Check multicast address format", may I ask what is the reason for removing it now?
> 
> Yes, at this very moment the check is not needless but it will be in a brief
> future. dev_mc_add will look very similar like dev_unicast_add. But ok. Here's
> patch adding the check in dev_mc_add right now to correct this state. Thanks Or.

Is this enough?

The problem this statement is trying to solve had to do with bonding
creating multicast addresess for ethernet rather than infiniband in
some cases. This happens because bonding makes a device that switches
from ethernet to infiniband during its lifetime. I'm not quite
sure what kind of life cycle those addresses go through, but if they
somehow stay on the mc_list then ipoib_mcast_addr_is_valid will still
need to have the check.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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: Gianfar driver failing on MPC8641D based board
From: Kumar Gopalpet-B05799 @ 2010-02-27  5:35 UTC (permalink / raw)
  To: avorontsov, Paul Gortmaker
  Cc: Martyn Welch, netdev, linux-kernel, linuxppc-dev list, davem
In-Reply-To: <20100226213825.GA32363@oksana.dev.rtsoft.ru>

 

>-----Original Message-----
>From: Anton Vorontsov [mailto:avorontsov@ru.mvista.com] 
>Sent: Saturday, February 27, 2010 3:08 AM
>To: Paul Gortmaker
>Cc: Martyn Welch; netdev@vger.kernel.org; 
>linux-kernel@vger.kernel.org; linuxppc-dev list; Kumar 
>Gopalpet-B05799; davem@davemloft.net
>Subject: Re: Gianfar driver failing on MPC8641D based board
>
>On Fri, Feb 26, 2010 at 11:27:42AM -0500, Paul Gortmaker wrote:
>> On 10-02-26 11:10 AM, Anton Vorontsov wrote:
>> > On Fri, Feb 26, 2010 at 03:34:07PM +0000, Martyn Welch wrote:
>> > [...]
>> >> Out of 10 boot attempts, 7 failed.
>> > 
>> > OK, I see why. With ip=on (dhcp boot) it's much harder to trigger 
>> > it. With static ip config can I see the same.
>> 
>> I'd kind of expected to see us stuck in gianfar on that 
>lock, but the 
>> SysRQ-T doesn't show us hung up anywhere in gianfar itself.
>> [This was on a base 2.6.33, with just a small sysrq fix patch]
>
>> [df841a30] [c0009fc4] __switch_to+0x8c/0xf8                  
>                   
>> [df841a50] [c0350160] schedule+0x354/0x92c                   
>                   
>> [df841ae0] [c0331394] rpc_wait_bit_killable+0x2c/0x54        
>                   
>> [df841af0] [c0350eb0] __wait_on_bit+0x9c/0x108               
>                   
>> [df841b10] [c0350fc0] out_of_line_wait_on_bit+0xa4/0xb4      
>                   
>> [df841b40] [c0331cf0] __rpc_execute+0x16c/0x398              
>                   
>> [df841b90] [c0329abc] rpc_run_task+0x48/0x9c                 
>                   
>> [df841ba0] [c0329c40] rpc_call_sync+0x54/0x88                
>                   
>> [df841bd0] [c015e780] nfs_proc_lookup+0x94/0xe8              
>                   
>> [df841c20] [c014eb60] nfs_lookup+0x12c/0x230                 
>                   
>> [df841d50] [c00b9680] do_lookup+0x118/0x288                  
>                   
>> [df841d80] [c00bb904] link_path_walk+0x194/0x1118            
>                   
>> [df841df0] [c00bcb08] path_walk+0x8c/0x168                   
>                   
>> [df841e20] [c00bcd6c] do_path_lookup+0x74/0x7c               
>                   
>> [df841e40] [c00be148] do_filp_open+0x5d4/0xba4               
>                   
>> [df841f10] [c00abe94] do_sys_open+0xac/0x190                 
>                   
>
>Yeah, I don't think this is gianfar-related. It must be 
>something else triggered by the fact that gianfar no longer 
>sends stuff.
>
>OK, I think I found what's happening in gianfar.
>
>Some background...
>
>start_xmit() prepares new skb for transmitting, generally it 
>does three things:
>
>1. sets up all BDs (marks them ready to send), except the first one.
>2. stores skb into tx_queue->tx_skbuff so that clean_tx_ring()
>   would cleanup it later.
>3. sets up the first BD, i.e. marks it ready.
>
>Here is what clean_tx_ring() does:
>
>1. reads skbs from tx_queue->tx_skbuff
>2. Checks if the *last* BD is ready. If it's still ready [to send]
>   then it it isn't transmitted, so clean_tx_ring() returns.
>   Otherwise it actually cleanups BDs. All is OK.
>
>Now, if there is just one BD, code flow:
>
>- start_xmit(): stores skb into tx_skbuff. Note that the first BD
>  (which is also the last one) isn't marked as ready, yet.
>- clean_tx_ring(): sees that skb is not null, *and* its lstatus
>  says that it is NOT ready (like if BD was sent), so it cleans
>  it up (bad!)
>- start_xmit(): marks BD as ready [to send], but it's too late.
>
>We can fix this simply by reordering lstatus/tx_skbuff writes.
>
>It works flawlessly on my p2020, please try it.

Anton,

Understood, and thanks for the explanation. Am I correct in saying that
this is
due to the out-of-order execution capability on powerpc ?

I have one more question, why don't we use use atomic_t for num_txbdfree
and
completely  do away with spin_locks in gfar_clean_tx_ring() and
gfar_start_xmit().
In an non-SMP, scenario I would feel there is absolutely no requirement
of spin_locks
and in case of SMP atomic operation would be much more safer on powerpc
rather than spin_locks.

What is your suggestion ?


--

Thanks
Sandeep

>
>Thanks!
>
>
>diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c 
>index 8bd3c9f..cccb409 100644
>--- a/drivers/net/gianfar.c
>+++ b/drivers/net/gianfar.c
>@@ -2021,7 +2021,6 @@ static int gfar_start_xmit(struct 
>sk_buff *skb, struct net_device *dev)
> 	}
> 
> 	/* setup the TxBD length and buffer pointer for the first BD */
>-	tx_queue->tx_skbuff[tx_queue->skb_curtx] = skb;
> 	txbdp_start->bufPtr = dma_map_single(&priv->ofdev->dev, 
>skb->data,
> 			skb_headlen(skb), DMA_TO_DEVICE);
> 
>@@ -2053,6 +2052,10 @@ static int gfar_start_xmit(struct 
>sk_buff *skb, struct net_device *dev)
> 
> 	txbdp_start->lstatus = lstatus;
> 
>+	eieio(); /* force lstatus write before tx_skbuff */
>+
>+	tx_queue->tx_skbuff[tx_queue->skb_curtx] = skb;
>+
> 	/* Update the current skb pointer to the next entry we will use
> 	 * (wrapping if necessary) */
> 	tx_queue->skb_curtx = (tx_queue->skb_curtx + 1) &
>
>

^ permalink raw reply

* Re: [net-next-2.6 PATCH v2] ethtool: Add direct access to ops->get_sset_count
From: Ben Hutchings @ 2010-02-27  4:37 UTC (permalink / raw)
  To: Jeff Kirsher; +Cc: davem, netdev, gospo, Jeff Garzik, Peter P Waskiewicz Jr
In-Reply-To: <20100227034927.1123.8292.stgit@localhost.localdomain>

On Fri, 2010-02-26 at 19:49 -0800, Jeff Kirsher wrote:
> From: Jeff Garzik <jgarzik@redhat.com>
> 
> This patch is an alternative approach for accessing string
> counts, vs. the drvinfo indirect approach.  This way the drvinfo
> space doesn't run out, and we don't break ABI later.
> 
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

Slander!

> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> 
>  include/linux/ethtool.h |   25 ++++++++++++-----
>  net/core/ethtool.c      |   71 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index cca1c3d..ff2a49c 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -239,10 +239,10 @@ struct ethtool_pauseparam {
>  
>  #define ETH_GSTRING_LEN		32
>  enum ethtool_stringset {
> -	ETH_SS_TEST		= 0,
> -	ETH_SS_STATS,
> -	ETH_SS_PRIV_FLAGS,
> -	ETH_SS_NTUPLE_FILTERS,
> +	ETH_SS_TEST		= (1 << 0),
> +	ETH_SS_STATS		= (1 << 1),
> +	ETH_SS_PRIV_FLAGS	= (1 << 2),
> +	ETH_SS_NTUPLE_FILTERS	= (1 << 3),
>  };
[...]

This breaks the established ABI for ETHTOOL_GSTRINGS.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


^ permalink raw reply

* [net-next-2.6 PATCH v2] ethtool: Correctly pull n-tuple string length for get_rx_ntuple
From: Jeff Kirsher @ 2010-02-27  3:50 UTC (permalink / raw)
  To: jeff, davem; +Cc: netdev, gospo, Peter P Waskiewicz Jr, Jeff Kirsher

From: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>

This patch fixes inconsistencies with the kernel header files, while
correctly gets the variable length string counts for the get_rx_ntuple
return value.  It does this by using the new GSSET_INFO ioctl.

Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 ethtool-copy.h |   22 ++++++++++++++++------
 ethtool.c      |   18 +++++++++++++++---
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 8681f5e..c8e56b1 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -239,10 +239,10 @@ struct ethtool_pauseparam {
 
 #define ETH_GSTRING_LEN		32
 enum ethtool_stringset {
-	ETH_SS_TEST		= 0,
-	ETH_SS_STATS,
-	ETH_SS_PRIV_FLAGS,
-	ETH_SS_NTUPLE_FILTERS,
+	ETH_SS_TEST		= (1 << 0),
+	ETH_SS_STATS		= (1 << 1),
+	ETH_SS_PRIV_FLAGS	= (1 << 2),
+	ETH_SS_NTUPLE_FILTERS	= (1 << 3),
 };
 
 /* for passing string sets for data tagging */
@@ -253,6 +253,17 @@ struct ethtool_gstrings {
 	__u8	data[0];
 };
 
+struct ethtool_sset_info {
+	__u32   cmd;		/* ETHTOOL_GSSET_INFO */
+	__u32   reserved;
+	__u64   sset_mask;	/* input: each bit selects an sset to query */
+				/* output: each bit a returned sset */
+	__u32   data[0];	/* ETH_SS_xxx count, in order, based on bits
+				   in sset_mask.  One bit implies one
+				   __u32, two bits implies two
+				   __u32's, etc. */
+};
+
 enum ethtool_test_flags {
 	ETH_TEST_FL_OFFLINE	= (1 << 0),	/* online / offline */
 	ETH_TEST_FL_FAILED	= (1 << 1),	/* test passed / failed */
@@ -389,8 +400,6 @@ struct ethtool_rx_ntuple_flow_spec {
 #define ETHTOOL_RXNTUPLE_ACTION_DROP -1
 };
 
-#define ETHTOOL_MAX_NTUPLE_LIST_ENTRY 1024
-#define ETHTOOL_MAX_NTUPLE_STRING_PER_ENTRY 14
 struct ethtool_rx_ntuple {
 	__u32					cmd;
 	struct ethtool_rx_ntuple_flow_spec	fs;
@@ -466,6 +475,7 @@ struct ethtool_flash {
 
 #define ETHTOOL_SRXNTUPLE	0x00000035 /* Add an n-tuple filter to device */
 #define ETHTOOL_GRXNTUPLE	0x00000036 /* Get n-tuple filters from device */
+#define ETHTOOL_GSSET_INFO	0x00000037 /* Get string set info */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/ethtool.c b/ethtool.c
index fc9e419..12e0430 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2657,11 +2657,23 @@ static int do_srxntuple(int fd, struct ifreq *ifr)
 
 static int do_grxntuple(int fd, struct ifreq *ifr)
 {
+	struct ethtool_sset_info *sset_info;
 	struct ethtool_gstrings *strings;
 	int sz_str, n_strings, err, i;
 
-	n_strings = ETHTOOL_MAX_NTUPLE_LIST_ENTRY *
-	            ETHTOOL_MAX_NTUPLE_STRING_PER_ENTRY;
+	sset_info = malloc(sizeof(struct ethtool_sset_info) + sizeof(u32));
+	sset_info->cmd = ETHTOOL_GSSET_INFO;
+	sset_info->sset_mask = ETH_SS_NTUPLE_FILTERS;
+	ifr->ifr_data = (caddr_t)sset_info;
+	err = send_ioctl(fd, ifr);
+
+	if (err < 0) {
+		perror("Cannot get driver strings info");
+		return 100;
+	}
+
+	n_strings = sset_info->data[0];
+	free(sset_info);
 	sz_str = n_strings * ETH_GSTRING_LEN;
 
 	strings = calloc(1, sz_str + sizeof(struct ethtool_gstrings));
@@ -2678,7 +2690,7 @@ static int do_grxntuple(int fd, struct ifreq *ifr)
 	if (err < 0) {
 		perror("Cannot get Rx n-tuple information");
 		free(strings);
-		return 100;
+		return 101;
 	}
 
 	n_strings = strings->len;


^ permalink raw reply related

* [net-next-2.6 PATCH v2] ethtool: Add direct access to ops->get_sset_count
From: Jeff Kirsher @ 2010-02-27  3:49 UTC (permalink / raw)
  To: davem; +Cc: netdev, gospo, Jeff Garzik, Peter P Waskiewicz Jr, Jeff Kirsher

From: Jeff Garzik <jgarzik@redhat.com>

This patch is an alternative approach for accessing string
counts, vs. the drvinfo indirect approach.  This way the drvinfo
space doesn't run out, and we don't break ABI later.

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 include/linux/ethtool.h |   25 ++++++++++++-----
 net/core/ethtool.c      |   71 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 7 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index cca1c3d..ff2a49c 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -239,10 +239,10 @@ struct ethtool_pauseparam {
 
 #define ETH_GSTRING_LEN		32
 enum ethtool_stringset {
-	ETH_SS_TEST		= 0,
-	ETH_SS_STATS,
-	ETH_SS_PRIV_FLAGS,
-	ETH_SS_NTUPLE_FILTERS,
+	ETH_SS_TEST		= (1 << 0),
+	ETH_SS_STATS		= (1 << 1),
+	ETH_SS_PRIV_FLAGS	= (1 << 2),
+	ETH_SS_NTUPLE_FILTERS	= (1 << 3),
 };
 
 /* for passing string sets for data tagging */
@@ -253,6 +253,17 @@ struct ethtool_gstrings {
 	__u8	data[0];
 };
 
+struct ethtool_sset_info {
+	__u32	cmd;		/* ETHTOOL_GSSET_INFO */
+	__u32	reserved;
+	__u64	sset_mask;	/* input: each bit selects an sset to query */
+				/* output: each bit a returned sset */
+	__u32	data[0];	/* ETH_SS_xxx count, in order, based on bits
+				   in sset_mask.  One bit implies one
+				   __u32, two bits implies two
+				   __u32's, etc. */
+};
+
 enum ethtool_test_flags {
 	ETH_TEST_FL_OFFLINE	= (1 << 0),	/* online / offline */
 	ETH_TEST_FL_FAILED	= (1 << 1),	/* test passed / failed */
@@ -606,9 +617,9 @@ struct ethtool_ops {
 #define	ETHTOOL_SRXCLSRLINS	0x00000032 /* Insert RX classification rule */
 #define	ETHTOOL_FLASHDEV	0x00000033 /* Flash firmware to device */
 #define	ETHTOOL_RESET		0x00000034 /* Reset hardware */
-
-#define ETHTOOL_SRXNTUPLE	0x00000035 /* Add an n-tuple filter to device */
-#define ETHTOOL_GRXNTUPLE	0x00000036 /* Get n-tuple filters from device */
+#define	ETHTOOL_SRXNTUPLE	0x00000035 /* Add an n-tuple filter to device */
+#define	ETHTOOL_GRXNTUPLE	0x00000036 /* Get n-tuple filters from device */
+#define	ETHTOOL_GSSET_INFO	0x00000037 /* Get string set info */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 31b1edd..10cfa08 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -212,6 +212,10 @@ static noinline int ethtool_get_drvinfo(struct net_device *dev, void __user *use
 	info.cmd = ETHTOOL_GDRVINFO;
 	ops->get_drvinfo(dev, &info);
 
+	/*
+	 * this method of obtaining string set info is deprecated;
+	 * consider using ETHTOOL_GSSET_INFO instead
+	 */
 	if (ops->get_sset_count) {
 		int rc;
 
@@ -238,6 +242,70 @@ static noinline int ethtool_get_drvinfo(struct net_device *dev, void __user *use
 /*
  * noinline attribute so that gcc doesnt use too much stack in dev_ethtool()
  */
+static noinline int ethtool_get_sset_info(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_sset_info info;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	u64 sset_mask;
+	int i, idx = 0, n_bits = 0, ret, rc;
+	u32 *info_buf = NULL;
+
+	if (!ops->get_sset_count)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&info, useraddr, sizeof(info)))
+		return -EFAULT;
+
+	/* store copy of mask, because we zero struct later on */
+	sset_mask = info.sset_mask;
+	if (!sset_mask)
+		return 0;
+
+	/* calculate size of return buffer */
+	for (i = 0; i < 64; i++)
+		if (sset_mask & (1ULL << i))
+			n_bits++;
+
+	memset(&info, 0, sizeof(info));
+	info.cmd = ETHTOOL_GSSET_INFO;
+
+	info_buf = kzalloc(n_bits * sizeof(u32), GFP_USER);
+	if (!info_buf)
+		return -ENOMEM;
+
+	/*
+	 * fill return buffer based on input bitmask and successful
+	 * get_sset_count return
+	 */
+	for (i = 0; i < 64; i++) {
+		if (!(sset_mask & (1ULL << i)))
+			continue;
+
+		rc = ops->get_sset_count(dev, (1ULL << i));
+		if (rc >= 0) {
+			info.sset_mask |= (1ULL << i);
+			info_buf[idx++] = rc;
+		}
+	}
+
+	ret = -EFAULT;
+	if (copy_to_user(useraddr, &info, sizeof(info)))
+		goto out;
+
+	useraddr += offsetof(struct ethtool_sset_info, data);
+	if (copy_to_user(useraddr, info_buf, idx * sizeof(u32)))
+		goto out;
+
+	ret = 0;
+
+out:
+	kfree(info_buf);
+	return ret;
+}
+
+/*
+ * noinline attribute so that gcc doesnt use too much stack in dev_ethtool()
+ */
 static noinline int ethtool_set_rxnfc(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_rxnfc cmd;
@@ -1469,6 +1537,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GRXNTUPLE:
 		rc = ethtool_get_rx_ntuple(dev, useraddr);
 		break;
+	case ETHTOOL_GSSET_INFO:
+		rc = ethtool_get_sset_info(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}


^ permalink raw reply related

* Re: [RFC PATCH] fix problems with NETIF_F_HIGHDMA in networking drivers
From: Robert Hancock @ 2010-02-27  3:08 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz; +Cc: David Miller, linux-kernel, netdev, linux-usb
In-Reply-To: <201002261625.24523.bzolnier@gmail.com>

On Fri, Feb 26, 2010 at 9:25 AM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
> On Friday 26 February 2010 03:46:45 pm Robert Hancock wrote:
>> On Fri, Feb 26, 2010 at 3:36 AM, David Miller <davem@davemloft.net> wrote:
>> > From: Robert Hancock <hancockrwd@gmail.com>
>> > Date: Mon, 22 Feb 2010 20:45:45 -0600
>> >
>> >> Many networking drivers have issues with the use of the NETIF_F_HIGHDMA flag.
>> >> This flag actually indicates whether or not the device/driver can handle
>> >> skbs located in high memory (as opposed to lowmem). However, many drivers
>> >> incorrectly treat this flag as indicating that 64-bit DMA is supported, which
>> >> has nothing to do with its actual function. It makes no sense to make setting
>> >> NETIF_F_HIGHDMA conditional on whether a 64-bit DMA mask has been set, as many
>> >> drivers do, since if highmem DMA is supported at all, it should work regardless
>> >> of whether 64-bit DMA is supported. Failing to set NETIF_F_HIGHDMA when it
>> >> should be can hurt performance on architectures which use highmem since it
>> >> results in needless data copying.
>> >>
>> >> This fixes up the networking drivers which currently use NETIF_F_HIGHDMA to
>> >> not do so conditionally on DMA mask settings.
>> >>
>> >> For the USB kaweth and usbnet drivers, this patch also uncomments and corrects
>> >> some code to set NETIF_F_HIGHDMA based on the USB host controller's DMA mask.
>> >> These drivers should be able to access highmem unless the host controller is
>> >> non-DMA-capable, which is indicated by the DMA mask being null.
>> >>
>> >> Signed-off-by: Robert Hancock <hancockrwd@gmail.com>
>> >
>> > Well, if the device isn't using 64-bit DMA addressing and the platform
>> > uses direct (no-iommu) mapping of physical to DMA addresses , won't
>> > your change break things?  The device will get a >4GB DMA address or
>> > the DMA mapping layer will signal an error.
>> >
>> > That's really part of the what the issue is I think.
>> >
>> > So, this trigger the check in check_addr() in
>> > arch/x86/kernel/pci-nommu.c when such packets try to get mapped by the
>> > driver, right?
>> >
>> > That will make the DMA mapping call fail, and the packet will be
>> > dropped permanently.  And hey, on top of it, many of these drivers you
>> > remove the setting from don't even check the mapping call return
>> > values for errors.
>> >
>> > So even bigger breakage.  One example is drivers/net/8139cp.c,
>> > it just does dma_map_single() and uses the result.
>> >
>> > It really depends upon that NETIF_F_HIGHDMA setting for correct
>> > operation.
>> >
>> > And even if something like swiotlb is available, now we're going
>> > to do bounce buffering which is largely equivalent to what
>> > a lack of NETIF_F_HIGHDMA will do.  Except that once NETIF_F_HIGHDMA
>> > copies the packet to lowmem it will only do that once, whereas if
>> > the packet goes to multiple devices swiotlb might copy the packet
>> > to a bounce buffer multiple times.
>> >
>> > We definitely can't apply your patch as-is.
>>
>> Hmm.. Yeah, there is a bit of a mess there. I'm thinking of the
>> particular example of i386 where you have 32-bit DMA devices with more
>> than 4GB of RAM. If you then allow the device to access highmem then
>> the DMA mapping API can get presented with addresses above 4GB and
>> AFAIK I don't think it can cope with that situation on that platform.
>>
>> Problem is that the NETIF_F_HIGHDMA check is generally too restrictive
>> in that situation, and it's really conflating two things into one (the
>> genuine can't-access-highmem part, and the "oh by the way, if highmem
>> can be >4GB then we can't access that") . If you have 3GB of RAM on
>> i386 with one of these drivers, you'll have packets being bounced
>> through lowmem without any real reason. I'll have a look into things a
>> bit further..
>
> Maybe it would be useful to start with splitting NETIF_F_HIGHDMA on two
> independent flags, i.e.:
>
>        #define NETIF_F_DMA_HIGH        (1 << 27)
>        #define NETIF_F_DMA_64BIT       (1 << 28)
>
> and keeping NETIF_F_HIGHDMA as
>
>        #define NETIF_F_HIGHDMA         (NETIF_F_DMA_HIGH | NET_F_DMA_64BIT)
>
> for now..?
>
> [ Next step would involve fixing illegal_highdma() check in net/core/dev.c
>  to distinguish between those new flags which in turn should allow sorting
>  out code in the device drivers on *per-driver* basis. ]

That seems like a reasonable approach to me. Only question is how to
implement the check for DMA_64BIT. Can we just check page_to_phys on
each of the pages in the skb to see if it's > 0xffffffff ? Are there
any architectures where it's more complicated than that?

Also, presumably these flags still wouldn't have any effect on
non-highmem architectures, same as NETIF_F_HIGHDMA now, since the best
we can do is copy the data to lowmem, which could still be >4GB in
this case. Therefore, the IOMMU (either HW or SWIOTLB, one of which
had better exist) just has to deal with it. (I suppose you could copy
to a buffer allocated with GFP_DMA32, but that likely wouldn't be a
win if you had a hardware IOMMU.)

^ permalink raw reply

* RE: [PATCH 4/7] cxgb4: Add packet queues and packet DMA code
From: Dimitrios Michailidis @ 2010-02-27  1:42 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev



> -----Original Message-----
> From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> Sent: Friday, February 26, 2010 5:31 PM
> To: Dimitrios Michailidis
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH 4/7] cxgb4: Add packet queues and packet DMA code
> 
> On Fri, 26 Feb 2010 16:52:10 -0800
> "Dimitrios Michailidis" <dm@chelsio.com> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: netdev-owner@vger.kernel.org
> > [mailto:netdev-owner@vger.kernel.org]
> > > On Behalf Of Stephen Hemminger
> > > Sent: Friday, February 26, 2010 4:16 PM
> > > To: Dimitrios Michailidis
> > > Cc: netdev@vger.kernel.org
> > > Subject: Re: [PATCH 4/7] cxgb4: Add packet queues and packet DMA
code
> > >
> > > On Fri, 26 Feb 2010 16:10:07 -0800
> > > "Dimitrios Michailidis" <dm@chelsio.com> wrote:
> > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> > > > > Sent: Friday, February 26, 2010 3:58 PM
> > > > > To: Dimitrios Michailidis
> > > > > Cc: netdev@vger.kernel.org
> > > > > Subject: Re: [PATCH 4/7] cxgb4: Add packet queues and packet
DMA
> > code
> > > > >
> > > > > On Fri, 26 Feb 2010 15:35:36 -0800
> > > > > Dimitris Michailidis <dm@chelsio.com> wrote:
> > > > >
> > > > > > +
> > > > > > +/**
> > > > > > + *	need_skb_unmap - does the platform need unmapping of
> > sk_buffs?
> > > > > > + *
> > > > > > + *	Returns true if the platfrom needs sk_buff unmapping.
> > The
> > > > compiler
> > > > > > + *	optimizes away unecessary code if this returns true.
> > > > > > + */
> > > > > > +static inline int need_skb_unmap(void)
> > > > > > +{
> > > > > > +	/*
> > > > > > +	 * This structure is used to tell if the platfrom needs
> > buffer
> > > > > > +	 * unmapping by checking if DECLARE_PCI_UNMAP_ADDR
> > defines
> > > > anything.
> > > > > > +	 */
> > > > > > +	struct dummy {
> > > > > > +		DECLARE_PCI_UNMAP_ADDR(addr);
> > > > > > +	};
> > > > > > +
> > > > > > +	return sizeof(struct dummy) != 0;
> > > > > > +}
> > > > > > +
> > > > >
> > > > > I would prefer one code path and let the compiler decide if
unmap
> > > > > should be nop; rather than this kind of trick code.
> > > >
> > > > I believe this function does not generate any code, the compiler
> > > > statically figures out the result and optimizes any conditionals
> > that
> > > > call it.  What option do you have in mind that would tell the
> > compiler
> > > > if unmap is nop?
> > >
> > > Why do you care, let compiler decide.
> >
> > I want the compiler to decide but I must be missing something.  How
is
> > the compiler going to decide?  The purpose of this code is to tell
the
> > compiler if unmapping is nop.  Does it have another way to tell?
> 
> I looked closer, it is all tied up with deferred skb unmapping and
that
> code looks inherited from cxgb3. Not sure why you need to hold to skb
for
> so long: some offload requirement? Anyway keep it they way it is..

Yes, offload packets can have their payload DMAd after their DMA
descriptor is freed and so they need to remain mapped longer.

^ permalink raw reply

* Re: [PATCH 4/7] cxgb4: Add packet queues and packet DMA code
From: Stephen Hemminger @ 2010-02-27  1:31 UTC (permalink / raw)
  To: Dimitrios Michailidis; +Cc: netdev
In-Reply-To: <8A71B368A89016469F72CD08050AD33401359B6A@maui.asicdesigners.com>

On Fri, 26 Feb 2010 16:52:10 -0800
"Dimitrios Michailidis" <dm@chelsio.com> wrote:

> 
> 
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org
> [mailto:netdev-owner@vger.kernel.org]
> > On Behalf Of Stephen Hemminger
> > Sent: Friday, February 26, 2010 4:16 PM
> > To: Dimitrios Michailidis
> > Cc: netdev@vger.kernel.org
> > Subject: Re: [PATCH 4/7] cxgb4: Add packet queues and packet DMA code
> > 
> > On Fri, 26 Feb 2010 16:10:07 -0800
> > "Dimitrios Michailidis" <dm@chelsio.com> wrote:
> > 
> > >
> > >
> > > > -----Original Message-----
> > > > From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> > > > Sent: Friday, February 26, 2010 3:58 PM
> > > > To: Dimitrios Michailidis
> > > > Cc: netdev@vger.kernel.org
> > > > Subject: Re: [PATCH 4/7] cxgb4: Add packet queues and packet DMA
> code
> > > >
> > > > On Fri, 26 Feb 2010 15:35:36 -0800
> > > > Dimitris Michailidis <dm@chelsio.com> wrote:
> > > >
> > > > > +
> > > > > +/**
> > > > > + *	need_skb_unmap - does the platform need unmapping of
> sk_buffs?
> > > > > + *
> > > > > + *	Returns true if the platfrom needs sk_buff unmapping.
> The
> > > compiler
> > > > > + *	optimizes away unecessary code if this returns true.
> > > > > + */
> > > > > +static inline int need_skb_unmap(void)
> > > > > +{
> > > > > +	/*
> > > > > +	 * This structure is used to tell if the platfrom needs
> buffer
> > > > > +	 * unmapping by checking if DECLARE_PCI_UNMAP_ADDR
> defines
> > > anything.
> > > > > +	 */
> > > > > +	struct dummy {
> > > > > +		DECLARE_PCI_UNMAP_ADDR(addr);
> > > > > +	};
> > > > > +
> > > > > +	return sizeof(struct dummy) != 0;
> > > > > +}
> > > > > +
> > > >
> > > > I would prefer one code path and let the compiler decide if unmap
> > > > should be nop; rather than this kind of trick code.
> > >
> > > I believe this function does not generate any code, the compiler
> > > statically figures out the result and optimizes any conditionals
> that
> > > call it.  What option do you have in mind that would tell the
> compiler
> > > if unmap is nop?
> > 
> > Why do you care, let compiler decide.
> 
> I want the compiler to decide but I must be missing something.  How is
> the compiler going to decide?  The purpose of this code is to tell the
> compiler if unmapping is nop.  Does it have another way to tell?

I looked closer, it is all tied up with deferred skb unmapping and that
code looks inherited from cxgb3. Not sure why you need to hold to skb for
so long: some offload requirement? Anyway keep it they way it is..

-- 

^ permalink raw reply

* [net-next PATCH v6 3/3] net: reserve ports for applications using fixed port numbers
From: Octavian Purdila @ 2010-02-27  1:25 UTC (permalink / raw)
  To: David Miller
  Cc: Octavian Purdila, Linux Kernel Network Developers,
	Linux Kernel Developers, WANG Cong, Neil Horman, Eric Dumazet,
	Eric W. Biederman
In-Reply-To: <1267233952-5856-1-git-send-email-opurdila@ixiacom.com>

This patch introduces /proc/sys/net/ipv4/ip_local_reserved_ports which
allows users to reserve ports for third-party applications.

The reserved ports will not be used by automatic port assignments
(e.g. when calling connect() or bind() with port number 0). Explicit
port allocation behavior is unchanged.

Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>
Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
---
 Documentation/networking/ip-sysctl.txt |   31 +++++++++++++++++++++++++++++++
 drivers/infiniband/core/cma.c          |    7 ++++++-
 include/net/ip.h                       |    6 ++++++
 net/ipv4/af_inet.c                     |    8 +++++++-
 net/ipv4/inet_connection_sock.c        |    6 ++++++
 net/ipv4/inet_hashtables.c             |    2 ++
 net/ipv4/sysctl_net_ipv4.c             |   17 +++++++++++++++++
 net/ipv4/udp.c                         |    3 ++-
 net/sctp/socket.c                      |    2 ++
 9 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 8b72c88..d0536b5 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -588,6 +588,37 @@ ip_local_port_range - 2 INTEGERS
 	(i.e. by default) range 1024-4999 is enough to issue up to
 	2000 connections per second to systems supporting timestamps.
 
+ip_local_reserved_ports - list of comma separated ranges
+	Specify the ports which are reserved for known third-party
+	applications. These ports will not be used by automatic port
+	assignments (e.g. when calling connect() or bind() with port
+	number 0). Explicit port allocation behavior is unchanged.
+
+	The format used for both input and output is a comma separated
+	list of ranges (e.g. "1,2-4,10-10" for ports 1, 2, 3, 4 and
+	10). Writing to the file will clear all previously reserved
+	ports and update the current list with the one given in the
+	input.
+
+	Note that ip_local_port_range and ip_local_reserved_ports
+	settings are independent and both are considered by the kernel
+	when determining which ports are available for automatic port
+	assignments.
+
+	You can reserve ports which are not in the current
+	ip_local_port_range, e.g.:
+
+	$ cat /proc/sys/net/ipv4/ip_local_port_range
+	32000	61000
+	$ cat /proc/sys/net/ipv4/ip_local_reserved_ports
+	8080,9148
+
+	although this is redundant. However such a setting is useful
+	if later the port range is changed to a value that will
+	include the reserved ports.
+
+	Default: Empty
+
 ip_nonlocal_bind - BOOLEAN
 	If set, allows processes to bind() to non-local IP addresses,
 	which can be quite useful - but may break some applications.
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 875e34e..06c9fa5 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1979,6 +1979,8 @@ retry:
 	/* FIXME: add proper port randomization per like inet_csk_get_port */
 	do {
 		ret = idr_get_new_above(ps, bind_list, next_port, &port);
+		if (inet_is_reserved_local_port(port))
+			ret = -EAGAIN;
 	} while ((ret == -EAGAIN) && idr_pre_get(ps, GFP_KERNEL));
 
 	if (ret)
@@ -2995,10 +2997,13 @@ static int __init cma_init(void)
 {
 	int ret, low, high, remaining;
 
-	get_random_bytes(&next_port, sizeof next_port);
 	inet_get_local_port_range(&low, &high);
+again:
+	get_random_bytes(&next_port, sizeof next_port);
 	remaining = (high - low) + 1;
 	next_port = ((unsigned int) next_port % remaining) + low;
+	if (inet_is_reserved_local_port(next_port))
+		goto again;
 
 	cma_wq = create_singlethread_workqueue("rdma_cm");
 	if (!cma_wq)
diff --git a/include/net/ip.h b/include/net/ip.h
index 503994a..3da9004 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -184,6 +184,12 @@ extern struct local_ports {
 } sysctl_local_ports;
 extern void inet_get_local_port_range(int *low, int *high);
 
+extern unsigned long *sysctl_local_reserved_ports;
+static inline int inet_is_reserved_local_port(int port)
+{
+	return test_bit(port, sysctl_local_reserved_ports);
+}
+
 extern int sysctl_ip_default_ttl;
 extern int sysctl_ip_nonlocal_bind;
 
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 33b7dff..e283fbe 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1546,9 +1546,13 @@ static int __init inet_init(void)
 
 	BUILD_BUG_ON(sizeof(struct inet_skb_parm) > sizeof(dummy_skb->cb));
 
+	sysctl_local_reserved_ports = kzalloc(65536 / 8, GFP_KERNEL);
+	if (!sysctl_local_reserved_ports)
+		goto out;
+
 	rc = proto_register(&tcp_prot, 1);
 	if (rc)
-		goto out;
+		goto out_free_reserved_ports;
 
 	rc = proto_register(&udp_prot, 1);
 	if (rc)
@@ -1647,6 +1651,8 @@ out_unregister_udp_proto:
 	proto_unregister(&udp_prot);
 out_unregister_tcp_proto:
 	proto_unregister(&tcp_prot);
+out_free_reserved_ports:
+	kfree(sysctl_local_reserved_ports);
 	goto out;
 }
 
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 8da6429..1acb462 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -37,6 +37,9 @@ struct local_ports sysctl_local_ports __read_mostly = {
 	.range = { 32768, 61000 },
 };
 
+unsigned long *sysctl_local_reserved_ports;
+EXPORT_SYMBOL(sysctl_local_reserved_ports);
+
 void inet_get_local_port_range(int *low, int *high)
 {
 	unsigned seq;
@@ -108,6 +111,8 @@ again:
 
 		smallest_size = -1;
 		do {
+			if (inet_is_reserved_local_port(rover))
+				goto next_nolock;
 			head = &hashinfo->bhash[inet_bhashfn(net, rover,
 					hashinfo->bhash_size)];
 			spin_lock(&head->lock);
@@ -130,6 +135,7 @@ again:
 			break;
 		next:
 			spin_unlock(&head->lock);
+		next_nolock:
 			if (++rover > high)
 				rover = low;
 		} while (--remaining > 0);
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 2b79377..d3e160a 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -456,6 +456,8 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
 		local_bh_disable();
 		for (i = 1; i <= remaining; i++) {
 			port = low + (i + offset) % remaining;
+			if (inet_is_reserved_local_port(port))
+				continue;
 			head = &hinfo->bhash[inet_bhashfn(net, port,
 					hinfo->bhash_size)];
 			spin_lock(&head->lock);
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index c1bc074..82d3c47 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -298,6 +298,13 @@ static struct ctl_table ipv4_table[] = {
 		.mode		= 0644,
 		.proc_handler	= ipv4_local_port_range,
 	},
+	{
+		.procname	= "ip_local_reserved_ports",
+		.data		= NULL, /* initialized in sysctl_ipv4_init */
+		.maxlen		= 65536,
+		.mode		= 0644,
+		.proc_handler	= proc_do_large_bitmap,
+	},
 #ifdef CONFIG_IP_MULTICAST
 	{
 		.procname	= "igmp_max_memberships",
@@ -735,6 +742,16 @@ static __net_initdata struct pernet_operations ipv4_sysctl_ops = {
 static __init int sysctl_ipv4_init(void)
 {
 	struct ctl_table_header *hdr;
+	struct ctl_table *i;
+
+	for (i = ipv4_table; i->procname; i++) {
+		if (strcmp(i->procname, "ip_local_reserved_ports") == 0) {
+			i->data = sysctl_local_reserved_ports;
+			break;
+		}
+	}
+	if (!i->procname)
+		return -EINVAL;
 
 	hdr = register_sysctl_paths(net_ipv4_ctl_path, ipv4_table);
 	if (hdr == NULL)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 608a544..bfd0a6a 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -232,7 +232,8 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum,
 			 */
 			do {
 				if (low <= snum && snum <= high &&
-				    !test_bit(snum >> udptable->log, bitmap))
+				    !test_bit(snum >> udptable->log, bitmap) &&
+				    !inet_is_reserved_local_port(snum))
 					goto found;
 				snum += rand;
 			} while (snum != first);
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index f6d1e59..1f839d0 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5432,6 +5432,8 @@ static long sctp_get_port_local(struct sock *sk, union sctp_addr *addr)
 			rover++;
 			if ((rover < low) || (rover > high))
 				rover = low;
+			if (inet_is_reserved_local_port(rover))
+				continue;
 			index = sctp_phashfn(rover);
 			head = &sctp_port_hashtable[index];
 			sctp_spin_lock(&head->lock);
-- 
1.5.6.5

^ permalink raw reply related

* [net-next PATCH v6 2/3] sysctl: add proc_do_large_bitmap
From: Octavian Purdila @ 2010-02-27  1:25 UTC (permalink / raw)
  To: David Miller
  Cc: Octavian Purdila, Linux Kernel Network Developers,
	Linux Kernel Developers, WANG Cong, Eric W. Biederman
In-Reply-To: <1267233952-5856-1-git-send-email-opurdila@ixiacom.com>

The new function can be used to read/write large bitmaps via /proc. A
comma separated range format is used for compact output and input
(e.g. 1,3-4,10-10).

Writing into the file will first reset the bitmap then update it
based on the given input.

Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>
Cc: WANG Cong <amwang@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/sysctl.h |    2 +
 kernel/sysctl.c        |  140 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+), 0 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index f66014c..7bb5cb6 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -980,6 +980,8 @@ extern int proc_doulongvec_minmax(struct ctl_table *, int,
 				  void __user *, size_t *, loff_t *);
 extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int,
 				      void __user *, size_t *, loff_t *);
+extern int proc_do_large_bitmap(struct ctl_table *, int,
+				void __user *, size_t *, loff_t *);
 
 /*
  * Register a set of sysctl names by calling register_sysctl_table
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 0873846..5d599cd 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2071,6 +2071,23 @@ static bool isanyof(char c, const char *v, unsigned len)
 	return true;
 }
 
+static int proc_skip_anyof(char __user **buf, size_t *size,
+			   const char *v, unsigned len)
+{
+	char c;
+
+	while (*size) {
+		if (get_user(c, *buf))
+			return -EFAULT;
+		if (!isanyof(c, v, len))
+			break;
+		(*size)--;
+		(*buf)++;
+	}
+
+	return 0;
+}
+
 #define TMPBUFLEN 22
 /**
  * proc_get_ulong - reads an ASCII formated integer from a user buffer
@@ -2662,6 +2679,129 @@ static int proc_do_cad_pid(struct ctl_table *table, int write,
 	return 0;
 }
 
+/**
+ * proc_do_large_bitmap - read/write from/to a large bitmap
+ * @table: the sysctl table
+ * @write: %TRUE if this is a write to the sysctl file
+ * @buffer: the user buffer
+ * @lenp: the size of the user buffer
+ * @ppos: file position
+ *
+ * The bitmap is stored at table->data and the bitmap length (in bits)
+ * in table->maxlen.
+ *
+ * We use a range comma separated format (e.g. 1,3-4,10-10) so that
+ * large bitmaps may be represented in a compact manner. Writing into
+ * the file will clear the bitmap then update it with the given input.
+ *
+ * Returns 0 on success.
+ */
+int proc_do_large_bitmap(struct ctl_table *table, int write,
+			 void __user *_buffer, size_t *lenp, loff_t *ppos)
+{
+	int err = 0;
+	bool first = 1;
+	size_t left = *lenp;
+	unsigned long bitmap_len = table->maxlen;
+	char __user *buffer = (char __user *) _buffer;
+	unsigned long *bitmap = (unsigned long *) table->data;
+	char tr_a[] = { '-', ',', '\n', 0 }, tr_b[] = { ',', '\n', 0 }, c;
+	char tr_end[] = { '\n', 0 };
+
+
+	if (!bitmap_len || !left || (*ppos && !write)) {
+		*lenp = 0;
+		return 0;
+	}
+
+	if (write) {
+		err = proc_skip_anyof(&buffer, &left, tr_end, sizeof(tr_end));
+		while (!err && left) {
+			unsigned long val_a, val_b;
+			bool neg;
+
+			err = proc_get_ulong(&buffer, &left, &val_a, &neg, tr_a,
+					     sizeof(tr_a), &c);
+			if (err)
+				break;
+			if (val_a >= bitmap_len || neg) {
+				err = -EINVAL;
+				break;
+			}
+
+			val_b = val_a;
+			if (left) {
+				buffer++;
+				left--;
+			}
+
+			if (c == '-') {
+				err = proc_get_ulong(&buffer, &left, &val_b,
+						     &neg, tr_b, sizeof(tr_b),
+						     &c);
+				if (err)
+					break;
+				if (val_b >= bitmap_len || neg ||
+				    val_a > val_b) {
+					err = -EINVAL;
+					break;
+				}
+				if (left) {
+					buffer++;
+					left--;
+				}
+			}
+
+			if (first)
+				bitmap_clear(bitmap, 0, bitmap_len);
+
+			while (val_a <= val_b)
+				set_bit(val_a++, bitmap);
+
+			first = 0;
+			err = proc_skip_anyof(&buffer, &left, tr_end,
+					      sizeof(tr_end));
+		}
+	} else {
+		unsigned long bit_a, bit_b = 0;
+
+		while (left) {
+			bit_a = find_next_bit(bitmap, bitmap_len, bit_b);
+			if (bit_a >= bitmap_len)
+				break;
+			bit_b = find_next_zero_bit(bitmap, bitmap_len,
+						   bit_a + 1) - 1;
+
+			err = proc_put_ulong(&buffer, &left, bit_a, 0, first,
+					     ',');
+			if (err)
+				break;
+			if (bit_a != bit_b) {
+				err = proc_put_char(&buffer, &left, '-');
+				if (err)
+					break;
+				err = proc_put_ulong(&buffer, &left, bit_b, 0,
+						     1, 0);
+				if (err)
+					break;
+			}
+
+			first = 0; bit_b++;
+		}
+		if (!err)
+			err = proc_put_char(&buffer, &left, '\n');
+	}
+
+	if (first) {
+		if (err)
+			return err;
+		bitmap_clear(bitmap, 0, bitmap_len);
+	}
+	*lenp -= left;
+	*ppos += *lenp;
+	return 0;
+}
+
 #else /* CONFIG_PROC_FS */
 
 int proc_dostring(struct ctl_table *table, int write,
-- 
1.5.6.5

^ permalink raw reply related

* [net-next PATCH v6 1/3] sysctl: refactor integer handling proc code
From: Octavian Purdila @ 2010-02-27  1:25 UTC (permalink / raw)
  To: David Miller
  Cc: Octavian Purdila, Linux Kernel Network Developers,
	Linux Kernel Developers, WANG Cong, Eric W. Biederman
In-Reply-To: <1267233952-5856-1-git-send-email-opurdila@ixiacom.com>

As we are about to add another integer handling proc function a little
bit of cleanup is in order: add a few helper functions to improve code
readability and decrease code duplication.

In the process a bug is also fixed: if the user specifies a number
with more then 20 digits it will be interpreted as two integers
(e.g. 10000...13 will be interpreted as 100.... and 13).

Behavior for EFAULT handling was changed as well. Previous to this
patch, when an EFAULT error occurred in the middle of a write
operation, although some of the elements were set, that was not
acknowledged to the user (by shorting the write and returning the
number of bytes accepted). EFAULT is now treated just like any other
errors by acknowledging the amount of bytes accepted.

Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>
Cc: WANG Cong <amwang@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
---
 kernel/sysctl.c |  364 ++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 210 insertions(+), 154 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 8a68b24..0873846 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2039,8 +2039,148 @@ int proc_dostring(struct ctl_table *table, int write,
 			       buffer, lenp, ppos);
 }
 
+static int proc_skip_wspace(char __user **buf, size_t *size)
+{
+	char c;
+
+	while (*size) {
+		if (get_user(c, *buf))
+			return -EFAULT;
+		if (!isspace(c))
+			break;
+		(*size)--;
+		(*buf)++;
+	}
+
+	return 0;
+}
+
+static bool isanyof(char c, const char *v, unsigned len)
+{
+	int i;
+
+	if (!len)
+		return false;
+
+	for (i = 0; i < len; i++)
+		if (c == v[i])
+			break;
+	if (i == len)
+		return false;
+
+	return true;
+}
+
+#define TMPBUFLEN 22
+/**
+ * proc_get_ulong - reads an ASCII formated integer from a user buffer
+ *
+ * @buf - user buffer
+ * @size - size of the user buffer
+ * @val - this is where the number will be stored
+ * @neg - set to %TRUE if number is negative
+ * @perm_tr - a vector which contains the allowed trailers
+ * @perm_tr_len - size of the perm_tr vector
+ * @tr - pointer to store the trailer character
+ *
+ * In case of success 0 is returned and buf and size are updated with
+ * the amount of bytes read. If tr is non NULL and a trailing
+ * character exist (size is non zero after returning from this
+ * function) tr is updated with the trailing character.
+ */
+static int proc_get_ulong(char __user **buf, size_t *size,
+			  unsigned long *val, bool *neg,
+			  const char *perm_tr, unsigned perm_tr_len, char *tr)
+{
+	int len;
+	char *p, tmp[TMPBUFLEN];
+
+	if (!*size)
+		return -EINVAL;
+
+	len = *size;
+	if (len > TMPBUFLEN-1)
+		len = TMPBUFLEN-1;
+
+	if (copy_from_user(tmp, *buf, len))
+		return -EFAULT;
+
+	tmp[len] = 0;
+	p = tmp;
+	if (*p == '-' && *size > 1) {
+		*neg = 1;
+		p++;
+	} else
+		*neg = 0;
+	if (!isdigit(*p))
+		return -EINVAL;
+
+	*val = simple_strtoul(p, &p, 0);
+
+	len = p - tmp;
+
+	/* We don't know if the next char is whitespace thus we may accept
+	 * invalid integers (e.g. 1234...a) or two integers instead of one
+	 * (e.g. 123...1). So lets not allow such large numbers. */
+	if (len == TMPBUFLEN - 1)
+		return -EINVAL;
+
+	if (len < *size && perm_tr_len && !isanyof(*p, perm_tr, perm_tr_len))
+		return -EINVAL;
+
+	if (tr && (len < *size))
+		*tr = *p;
+
+	*buf += len;
+	*size -= len;
+
+	return 0;
+}
+
+/**
+ * proc_put_ulong - coverts an integer to a decimal ASCII formated string
+ *
+ * @buf - the user buffer
+ * @size - the size of the user buffer
+ * @val - the integer to be converted
+ * @neg - sign of the number, %TRUE for negative
+ * @first - if %FALSE will insert a separator character before the number
+ * @separator - the separator character
+ *
+ * In case of success 0 is returned and buf and size are updated with
+ * the amount of bytes read.
+ */
+static int proc_put_ulong(char __user **buf, size_t *size, unsigned long val,
+			  bool neg, bool first, char separator)
+{
+	int len;
+	char tmp[TMPBUFLEN], *p = tmp;
+
+	if (!first)
+		*p++ = separator;
+	sprintf(p, "%s%lu", neg ? "-" : "", val);
+	len = strlen(tmp);
+	if (len > *size)
+		len = *size;
+	if (copy_to_user(*buf, tmp, len))
+		return -EFAULT;
+	*size -= len;
+	*buf += len;
+	return 0;
+}
+#undef TMPBUFLEN
+
+static int proc_put_char(char __user **buf, size_t *size, char c)
+{
+	if (*size) {
+		if (put_user(c, *buf))
+			return -EFAULT;
+		(*size)--, (*buf)++;
+	}
+	return 0;
+}
 
-static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp,
+static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
 				 int *valp,
 				 int write, void *data)
 {
@@ -2049,7 +2189,7 @@ static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp,
 	} else {
 		int val = *valp;
 		if (val < 0) {
-			*negp = -1;
+			*negp = 1;
 			*lvalp = (unsigned long)-val;
 		} else {
 			*negp = 0;
@@ -2059,20 +2199,18 @@ static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp,
 	return 0;
 }
 
+static const char proc_wspace_sep[] = { ' ', '\t', '\n', 0 };
+
 static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
-		  int write, void __user *buffer,
+		  int write, void __user *_buffer,
 		  size_t *lenp, loff_t *ppos,
-		  int (*conv)(int *negp, unsigned long *lvalp, int *valp,
+		  int (*conv)(bool *negp, unsigned long *lvalp, int *valp,
 			      int write, void *data),
 		  void *data)
 {
-#define TMPBUFLEN 21
-	int *i, vleft, first = 1, neg;
-	unsigned long lval;
-	size_t left, len;
-	
-	char buf[TMPBUFLEN], *p;
-	char __user *s = buffer;
+	int *i, vleft, first = 1, err = 0;
+	size_t left;
+	char __user *buffer = (char __user *) _buffer;
 	
 	if (!tbl_data || !table->maxlen || !*lenp ||
 	    (*ppos && !write)) {
@@ -2088,88 +2226,48 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
 		conv = do_proc_dointvec_conv;
 
 	for (; left && vleft--; i++, first=0) {
-		if (write) {
-			while (left) {
-				char c;
-				if (get_user(c, s))
-					return -EFAULT;
-				if (!isspace(c))
-					break;
-				left--;
-				s++;
-			}
-			if (!left)
-				break;
-			neg = 0;
-			len = left;
-			if (len > sizeof(buf) - 1)
-				len = sizeof(buf) - 1;
-			if (copy_from_user(buf, s, len))
-				return -EFAULT;
-			buf[len] = 0;
-			p = buf;
-			if (*p == '-' && left > 1) {
-				neg = 1;
-				p++;
-			}
-			if (*p < '0' || *p > '9')
-				break;
-
-			lval = simple_strtoul(p, &p, 0);
+		unsigned long lval;
+		bool neg;
 
-			len = p-buf;
-			if ((len < left) && *p && !isspace(*p))
+		if (write) {
+			err = proc_skip_wspace(&buffer, &left);
+			if (err)
+				return err;
+			err = proc_get_ulong(&buffer, &left, &lval, &neg,
+					     proc_wspace_sep,
+					     sizeof(proc_wspace_sep), NULL);
+			if (err)
 				break;
-			s += len;
-			left -= len;
-
-			if (conv(&neg, &lval, i, 1, data))
+			if (conv(&neg, &lval, i, 1, data)) {
+				err = -EINVAL;
 				break;
+			}
 		} else {
-			p = buf;
-			if (!first)
-				*p++ = '\t';
-	
-			if (conv(&neg, &lval, i, 0, data))
+			if (conv(&neg, &lval, i, 0, data)) {
+				err = -EINVAL;
 				break;
-
-			sprintf(p, "%s%lu", neg ? "-" : "", lval);
-			len = strlen(buf);
-			if (len > left)
-				len = left;
-			if(copy_to_user(s, buf, len))
-				return -EFAULT;
-			left -= len;
-			s += len;
-		}
-	}
-
-	if (!write && !first && left) {
-		if(put_user('\n', s))
-			return -EFAULT;
-		left--, s++;
-	}
-	if (write) {
-		while (left) {
-			char c;
-			if (get_user(c, s++))
-				return -EFAULT;
-			if (!isspace(c))
+			}
+			err = proc_put_ulong(&buffer, &left, lval, neg, first,
+					     '\t');
+			if (err)
 				break;
-			left--;
 		}
 	}
+
+	if (!write && !first && left && !err)
+		err = proc_put_char(&buffer, &left, '\n');
+	if (write && !err)
+		err = proc_skip_wspace(&buffer, &left);
 	if (write && first)
-		return -EINVAL;
+		return err ? : -EINVAL;
 	*lenp -= left;
 	*ppos += *lenp;
 	return 0;
-#undef TMPBUFLEN
 }
 
 static int do_proc_dointvec(struct ctl_table *table, int write,
 		  void __user *buffer, size_t *lenp, loff_t *ppos,
-		  int (*conv)(int *negp, unsigned long *lvalp, int *valp,
+		  int (*conv)(bool *negp, unsigned long *lvalp, int *valp,
 			      int write, void *data),
 		  void *data)
 {
@@ -2237,8 +2335,8 @@ struct do_proc_dointvec_minmax_conv_param {
 	int *max;
 };
 
-static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp, 
-					int *valp, 
+static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
+					int *valp,
 					int write, void *data)
 {
 	struct do_proc_dointvec_minmax_conv_param *param = data;
@@ -2251,7 +2349,7 @@ static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp,
 	} else {
 		int val = *valp;
 		if (val < 0) {
-			*negp = -1;
+			*negp = 1;
 			*lvalp = (unsigned long)-val;
 		} else {
 			*negp = 0;
@@ -2289,17 +2387,15 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 }
 
 static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write,
-				     void __user *buffer,
+				     void __user *_buffer,
 				     size_t *lenp, loff_t *ppos,
 				     unsigned long convmul,
 				     unsigned long convdiv)
 {
-#define TMPBUFLEN 21
-	unsigned long *i, *min, *max, val;
-	int vleft, first=1, neg;
-	size_t len, left;
-	char buf[TMPBUFLEN], *p;
-	char __user *s = buffer;
+	unsigned long *i, *min, *max;
+	int vleft, first = 1, err = 0;
+	size_t left;
+	char __user *buffer = (char __user *) _buffer;
 	
 	if (!data || !table->maxlen || !*lenp ||
 	    (*ppos && !write)) {
@@ -2314,82 +2410,42 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 	left = *lenp;
 	
 	for (; left && vleft--; i++, min++, max++, first=0) {
+		unsigned long val;
+
 		if (write) {
-			while (left) {
-				char c;
-				if (get_user(c, s))
-					return -EFAULT;
-				if (!isspace(c))
-					break;
-				left--;
-				s++;
-			}
-			if (!left)
-				break;
-			neg = 0;
-			len = left;
-			if (len > TMPBUFLEN-1)
-				len = TMPBUFLEN-1;
-			if (copy_from_user(buf, s, len))
-				return -EFAULT;
-			buf[len] = 0;
-			p = buf;
-			if (*p == '-' && left > 1) {
-				neg = 1;
-				p++;
-			}
-			if (*p < '0' || *p > '9')
-				break;
-			val = simple_strtoul(p, &p, 0) * convmul / convdiv ;
-			len = p-buf;
-			if ((len < left) && *p && !isspace(*p))
+			bool neg;
+
+			err = proc_skip_wspace(&buffer, &left);
+			if (err)
+				return err;
+			err = proc_get_ulong(&buffer, &left, &val, &neg,
+					     proc_wspace_sep,
+					     sizeof(proc_wspace_sep), NULL);
+			if (err)
 				break;
 			if (neg)
-				val = -val;
-			s += len;
-			left -= len;
-
-			if(neg)
 				continue;
 			if ((min && val < *min) || (max && val > *max))
 				continue;
 			*i = val;
 		} else {
-			p = buf;
-			if (!first)
-				*p++ = '\t';
-			sprintf(p, "%lu", convdiv * (*i) / convmul);
-			len = strlen(buf);
-			if (len > left)
-				len = left;
-			if(copy_to_user(s, buf, len))
-				return -EFAULT;
-			left -= len;
-			s += len;
-		}
-	}
-
-	if (!write && !first && left) {
-		if(put_user('\n', s))
-			return -EFAULT;
-		left--, s++;
-	}
-	if (write) {
-		while (left) {
-			char c;
-			if (get_user(c, s++))
-				return -EFAULT;
-			if (!isspace(c))
+			val = convdiv * (*i) / convmul;
+			err = proc_put_ulong(&buffer, &left, val, 0, first,
+					     '\t');
+			if (err)
 				break;
-			left--;
 		}
 	}
+
+	if (!write && !first && left && !err)
+		err = proc_put_char(&buffer, &left, '\n');
+	if (write && !err)
+		err = proc_skip_wspace(&buffer, &left);
 	if (write && first)
-		return -EINVAL;
+		return err ? : -EINVAL;
 	*lenp -= left;
 	*ppos += *lenp;
 	return 0;
-#undef TMPBUFLEN
 }
 
 static int do_proc_doulongvec_minmax(struct ctl_table *table, int write,
@@ -2450,7 +2506,7 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
 }
 
 
-static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp,
+static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp,
 					 int *valp,
 					 int write, void *data)
 {
@@ -2462,7 +2518,7 @@ static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp,
 		int val = *valp;
 		unsigned long lval;
 		if (val < 0) {
-			*negp = -1;
+			*negp = 1;
 			lval = (unsigned long)-val;
 		} else {
 			*negp = 0;
@@ -2473,7 +2529,7 @@ static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp,
 	return 0;
 }
 
-static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp,
+static int do_proc_dointvec_userhz_jiffies_conv(bool *negp, unsigned long *lvalp,
 						int *valp,
 						int write, void *data)
 {
@@ -2485,7 +2541,7 @@ static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp,
 		int val = *valp;
 		unsigned long lval;
 		if (val < 0) {
-			*negp = -1;
+			*negp = 1;
 			lval = (unsigned long)-val;
 		} else {
 			*negp = 0;
@@ -2496,7 +2552,7 @@ static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp,
 	return 0;
 }
 
-static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp,
+static int do_proc_dointvec_ms_jiffies_conv(bool *negp, unsigned long *lvalp,
 					    int *valp,
 					    int write, void *data)
 {
@@ -2506,7 +2562,7 @@ static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp,
 		int val = *valp;
 		unsigned long lval;
 		if (val < 0) {
-			*negp = -1;
+			*negp = 1;
 			lval = (unsigned long)-val;
 		} else {
 			*negp = 0;
-- 
1.5.6.5

^ permalink raw reply related

* [net-next PATCH v6 0/3] net: reserve ports for applications using fixed port numbers
From: Octavian Purdila @ 2010-02-27  1:25 UTC (permalink / raw)
  To: David Miller
  Cc: Octavian Purdila, Linux Kernel Network Developers,
	Linux Kernel Developers, Neil Horman, Eric Dumazet,
	Eric W. Biederman, WANG Cong

This patch introduces /proc/sys/net/ipv4/ip_local_reserved_ports which
allows users to reserve ports for third-party applications.

The reserved ports will not be used by automatic port assignments
(e.g. when calling connect() or bind() with port number 0). Explicit
port allocation behavior is unchanged.

Changes from the previous version:
- be more strict on accepted input (only comma separators, no spaces allowed)
- add to the docs a paragraph about ip_local_port_range and
  ip_local_reserved_ports relationship
- fix a few corner cases with parsing

There are still some miss behaviors with regard to proc parsing in odd
invalid cases (for "40000\0-40001" all is acknowledged but only 40000
is accepted) but they are not easy to fix without changing the current
"acknowledge how much we accepted" behavior.

Because of that and because the same issues are present in the
existing proc_dointvec code as well I don't think its worth holding
the actual feature (port reservation) after such petty error recovery
issues.

For the sake of discussion, I think Eric was right: the model we are
using is messy, we should only accept all input or none. If we can
(ABI implications) and you think its worth switching to this model I
can give it a try in a future patch.

Octavian Purdila (3):
  sysctl: refactor integer handling proc code
  sysctl: add proc_do_large_bitmap
  net: reserve ports for applications using fixed port numbers

 Documentation/networking/ip-sysctl.txt |   31 ++
 drivers/infiniband/core/cma.c          |    7 +-
 include/linux/sysctl.h                 |    2 +
 include/net/ip.h                       |    6 +
 kernel/sysctl.c                        |  504 ++++++++++++++++++++++----------
 net/ipv4/af_inet.c                     |    8 +-
 net/ipv4/inet_connection_sock.c        |    6 +
 net/ipv4/inet_hashtables.c             |    2 +
 net/ipv4/sysctl_net_ipv4.c             |   17 +
 net/ipv4/udp.c                         |    3 +-
 net/sctp/socket.c                      |    2 +
 11 files changed, 431 insertions(+), 157 deletions(-)

^ permalink raw reply

* RE: [PATCH 4/7] cxgb4: Add packet queues and packet DMA code
From: Dimitrios Michailidis @ 2010-02-27  0:52 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev



> -----Original Message-----
> From: netdev-owner@vger.kernel.org
[mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Stephen Hemminger
> Sent: Friday, February 26, 2010 4:16 PM
> To: Dimitrios Michailidis
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH 4/7] cxgb4: Add packet queues and packet DMA code
> 
> On Fri, 26 Feb 2010 16:10:07 -0800
> "Dimitrios Michailidis" <dm@chelsio.com> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> > > Sent: Friday, February 26, 2010 3:58 PM
> > > To: Dimitrios Michailidis
> > > Cc: netdev@vger.kernel.org
> > > Subject: Re: [PATCH 4/7] cxgb4: Add packet queues and packet DMA
code
> > >
> > > On Fri, 26 Feb 2010 15:35:36 -0800
> > > Dimitris Michailidis <dm@chelsio.com> wrote:
> > >
> > > > +
> > > > +/**
> > > > + *	need_skb_unmap - does the platform need unmapping of
sk_buffs?
> > > > + *
> > > > + *	Returns true if the platfrom needs sk_buff unmapping.
The
> > compiler
> > > > + *	optimizes away unecessary code if this returns true.
> > > > + */
> > > > +static inline int need_skb_unmap(void)
> > > > +{
> > > > +	/*
> > > > +	 * This structure is used to tell if the platfrom needs
buffer
> > > > +	 * unmapping by checking if DECLARE_PCI_UNMAP_ADDR
defines
> > anything.
> > > > +	 */
> > > > +	struct dummy {
> > > > +		DECLARE_PCI_UNMAP_ADDR(addr);
> > > > +	};
> > > > +
> > > > +	return sizeof(struct dummy) != 0;
> > > > +}
> > > > +
> > >
> > > I would prefer one code path and let the compiler decide if unmap
> > > should be nop; rather than this kind of trick code.
> >
> > I believe this function does not generate any code, the compiler
> > statically figures out the result and optimizes any conditionals
that
> > call it.  What option do you have in mind that would tell the
compiler
> > if unmap is nop?
> 
> Why do you care, let compiler decide.

I want the compiler to decide but I must be missing something.  How is
the compiler going to decide?  The purpose of this code is to tell the
compiler if unmapping is nop.  Does it have another way to tell?

^ permalink raw reply

* Re: [PATCH 10/13] bridge: Add multicast_router sysfs entries
From: Stephen Hemminger @ 2010-02-27  0:42 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David S. Miller, netdev
In-Reply-To: <E1Nl2Dj-0006wU-LU@gondolin.me.apana.org.au>

On Fri, 26 Feb 2010 23:35:15 +0800
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> bridge: Add multicast_router sysfs entries
> 
> This patch allows the user to forcibly enable/disable ports as
> having multicast routers attached.  A port with a multicast router
> will receive all multicast traffic.
> 
> The value 0 disables it completely.  The default is 1 which lets
> the system automatically detect the presence of routers (currently
> this is limited to picking up queries), and 2 means that the port
> will always receive all multicast traffic.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>


I like the functionality, but don't like users whacking on sysfs
directly. Could you send patches to integrate a user API into
bridge-utils; the utils are at: 
  git://git.kernel.org/pub/scm/linux/kernel/git/shemminger/bridge-utils.git

^ permalink raw reply

* [net-next-2.6 PATCH] ixgbe: move TC_PRIO_CONTROL check into ixgbe_select_queue()
From: Jeff Kirsher @ 2010-02-27  0:21 UTC (permalink / raw)
  To: davem; +Cc: netdev, gospo, John Fastabend, Peter P Waskiewicz Jr,
	Jeff Kirsher

From: John Fastabend <john.r.fastabend@intel.com>

Move TC_PRIO_CONTROL check and queue remapping into
ixgbe_select_queue().  Remapping queues after the qdisc
can result in the wrong qdisc queue being stopped with
netif_stop_subqueue().  Even if this is resolved and the
correct queue is stopped it can result in a queue being
blocked by TC_PRIO_CONTROL frames uneccesarily.  Moving
this into the select_queue routine maintains alignment
between tx_rings and qdisc queues.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/ixgbe/ixgbe_main.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index a961da2..45e3532 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -5639,8 +5639,14 @@ static u16 ixgbe_select_queue(struct net_device *dev, struct sk_buff *skb)
 		return txq;
 	}
 #endif
-	if (adapter->flags & IXGBE_FLAG_DCB_ENABLED)
-		return (skb->vlan_tci & IXGBE_TX_FLAGS_VLAN_PRIO_MASK) >> 13;
+	if (adapter->flags & IXGBE_FLAG_DCB_ENABLED) {
+		if (skb->priority == TC_PRIO_CONTROL)
+			txq = adapter->ring_feature[RING_F_DCB].indices-1;
+		else
+			txq = (skb->vlan_tci & IXGBE_TX_FLAGS_VLAN_PRIO_MASK)
+			       >> 13;
+		return txq;
+	}
 
 	return skb_tx_hash(dev, skb);
 }
@@ -5667,14 +5673,9 @@ static netdev_tx_t ixgbe_xmit_frame(struct sk_buff *skb,
 		tx_flags <<= IXGBE_TX_FLAGS_VLAN_SHIFT;
 		tx_flags |= IXGBE_TX_FLAGS_VLAN;
 	} else if (adapter->flags & IXGBE_FLAG_DCB_ENABLED) {
-		if (skb->priority != TC_PRIO_CONTROL) {
-			tx_flags |= ((skb->queue_mapping & 0x7) << 13);
-			tx_flags <<= IXGBE_TX_FLAGS_VLAN_SHIFT;
-			tx_flags |= IXGBE_TX_FLAGS_VLAN;
-		} else {
-			skb->queue_mapping =
-				adapter->ring_feature[RING_F_DCB].indices-1;
-		}
+		tx_flags |= ((skb->queue_mapping & 0x7) << 13);
+		tx_flags <<= IXGBE_TX_FLAGS_VLAN_SHIFT;
+		tx_flags |= IXGBE_TX_FLAGS_VLAN;
 	}
 
 	tx_ring = adapter->tx_ring[skb->queue_mapping];


^ permalink raw reply related

* [net-next-2.6 PATCH v2] net: consolidate netif_needs_gso() checks
From: Jeff Kirsher @ 2010-02-27  0:20 UTC (permalink / raw)
  To: davem; +Cc: netdev, gospo, John Fastabend, Jeff Kirsher

From: John Fastabend <john.r.fastabend@intel.com>

netif_needs_gso() is checked twice in the TX path once,
before submitting the skb to the qdisc and once after
it is dequeued from the qdisc just before calling
ndo_hard_start().  This opens a window for a user to
change the gso/tso or tx checksum settings that can
cause netif_needs_gso to be true in one check and false
in the other.

Specifically, changing TX checksum setting may cause
the warning in skb_gso_segment() to be triggered if
the checksum is calculated earlier.

This consolidates the netif_needs_gso() calls so that
the stack only checks if gso is needed in
dev_hard_start_xmit().

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/core/dev.c |   50 +++++++++++++++++++++++++++++---------------------
 1 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index eb7f1a4..626124d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1835,12 +1835,40 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int rc = NETDEV_TX_OK;
+	int need_gso = netif_needs_gso(dev, skb);
+
+	if (!need_gso) {
+		if (skb_has_frags(skb) &&
+		    !(dev->features & NETIF_F_FRAGLIST) &&
+		    __skb_linearize(skb))
+			goto out_kfree_skb;
+
+		/* Fragmented skb is linearized if device does not support SG,
+		 * or if at least one of fragments is in highmem and device
+		 * does not support DMA from it.
+		 */
+		if (skb_shinfo(skb)->nr_frags &&
+		    (!(dev->features & NETIF_F_SG) ||
+		      illegal_highdma(dev, skb)) &&
+		    __skb_linearize(skb))
+			goto out_kfree_skb;
+		/* If packet is not checksummed and device does not support
+		 * checksumming for this protocol, complete checksumming here.
+		 */
+		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			skb_set_transport_header(skb, skb->csum_start -
+				      skb_headroom(skb));
+			if (!dev_can_checksum(dev, skb) &&
+			     skb_checksum_help(skb))
+				goto out_kfree_skb;
+		}
+	}
 
 	if (likely(!skb->next)) {
 		if (!list_empty(&ptype_all))
 			dev_queue_xmit_nit(skb, dev);
 
-		if (netif_needs_gso(dev, skb)) {
+		if (need_gso) {
 			if (unlikely(dev_gso_segment(skb)))
 				goto out_kfree_skb;
 			if (skb->next)
@@ -2056,25 +2084,6 @@ int dev_queue_xmit(struct sk_buff *skb)
 	struct Qdisc *q;
 	int rc = -ENOMEM;
 
-	/* GSO will handle the following emulations directly. */
-	if (netif_needs_gso(dev, skb))
-		goto gso;
-
-	/* Convert a paged skb to linear, if required */
-	if (skb_needs_linearize(skb, dev) && __skb_linearize(skb))
-		goto out_kfree_skb;
-
-	/* If packet is not checksummed and device does not support
-	 * checksumming for this protocol, complete checksumming here.
-	 */
-	if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		skb_set_transport_header(skb, skb->csum_start -
-					      skb_headroom(skb));
-		if (!dev_can_checksum(dev, skb) && skb_checksum_help(skb))
-			goto out_kfree_skb;
-	}
-
-gso:
 	/* Disable soft irqs for various locks below. Also
 	 * stops preemption for RCU.
 	 */
@@ -2133,7 +2142,6 @@ gso:
 	rc = -ENETDOWN;
 	rcu_read_unlock_bh();
 
-out_kfree_skb:
 	kfree_skb(skb);
 	return rc;
 out:


^ permalink raw reply related

* Re: [PATCH 4/7] cxgb4: Add packet queues and packet DMA code
From: Stephen Hemminger @ 2010-02-27  0:16 UTC (permalink / raw)
  To: Dimitrios Michailidis; +Cc: netdev
In-Reply-To: <8A71B368A89016469F72CD08050AD33401359B69@maui.asicdesigners.com>

On Fri, 26 Feb 2010 16:10:07 -0800
"Dimitrios Michailidis" <dm@chelsio.com> wrote:

> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> > Sent: Friday, February 26, 2010 3:58 PM
> > To: Dimitrios Michailidis
> > Cc: netdev@vger.kernel.org
> > Subject: Re: [PATCH 4/7] cxgb4: Add packet queues and packet DMA code
> > 
> > On Fri, 26 Feb 2010 15:35:36 -0800
> > Dimitris Michailidis <dm@chelsio.com> wrote:
> > 
> > > +
> > > +/**
> > > + *	need_skb_unmap - does the platform need unmapping of sk_buffs?
> > > + *
> > > + *	Returns true if the platfrom needs sk_buff unmapping.  The
> compiler
> > > + *	optimizes away unecessary code if this returns true.
> > > + */
> > > +static inline int need_skb_unmap(void)
> > > +{
> > > +	/*
> > > +	 * This structure is used to tell if the platfrom needs buffer
> > > +	 * unmapping by checking if DECLARE_PCI_UNMAP_ADDR defines
> anything.
> > > +	 */
> > > +	struct dummy {
> > > +		DECLARE_PCI_UNMAP_ADDR(addr);
> > > +	};
> > > +
> > > +	return sizeof(struct dummy) != 0;
> > > +}
> > > +
> > 
> > I would prefer one code path and let the compiler decide if unmap
> > should be nop; rather than this kind of trick code.
> 
> I believe this function does not generate any code, the compiler
> statically figures out the result and optimizes any conditionals that
> call it.  What option do you have in mind that would tell the compiler
> if unmap is nop?

Why do you care, let compiler decide.

-- 

^ permalink raw reply

* RE: [PATCH 4/7] cxgb4: Add packet queues and packet DMA code
From: Dimitrios Michailidis @ 2010-02-27  0:10 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev



> -----Original Message-----
> From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> Sent: Friday, February 26, 2010 3:58 PM
> To: Dimitrios Michailidis
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH 4/7] cxgb4: Add packet queues and packet DMA code
> 
> On Fri, 26 Feb 2010 15:35:36 -0800
> Dimitris Michailidis <dm@chelsio.com> wrote:
> 
> > +
> > +/**
> > + *	need_skb_unmap - does the platform need unmapping of sk_buffs?
> > + *
> > + *	Returns true if the platfrom needs sk_buff unmapping.  The
compiler
> > + *	optimizes away unecessary code if this returns true.
> > + */
> > +static inline int need_skb_unmap(void)
> > +{
> > +	/*
> > +	 * This structure is used to tell if the platfrom needs buffer
> > +	 * unmapping by checking if DECLARE_PCI_UNMAP_ADDR defines
anything.
> > +	 */
> > +	struct dummy {
> > +		DECLARE_PCI_UNMAP_ADDR(addr);
> > +	};
> > +
> > +	return sizeof(struct dummy) != 0;
> > +}
> > +
> 
> I would prefer one code path and let the compiler decide if unmap
> should be nop; rather than this kind of trick code.

I believe this function does not generate any code, the compiler
statically figures out the result and optimizes any conditionals that
call it.  What option do you have in mind that would tell the compiler
if unmap is nop?

^ permalink raw reply

* [PATCH v3 4/7] pci: Add VPD information field helper functions
From: Matt Carlson @ 2010-02-27  0:04 UTC (permalink / raw)
  To: jbarnes; +Cc: linux-pci, netdev, andy, mcarlson, Michael Chan

This patch adds a preprocessor constant to describe the PCI VPD
information field header size and an inline function to extract the
size of the information field itself.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/bnx2.c  |   11 ++++++-----
 drivers/net/tg3.c   |    7 ++++---
 include/linux/pci.h |   13 +++++++++++++
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 02809da..5909d1a 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -7774,14 +7774,15 @@ bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 			goto vpd_done;
 
 		while (i < (block_end - 2)) {
-			int len = data[i + 2];
+			int len = pci_vpd_info_field_size(&data[i]);
 
-			if (i + 3 + len > block_end)
+			if (i + PCI_VPD_INFO_FLD_HDR_SIZE + len > block_end)
 				goto vpd_done;
 
 			if (data[i] == 'M' && data[i + 1] == 'N') {
 				if (len != 4 ||
-				    memcmp(&data[i + 3], "1028", 4))
+				    memcmp(&data[i + PCI_VPD_INFO_FLD_HDR_SIZE],
+					   "1028", 4))
 					goto vpd_done;
 				mn_match = true;
 
@@ -7790,9 +7791,9 @@ bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 					goto vpd_done;
 
 				v0_len = len;
-				v0_str = &data[i + 3];
+				v0_str = &data[i + PCI_VPD_INFO_FLD_HDR_SIZE];
 			}
-			i += 3 + len;
+			i += PCI_VPD_INFO_FLD_HDR_SIZE + len;
 
 			if (mn_match && v0_str) {
 				memcpy(bp->fw_version, v0_str, v0_len);
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 3e5696b..fc510dc 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12497,9 +12497,9 @@ static void __devinit tg3_read_partno(struct tg3 *tp)
 		while (i < (block_end - 2)) {
 			if (vpd_data[i + 0] == 'P' &&
 			    vpd_data[i + 1] == 'N') {
-				int partno_len = vpd_data[i + 2];
+				int partno_len = pci_vpd_info_field_size(&vpd_data[i]);
 
-				i += 3;
+				i += PCI_VPD_INFO_FLD_HDR_SIZE;
 				if (partno_len > TG3_BPN_SIZE ||
 				    (partno_len + i) > TG3_NVM_VPD_LEN)
 					goto out_not_found;
@@ -12510,7 +12510,8 @@ static void __devinit tg3_read_partno(struct tg3 *tp)
 				/* Success. */
 				return;
 			}
-			i += 3 + vpd_data[i + 2];
+			i += PCI_VPD_INFO_FLD_HDR_SIZE +
+			     pci_vpd_info_field_size(&vpd_data[i]);
 		}
 
 		/* Part number not found. */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c2da353..ead2492 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1362,6 +1362,8 @@ void pci_request_acs(void);
 #define PCI_VPD_LRDT_TAG_SIZE		3
 #define PCI_VPD_SRDT_TAG_SIZE		1
 
+#define PCI_VPD_INFO_FLD_HDR_SIZE	3
+
 /**
  * pci_vpd_lrdt_size - Extracts the Large Resource Data Type length
  * @lrdt: Pointer to the beginning of the Large Resource Data Type tag
@@ -1385,6 +1387,17 @@ static inline u8 pci_vpd_srdt_size(const u8 *srdt)
 }
 
 /**
+ * pci_vpd_info_field_size - Extracts the information field length
+ * @lrdt: Pointer to the beginning of an information field header
+ *
+ * Returns the extracted information field length.
+ */
+static inline u8 pci_vpd_info_field_size(const u8 *info_field)
+{
+	return info_field[2];
+}
+
+/**
  * pci_vpd_find_tag - Locates the Resource Data Type tag provided
  * @buf: Pointer to buffered vpd data
  * @off: The offset into the buffer at which to begin the search
-- 
1.6.4.4



^ permalink raw reply related

* [PATCH v3 5/7] pci: Add helper to search for VPD keywords
From: Matt Carlson @ 2010-02-27  0:04 UTC (permalink / raw)
  To: jbarnes; +Cc: linux-pci, netdev, andy, mcarlson, Michael Chan

This patch adds the pci_vpd_find_info_keyword() helper function to
find information field keywords within read-only and read-write large
resource data type sections.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/bnx2.c  |   54 ++++++++++++++++++++++++++++----------------------
 drivers/net/tg3.c   |   32 ++++++++++++-----------------
 drivers/pci/vpd.c   |   18 +++++++++++++++++
 include/linux/pci.h |   17 ++++++++++++++++
 4 files changed, 78 insertions(+), 43 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 5909d1a..9feab11 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -7759,48 +7759,54 @@ bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 	}
 
 	for (i = 0; i <= BNX2_VPD_LEN - 3; ) {
-		unsigned int block_end;
+		int j;
+		unsigned int block_end, rosize;
 
 		i = pci_vpd_find_tag(data, i, BNX2_VPD_LEN,
 				     PCI_VPD_LRDT_RO_DATA);
 		if (i < 0)
 			break;
 
-		block_end = (i + PCI_VPD_LRDT_TAG_SIZE +
-			    pci_vpd_lrdt_size(&data[i]));
+		rosize = pci_vpd_lrdt_size(&data[i]);
+		block_end = i + PCI_VPD_LRDT_TAG_SIZE + rosize;
 		i += PCI_VPD_LRDT_TAG_SIZE;
 
 		if (block_end > BNX2_VPD_LEN)
 			goto vpd_done;
 
-		while (i < (block_end - 2)) {
-			int len = pci_vpd_info_field_size(&data[i]);
+		j = pci_vpd_find_info_keyword(data, i, rosize,
+					      PCI_VPD_RO_KEYWORD_MFR_ID);
+		if (j > 0) {
+			int len = pci_vpd_info_field_size(&data[j]);
 
-			if (i + PCI_VPD_INFO_FLD_HDR_SIZE + len > block_end)
+			if (j + PCI_VPD_INFO_FLD_HDR_SIZE + len > block_end ||
+			    len != 4 ||
+			    memcmp(&data[j + PCI_VPD_INFO_FLD_HDR_SIZE],
+				   "1028", 4))
 				goto vpd_done;
 
-			if (data[i] == 'M' && data[i + 1] == 'N') {
-				if (len != 4 ||
-				    memcmp(&data[i + PCI_VPD_INFO_FLD_HDR_SIZE],
-					   "1028", 4))
-					goto vpd_done;
-				mn_match = true;
+			mn_match = true;
+		}
 
-			} else if (data[i] == 'V' && data[i + 1] == '0') {
-				if (len > BNX2_MAX_VER_SLEN)
-					goto vpd_done;
+		j = pci_vpd_find_info_keyword(data, i, rosize,
+					      PCI_VPD_RO_KEYWORD_VENDOR0);
+		if (j > 0) {
+			int len = pci_vpd_info_field_size(&data[j]);
 
-				v0_len = len;
-				v0_str = &data[i + PCI_VPD_INFO_FLD_HDR_SIZE];
-			}
-			i += PCI_VPD_INFO_FLD_HDR_SIZE + len;
-
-			if (mn_match && v0_str) {
-				memcpy(bp->fw_version, v0_str, v0_len);
-				bp->fw_version[v0_len] = ' ';
+			j += PCI_VPD_INFO_FLD_HDR_SIZE;
+			if (j + len > block_end || len > BNX2_MAX_VER_SLEN)
 				goto vpd_done;
-			}
+
+			v0_len = len;
+			v0_str = &data[j];
 		}
+
+		if (mn_match && v0_str) {
+			memcpy(bp->fw_version, v0_str, v0_len);
+			bp->fw_version[v0_len] = ' ';
+			goto vpd_done;
+		}
+
 		goto vpd_done;
 	}
 
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index fc510dc..731ba67 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12479,39 +12479,33 @@ static void __devinit tg3_read_partno(struct tg3 *tp)
 
 	/* Now parse and find the part number. */
 	for (i = 0; i < TG3_NVM_VPD_LEN - 2; ) {
-		unsigned int block_end;
+		unsigned int block_end, rosize;
 
 		i = pci_vpd_find_tag(vpd_data, i, TG3_NVM_VPD_LEN,
 				     PCI_VPD_LRDT_RO_DATA);
 		if (i < 0)
 			break;
 
-		block_end = i + PCI_VPD_LRDT_TAG_SIZE +
-			    pci_vpd_lrdt_size(&vpd_data[i]);
-
+		rosize = pci_vpd_lrdt_size(&vpd_data[i]);
+		block_end = i + PCI_VPD_LRDT_TAG_SIZE + rosize;
 		i += PCI_VPD_LRDT_TAG_SIZE;
 
 		if (block_end > TG3_NVM_VPD_LEN)
 			goto out_not_found;
 
-		while (i < (block_end - 2)) {
-			if (vpd_data[i + 0] == 'P' &&
-			    vpd_data[i + 1] == 'N') {
-				int partno_len = pci_vpd_info_field_size(&vpd_data[i]);
+		i = pci_vpd_find_info_keyword(vpd_data, i, rosize,
+					      PCI_VPD_RO_KEYWORD_PARTNO);
+		if (i > 0) {
+			u8 len = pci_vpd_info_field_size(&vpd_data[i]);
 
-				i += PCI_VPD_INFO_FLD_HDR_SIZE;
-				if (partno_len > TG3_BPN_SIZE ||
-				    (partno_len + i) > TG3_NVM_VPD_LEN)
-					goto out_not_found;
+			i += PCI_VPD_INFO_FLD_HDR_SIZE;
+			if (len > TG3_BPN_SIZE ||
+			    (len + i) > TG3_NVM_VPD_LEN)
+				break;
 
-				memcpy(tp->board_part_number,
-				       &vpd_data[i], partno_len);
+			memcpy(tp->board_part_number, &vpd_data[i], len);
 
-				/* Success. */
-				return;
-			}
-			i += PCI_VPD_INFO_FLD_HDR_SIZE +
-			     pci_vpd_info_field_size(&vpd_data[i]);
+			return;
 		}
 
 		/* Part number not found. */
diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index 6bc5545..a5a5ca1 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -41,3 +41,21 @@ int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt)
 	return -ENOENT;
 }
 EXPORT_SYMBOL_GPL(pci_vpd_find_tag);
+
+int pci_vpd_find_info_keyword(const u8 *buf, unsigned int off,
+			      unsigned int len, const char *kw)
+{
+	int i;
+
+	for (i = off; i + PCI_VPD_INFO_FLD_HDR_SIZE <= off + len;) {
+		if (buf[i + 0] == kw[0] &&
+		    buf[i + 1] == kw[1])
+			return i;
+
+		i += PCI_VPD_INFO_FLD_HDR_SIZE +
+		     pci_vpd_info_field_size(&buf[i]);
+	}
+
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(pci_vpd_find_info_keyword);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ead2492..7a7c7c3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1364,6 +1364,10 @@ void pci_request_acs(void);
 
 #define PCI_VPD_INFO_FLD_HDR_SIZE	3
 
+#define PCI_VPD_RO_KEYWORD_PARTNO	"PN"
+#define PCI_VPD_RO_KEYWORD_MFR_ID	"MN"
+#define PCI_VPD_RO_KEYWORD_VENDOR0	"V0"
+
 /**
  * pci_vpd_lrdt_size - Extracts the Large Resource Data Type length
  * @lrdt: Pointer to the beginning of the Large Resource Data Type tag
@@ -1409,5 +1413,18 @@ static inline u8 pci_vpd_info_field_size(const u8 *info_field)
  */
 int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt);
 
+/**
+ * pci_vpd_find_info_keyword - Locates an information field keyword in the VPD
+ * @buf: Pointer to buffered vpd data
+ * @off: The offset into the buffer at which to begin the search
+ * @len: The length of the buffer area, relative to off, in which to search
+ * @kw: The keyword to search for
+ *
+ * Returns the index where the information field keyword was found or
+ * -ENOENT otherwise.
+ */
+int pci_vpd_find_info_keyword(const u8 *buf, unsigned int off,
+			      unsigned int len, const char *kw);
+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */
-- 
1.6.4.4



^ permalink raw reply related

* [PATCH v3 7/7] tg3: Remove now useless VPD code
From: Matt Carlson @ 2010-02-27  0:04 UTC (permalink / raw)
  To: jbarnes; +Cc: linux-pci, netdev, andy, mcarlson

Now that the VPD searching code is abstracted away, the outer loop used
to detect the read-only large resource data type section is useless.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
---
 drivers/net/tg3.c |   52 +++++++++++++++++++++++-----------------------------
 1 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 731ba67..6e5feb6 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12440,7 +12440,8 @@ skip_phy_reset:
 static void __devinit tg3_read_partno(struct tg3 *tp)
 {
 	unsigned char vpd_data[TG3_NVM_VPD_LEN];   /* in little-endian format */
-	int i;
+	unsigned int block_end, rosize, len;
+	int i = 0;
 	u32 magic;
 
 	if ((tp->tg3_flags3 & TG3_FLG3_NO_NVRAM) ||
@@ -12462,7 +12463,7 @@ static void __devinit tg3_read_partno(struct tg3 *tp)
 		}
 	} else {
 		ssize_t cnt;
-		unsigned int pos = 0, i = 0;
+		unsigned int pos = 0;
 
 		for (; pos < TG3_NVM_VPD_LEN && i < 3; i++, pos += cnt) {
 			cnt = pci_read_vpd(tp->pdev, pos,
@@ -12477,40 +12478,33 @@ static void __devinit tg3_read_partno(struct tg3 *tp)
 			goto out_not_found;
 	}
 
-	/* Now parse and find the part number. */
-	for (i = 0; i < TG3_NVM_VPD_LEN - 2; ) {
-		unsigned int block_end, rosize;
+	i = pci_vpd_find_tag(vpd_data, 0, TG3_NVM_VPD_LEN,
+			     PCI_VPD_LRDT_RO_DATA);
+	if (i < 0)
+		goto out_not_found;
 
-		i = pci_vpd_find_tag(vpd_data, i, TG3_NVM_VPD_LEN,
-				     PCI_VPD_LRDT_RO_DATA);
-		if (i < 0)
-			break;
+	rosize = pci_vpd_lrdt_size(&vpd_data[i]);
+	block_end = i + PCI_VPD_LRDT_TAG_SIZE + rosize;
+	i += PCI_VPD_LRDT_TAG_SIZE;
 
-		rosize = pci_vpd_lrdt_size(&vpd_data[i]);
-		block_end = i + PCI_VPD_LRDT_TAG_SIZE + rosize;
-		i += PCI_VPD_LRDT_TAG_SIZE;
+	if (block_end > TG3_NVM_VPD_LEN)
+		goto out_not_found;
 
-		if (block_end > TG3_NVM_VPD_LEN)
-			goto out_not_found;
+	i = pci_vpd_find_info_keyword(vpd_data, i, rosize,
+				      PCI_VPD_RO_KEYWORD_PARTNO);
+	if (i < 0)
+		goto out_not_found;
 
-		i = pci_vpd_find_info_keyword(vpd_data, i, rosize,
-					      PCI_VPD_RO_KEYWORD_PARTNO);
-		if (i > 0) {
-			u8 len = pci_vpd_info_field_size(&vpd_data[i]);
+	len = pci_vpd_info_field_size(&vpd_data[i]);
 
-			i += PCI_VPD_INFO_FLD_HDR_SIZE;
-			if (len > TG3_BPN_SIZE ||
-			    (len + i) > TG3_NVM_VPD_LEN)
-				break;
-
-			memcpy(tp->board_part_number, &vpd_data[i], len);
+	i += PCI_VPD_INFO_FLD_HDR_SIZE;
+	if (len > TG3_BPN_SIZE ||
+	    (len + i) > TG3_NVM_VPD_LEN)
+		goto out_not_found;
 
-			return;
-		}
+	memcpy(tp->board_part_number, &vpd_data[i], len);
 
-		/* Part number not found. */
-		goto out_not_found;
-	}
+	return;
 
 out_not_found:
 	if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5906)
-- 
1.6.4.4



^ permalink raw reply related

* [PATCH v3 3/7] pci: Add helper to find a VPD resource data type
From: Matt Carlson @ 2010-02-27  0:04 UTC (permalink / raw)
  To: jbarnes; +Cc: linux-pci, netdev, andy, mcarlson, Michael Chan

This patch adds the pci_vpd_find_tag() helper function to find VPD
resource data types in a buffer.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/bnx2.c   |   24 ++++--------------------
 drivers/net/tg3.c    |   26 +++++---------------------
 drivers/pci/Makefile |    2 +-
 drivers/pci/vpd.c    |   43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h  |   12 ++++++++++++
 5 files changed, 65 insertions(+), 42 deletions(-)
 create mode 100644 drivers/pci/vpd.c

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 7caa98d..02809da 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -7759,28 +7759,12 @@ bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 	}
 
 	for (i = 0; i <= BNX2_VPD_LEN - 3; ) {
-		unsigned char val = data[i];
 		unsigned int block_end;
 
-		if (val & PCI_VPD_LRDT) {
-			if (i + PCI_VPD_LRDT_TAG_SIZE > BNX2_VPD_LEN)
-				break;
-
-			if (val != PCI_VPD_LRDT_RO_DATA) {
-				i += PCI_VPD_LRDT_TAG_SIZE +
-				     pci_vpd_lrdt_size(&data[i]);
-
-				continue;
-			}
-		} else {
-			if ((val & PCI_VPD_SRDT_TIN_MASK) == PCI_VPD_STIN_END)
-				break;
-
-			i += PCI_VPD_SRDT_TAG_SIZE +
-			     pci_vpd_srdt_size(&data[i]);
-
-			continue;
-		}
+		i = pci_vpd_find_tag(data, i, BNX2_VPD_LEN,
+				     PCI_VPD_LRDT_RO_DATA);
+		if (i < 0)
+			break;
 
 		block_end = (i + PCI_VPD_LRDT_TAG_SIZE +
 			    pci_vpd_lrdt_size(&data[i]));
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 4c24998..3e5696b 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12440,7 +12440,7 @@ skip_phy_reset:
 static void __devinit tg3_read_partno(struct tg3 *tp)
 {
 	unsigned char vpd_data[TG3_NVM_VPD_LEN];   /* in little-endian format */
-	unsigned int i;
+	int i;
 	u32 magic;
 
 	if ((tp->tg3_flags3 & TG3_FLG3_NO_NVRAM) ||
@@ -12479,28 +12479,12 @@ static void __devinit tg3_read_partno(struct tg3 *tp)
 
 	/* Now parse and find the part number. */
 	for (i = 0; i < TG3_NVM_VPD_LEN - 2; ) {
-		unsigned char val = vpd_data[i];
 		unsigned int block_end;
 
-		if (val & PCI_VPD_LRDT) {
-			if (i + PCI_VPD_LRDT_TAG_SIZE > TG3_NVM_VPD_LEN)
-				break;
-
-			if (val != PCI_VPD_LRDT_RO_DATA) {
-				i += PCI_VPD_LRDT_TAG_SIZE +
-				     pci_vpd_lrdt_size(&vpd_data[i]);
-
-				continue;
-			}
-		} else {
-			if ((val & PCI_VPD_SRDT_TIN_MASK) == PCI_VPD_STIN_END)
-				break;
-
-			i += PCI_VPD_SRDT_TAG_SIZE +
-			     pci_vpd_srdt_size(&vpd_data[i]);
-
-			continue;
-		}
+		i = pci_vpd_find_tag(vpd_data, i, TG3_NVM_VPD_LEN,
+				     PCI_VPD_LRDT_RO_DATA);
+		if (i < 0)
+			break;
 
 		block_end = i + PCI_VPD_LRDT_TAG_SIZE +
 			    pci_vpd_lrdt_size(&vpd_data[i]);
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 4df48d5..b2f6d77 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -4,7 +4,7 @@
 
 obj-y		+= access.o bus.o probe.o remove.o pci.o quirks.o \
 			pci-driver.o search.o pci-sysfs.o rom.o setup-res.o \
-			irq.o
+			irq.o vpd.o
 obj-$(CONFIG_PROC_FS) += proc.o
 obj-$(CONFIG_SYSFS) += slot.o
 
diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
new file mode 100644
index 0000000..6bc5545
--- /dev/null
+++ b/drivers/pci/vpd.c
@@ -0,0 +1,43 @@
+/*
+ * File:	vpd.c
+ * Purpose:	Provide PCI VPD support
+ *
+ * Copyright (C) 2010 Broadcom Corporation.
+ */
+
+#include <linux/pci.h>
+
+int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt)
+{
+	int i;
+
+	for (i = off; i < len; ) {
+		u8 val = buf[i];
+
+		if (val & PCI_VPD_LRDT) {
+			/* Don't return success of the tag isn't complete */
+			if (i + PCI_VPD_LRDT_TAG_SIZE > len)
+				break;
+
+			if (val == rdt)
+				return i;
+
+			i += PCI_VPD_LRDT_TAG_SIZE +
+			     pci_vpd_lrdt_size(&buf[i]);
+		} else {
+			u8 tag = val & ~PCI_VPD_SRDT_LEN_MASK;
+
+			if (tag == rdt)
+				return i;
+
+			if (tag == PCI_VPD_SRDT_END)
+				break;
+
+			i += PCI_VPD_SRDT_TAG_SIZE +
+			     pci_vpd_srdt_size(&buf[i]);
+		}
+	}
+
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(pci_vpd_find_tag);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d6f2b17..c2da353 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1384,5 +1384,17 @@ static inline u8 pci_vpd_srdt_size(const u8 *srdt)
 	return (*srdt) & PCI_VPD_SRDT_LEN_MASK;
 }
 
+/**
+ * pci_vpd_find_tag - Locates the Resource Data Type tag provided
+ * @buf: Pointer to buffered vpd data
+ * @off: The offset into the buffer at which to begin the search
+ * @len: The length of the vpd buffer
+ * @rdt: The Resource Data Type to search for
+ *
+ * Returns the index where the Resource Data Type was found or
+ * -ENOENT otherwise.
+ */
+int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt);
+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */
-- 
1.6.4.4



^ permalink raw reply related

* [PATCH v3 2/7] pci: Add large and small resource data type code
From: Matt Carlson @ 2010-02-27  0:04 UTC (permalink / raw)
  To: jbarnes; +Cc: linux-pci, netdev, andy, mcarlson, Michael Chan

This patch introduces more VPD preprocessor definitions to identify some
small and large resource data type item names.  The patch then continues
to correct how the tg3 and bnx2 drivers search for the "read-only data"
large resource data type.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/bnx2.c  |   23 +++++++++++++++++------
 drivers/net/tg3.c   |   23 +++++++++++++++++------
 include/linux/pci.h |   34 +++++++++++++++++++++++++++++++++-
 3 files changed, 67 insertions(+), 13 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index d466208..7caa98d 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -7762,15 +7762,26 @@ bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 		unsigned char val = data[i];
 		unsigned int block_end;
 
-		if (val == 0x82 || val == 0x91) {
-			i += PCI_VPD_LRDT_TAG_SIZE +
-			     pci_vpd_lrdt_size(&data[i]);
+		if (val & PCI_VPD_LRDT) {
+			if (i + PCI_VPD_LRDT_TAG_SIZE > BNX2_VPD_LEN)
+				break;
+
+			if (val != PCI_VPD_LRDT_RO_DATA) {
+				i += PCI_VPD_LRDT_TAG_SIZE +
+				     pci_vpd_lrdt_size(&data[i]);
+
+				continue;
+			}
+		} else {
+			if ((val & PCI_VPD_SRDT_TIN_MASK) == PCI_VPD_STIN_END)
+				break;
+
+			i += PCI_VPD_SRDT_TAG_SIZE +
+			     pci_vpd_srdt_size(&data[i]);
+
 			continue;
 		}
 
-		if (val != 0x90)
-			goto vpd_done;
-
 		block_end = (i + PCI_VPD_LRDT_TAG_SIZE +
 			    pci_vpd_lrdt_size(&data[i]));
 		i += PCI_VPD_LRDT_TAG_SIZE;
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index d01e133..4c24998 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12482,15 +12482,26 @@ static void __devinit tg3_read_partno(struct tg3 *tp)
 		unsigned char val = vpd_data[i];
 		unsigned int block_end;
 
-		if (val == 0x82 || val == 0x91) {
-			i += PCI_VPD_LRDT_TAG_SIZE +
-			     pci_vpd_lrdt_size(&vpd_data[i]);
+		if (val & PCI_VPD_LRDT) {
+			if (i + PCI_VPD_LRDT_TAG_SIZE > TG3_NVM_VPD_LEN)
+				break;
+
+			if (val != PCI_VPD_LRDT_RO_DATA) {
+				i += PCI_VPD_LRDT_TAG_SIZE +
+				     pci_vpd_lrdt_size(&vpd_data[i]);
+
+				continue;
+			}
+		} else {
+			if ((val & PCI_VPD_SRDT_TIN_MASK) == PCI_VPD_STIN_END)
+				break;
+
+			i += PCI_VPD_SRDT_TAG_SIZE +
+			     pci_vpd_srdt_size(&vpd_data[i]);
+
 			continue;
 		}
 
-		if (val != 0x90)
-			goto out_not_found;
-
 		block_end = i + PCI_VPD_LRDT_TAG_SIZE +
 			    pci_vpd_lrdt_size(&vpd_data[i]);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 038f3c0..d6f2b17 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1339,7 +1339,28 @@ static inline bool pci_is_pcie(struct pci_dev *dev)
 void pci_request_acs(void);
 
 
-#define PCI_VPD_LRDT_TAG_SIZE	3
+#define PCI_VPD_LRDT			0x80	/* Large Resource Data Type */
+#define PCI_VPD_LRDT_ID(x)		(x | PCI_VPD_LRDT)
+
+/* Large Resource Data Type Tag Item Names */
+#define PCI_VPD_LTIN_ID_STRING		0x02	/* Identifier String */
+#define PCI_VPD_LTIN_RO_DATA		0x10	/* Read-Only Data */
+#define PCI_VPD_LTIN_RW_DATA		0x11	/* Read-Write Data */
+
+#define PCI_VPD_LRDT_ID_STRING		PCI_VPD_LRDT_ID(PCI_VPD_LTIN_ID_STRING)
+#define PCI_VPD_LRDT_RO_DATA		PCI_VPD_LRDT_ID(PCI_VPD_LTIN_RO_DATA)
+#define PCI_VPD_LRDT_RW_DATA		PCI_VPD_LRDT_ID(PCI_VPD_LTIN_RW_DATA)
+
+/* Small Resource Data Type Tag Item Names */
+#define PCI_VPD_STIN_END		0x78	/* End */
+
+#define PCI_VPD_SRDT_END		PCI_VPD_STIN_END
+
+#define PCI_VPD_SRDT_TIN_MASK		0x78
+#define PCI_VPD_SRDT_LEN_MASK		0x07
+
+#define PCI_VPD_LRDT_TAG_SIZE		3
+#define PCI_VPD_SRDT_TAG_SIZE		1
 
 /**
  * pci_vpd_lrdt_size - Extracts the Large Resource Data Type length
@@ -1352,5 +1373,16 @@ static inline u16 pci_vpd_lrdt_size(const u8 *lrdt)
 	return (u16)lrdt[1] + ((u16)lrdt[2] << 8);
 }
 
+/**
+ * pci_vpd_srdt_size - Extracts the Small Resource Data Type length
+ * @lrdt: Pointer to the beginning of the Small Resource Data Type tag
+ *
+ * Returns the extracted Small Resource Data Type length.
+ */
+static inline u8 pci_vpd_srdt_size(const u8 *srdt)
+{
+	return (*srdt) & PCI_VPD_SRDT_LEN_MASK;
+}
+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */
-- 
1.6.4.4



^ permalink raw reply related

* [PATCH v3 6/7] bnx2: Remove now useless VPD code
From: Matt Carlson @ 2010-02-27  0:04 UTC (permalink / raw)
  To: jbarnes; +Cc: linux-pci, netdev, andy, mcarlson

Now that the VPD searching code is abstracted away, the outer loop used
to detect the read-only large resource data type section is useless.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
---
 drivers/net/bnx2.c |   74 ++++++++++++++++++++-------------------------------
 1 files changed, 29 insertions(+), 45 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 9feab11..24e9c21 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -7733,10 +7733,9 @@ bnx2_get_pci_speed(struct bnx2 *bp)
 static void __devinit
 bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 {
-	int rc, i, v0_len = 0;
+	int rc, i, j;
 	u8 *data;
-	u8 *v0_str = NULL;
-	bool mn_match = false;
+	unsigned int block_end, rosize, len;
 
 #define BNX2_VPD_NVRAM_OFFSET	0x300
 #define BNX2_VPD_LEN		128
@@ -7758,57 +7757,42 @@ bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 		data[i + 3] = data[i + BNX2_VPD_LEN];
 	}
 
-	for (i = 0; i <= BNX2_VPD_LEN - 3; ) {
-		int j;
-		unsigned int block_end, rosize;
-
-		i = pci_vpd_find_tag(data, i, BNX2_VPD_LEN,
-				     PCI_VPD_LRDT_RO_DATA);
-		if (i < 0)
-			break;
-
-		rosize = pci_vpd_lrdt_size(&data[i]);
-		block_end = i + PCI_VPD_LRDT_TAG_SIZE + rosize;
-		i += PCI_VPD_LRDT_TAG_SIZE;
-
-		if (block_end > BNX2_VPD_LEN)
-			goto vpd_done;
+	i = pci_vpd_find_tag(data, 0, BNX2_VPD_LEN, PCI_VPD_LRDT_RO_DATA);
+	if (i < 0)
+		goto vpd_done;
 
-		j = pci_vpd_find_info_keyword(data, i, rosize,
-					      PCI_VPD_RO_KEYWORD_MFR_ID);
-		if (j > 0) {
-			int len = pci_vpd_info_field_size(&data[j]);
+	rosize = pci_vpd_lrdt_size(&data[i]);
+	i += PCI_VPD_LRDT_TAG_SIZE;
+	block_end = i + rosize;
 
-			if (j + PCI_VPD_INFO_FLD_HDR_SIZE + len > block_end ||
-			    len != 4 ||
-			    memcmp(&data[j + PCI_VPD_INFO_FLD_HDR_SIZE],
-				   "1028", 4))
-				goto vpd_done;
+	if (block_end > BNX2_VPD_LEN)
+		goto vpd_done;
 
-			mn_match = true;
-		}
+	j = pci_vpd_find_info_keyword(data, i, rosize,
+				      PCI_VPD_RO_KEYWORD_MFR_ID);
+	if (j < 0)
+		goto vpd_done;
 
-		j = pci_vpd_find_info_keyword(data, i, rosize,
-					      PCI_VPD_RO_KEYWORD_VENDOR0);
-		if (j > 0) {
-			int len = pci_vpd_info_field_size(&data[j]);
+	len = pci_vpd_info_field_size(&data[j]);
 
-			j += PCI_VPD_INFO_FLD_HDR_SIZE;
-			if (j + len > block_end || len > BNX2_MAX_VER_SLEN)
-				goto vpd_done;
+	j += PCI_VPD_INFO_FLD_HDR_SIZE;
+	if (j + len > block_end || len != 4 ||
+	    memcmp(&data[j], "1028", 4))
+		goto vpd_done;
 
-			v0_len = len;
-			v0_str = &data[j];
-		}
+	j = pci_vpd_find_info_keyword(data, i, rosize,
+				      PCI_VPD_RO_KEYWORD_VENDOR0);
+	if (j < 0)
+		goto vpd_done;
 
-		if (mn_match && v0_str) {
-			memcpy(bp->fw_version, v0_str, v0_len);
-			bp->fw_version[v0_len] = ' ';
-			goto vpd_done;
-		}
+	len = pci_vpd_info_field_size(&data[j]);
 
+	j += PCI_VPD_INFO_FLD_HDR_SIZE;
+	if (j + len > block_end || len > BNX2_MAX_VER_SLEN)
 		goto vpd_done;
-	}
+
+	memcpy(bp->fw_version, &data[j], len);
+	bp->fw_version[len] = ' ';
 
 vpd_done:
 	kfree(data);
-- 
1.6.4.4

^ permalink raw reply related

* [PATCH v3 1/7] pci: Add PCI LRDT tag size and section size
From: Matt Carlson @ 2010-02-27  0:04 UTC (permalink / raw)
  To: jbarnes; +Cc: linux-pci, netdev, andy, mcarlson, Michael Chan

This patch adds a preprocessor constant to describe the PCI VPD large
resource data type tag size and an inline function to extract the large
resource section size from the large resource data type tag.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/bnx2.c  |    8 +++++---
 drivers/net/tg3.c   |   13 ++++++-------
 include/linux/pci.h |   14 ++++++++++++++
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 65df1de..d466208 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -7763,15 +7763,17 @@ bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 		unsigned int block_end;
 
 		if (val == 0x82 || val == 0x91) {
-			i = (i + 3 + (data[i + 1] + (data[i + 2] << 8)));
+			i += PCI_VPD_LRDT_TAG_SIZE +
+			     pci_vpd_lrdt_size(&data[i]);
 			continue;
 		}
 
 		if (val != 0x90)
 			goto vpd_done;
 
-		block_end = (i + 3 + (data[i + 1] + (data[i + 2] << 8)));
-		i += 3;
+		block_end = (i + PCI_VPD_LRDT_TAG_SIZE +
+			    pci_vpd_lrdt_size(&data[i]));
+		i += PCI_VPD_LRDT_TAG_SIZE;
 
 		if (block_end > BNX2_VPD_LEN)
 			goto vpd_done;
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 7f82b02..d01e133 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12483,19 +12483,18 @@ static void __devinit tg3_read_partno(struct tg3 *tp)
 		unsigned int block_end;
 
 		if (val == 0x82 || val == 0x91) {
-			i = (i + 3 +
-			     (vpd_data[i + 1] +
-			      (vpd_data[i + 2] << 8)));
+			i += PCI_VPD_LRDT_TAG_SIZE +
+			     pci_vpd_lrdt_size(&vpd_data[i]);
 			continue;
 		}
 
 		if (val != 0x90)
 			goto out_not_found;
 
-		block_end = (i + 3 +
-			     (vpd_data[i + 1] +
-			      (vpd_data[i + 2] << 8)));
-		i += 3;
+		block_end = i + PCI_VPD_LRDT_TAG_SIZE +
+			    pci_vpd_lrdt_size(&vpd_data[i]);
+
+		i += PCI_VPD_LRDT_TAG_SIZE;
 
 		if (block_end > TG3_NVM_VPD_LEN)
 			goto out_not_found;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c1968f4..038f3c0 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1338,5 +1338,19 @@ static inline bool pci_is_pcie(struct pci_dev *dev)
 
 void pci_request_acs(void);
 
+
+#define PCI_VPD_LRDT_TAG_SIZE	3
+
+/**
+ * pci_vpd_lrdt_size - Extracts the Large Resource Data Type length
+ * @lrdt: Pointer to the beginning of the Large Resource Data Type tag
+ *
+ * Returns the extracted Large Resource Data Type length.
+ */
+static inline u16 pci_vpd_lrdt_size(const u8 *lrdt)
+{
+	return (u16)lrdt[1] + ((u16)lrdt[2] << 8);
+}
+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */
-- 
1.6.4.4

^ permalink raw reply related


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