Some NIPQUAD_FMT -> %pI4 conversions are broken

Some NIPQUAD_FMT -> %pI4 conversions are broken

[Roland Dreier]

I just noticed (by getting a messed up print in my kernel log) that most
of the changes to drivers/infiniband/hw/nes in commit 63779436
("drivers: replace NIPQUAD()") were wrong: the idea was to change printk
statements like

	printk("..." NIPQUAD_FMT "...", NIPQUAD(foo));

but most of the changes in nes were to code like

	printk("..." NIPQUAD_FMT "...", HIPQUAD(foo));

ie *H* IPQUAD not *N* IPQUAD, indicating the foo is in host endian
order, and hence the new code

	printk("...%pI4...", &foo);

prints the IP in reverse order now.

I don't see a good way to fix this without introducing a temporary
variable to hold a swapped IP address... but is there a better way?

 - R.

Re: Some NIPQUAD_FMT -> %pI4 conversions are broken

[Harvey Harrison]

On Fri, 2009-01-09 at 19:47 -0800, Roland Dreier wrote:
> I just noticed (by getting a messed up print in my kernel log) that most
> of the changes to drivers/infiniband/hw/nes in commit 63779436
> ("drivers: replace NIPQUAD()") were wrong: the idea was to change printk
> statements like
> 
> 	printk("..." NIPQUAD_FMT "...", NIPQUAD(foo));
> 
> but most of the changes in nes were to code like
> 
> 	printk("..." NIPQUAD_FMT "...", HIPQUAD(foo));
> 
> ie *H* IPQUAD not *N* IPQUAD, indicating the foo is in host endian
> order, and hence the new code
> 
> 	printk("...%pI4...", &foo);
> 
> prints the IP in reverse order now.
> 
> I don't see a good way to fix this without introducing a temporary
> variable to hold a swapped IP address... but is there a better way?
> 

My apologies indeed, the best choice I think is to add a temp var for
now and I'll work with you to eliminate the byteswapping further up the
callchain so a be32 gets passed down, but that might be too involved for 2.6.29.

Sound OK?

Harvey

Re: Some NIPQUAD_FMT -> %pI4 conversions are broken

[David Miller]

From: Harvey Harrison <harvey.harrison@gmail.com>
Date: Fri, 09 Jan 2009 22:07:25 -0800

> My apologies indeed, the best choice I think is to add a temp var
> for now and I'll work with you to eliminate the byteswapping further
> up the callchain so a be32 gets passed down, but that might be too
> involved for 2.6.29.

Alternatively we could provide a %p* variant that took
cpu endian addresses.

I don't know how useful that would be, generally.

Re: Some NIPQUAD_FMT -> %pI4 conversions are broken

[Harvey Harrison]

On Fri, 2009-01-09 at 22:15 -0800, David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Fri, 09 Jan 2009 22:07:25 -0800
> 
> > My apologies indeed, the best choice I think is to add a temp var
> > for now and I'll work with you to eliminate the byteswapping further
> > up the callchain so a be32 gets passed down, but that might be too
> > involved for 2.6.29.
> 
> Alternatively we could provide a %p* variant that took
> cpu endian addresses.
> 
> I don't know how useful that would be, generally.

I don't think we want that, it would be better to just start work on the
nes drivers to work directly on be32s throughout, and eject all of the
byteswapping throughout the driver.

Having such a %p helper for host-endian just encourages more drivers to
be written the same way.

Harvey

[PATCH] nes: fix reversed IP address

[Harvey Harrison]

commit 63779436ab4ad0867bcea53bf853b0004d7b895di (drivers: replace NIPQUAD())
accidentally replaced two HIPQUAD()s causing IP addresses to be
printed in reverse order, add two temporary local vars.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
I'll start working on a patch working directly with _be32s, but this
is the safest for now.

 drivers/infiniband/hw/nes/nes_cm.c    |    3 ++-
 drivers/infiniband/hw/nes/nes_utils.c |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/nes/nes_cm.c b/drivers/infiniband/hw/nes/nes_cm.c
index 6ba57e9..2395b20 100644
--- a/drivers/infiniband/hw/nes/nes_cm.c
+++ b/drivers/infiniband/hw/nes/nes_cm.c
@@ -778,12 +778,13 @@ static struct nes_cm_node *find_node(struct nes_cm_core *cm_core,
 	unsigned long flags;
 	struct list_head *hte;
 	struct nes_cm_node *cm_node;
+	__be32 tmp_addr = cpu_to_be32(loc_addr);
 
 	/* get a handle on the hte */
 	hte = &cm_core->connected_nodes;
 
 	nes_debug(NES_DBG_CM, "Searching for an owner node: %pI4:%x from core %p->%p\n",
-		  &loc_addr, loc_port, cm_core, hte);
+		  &tmp_addr, loc_port, cm_core, hte);
 
 	/* walk list and find cm_node associated with this session ID */
 	spin_lock_irqsave(&cm_core->ht_lock, flags);
diff --git a/drivers/infiniband/hw/nes/nes_utils.c b/drivers/infiniband/hw/nes/nes_utils.c
index aa9b734..feb7401 100644
--- a/drivers/infiniband/hw/nes/nes_utils.c
+++ b/drivers/infiniband/hw/nes/nes_utils.c
@@ -655,6 +655,7 @@ int nes_arp_table(struct nes_device *nesdev, u32 ip_addr, u8 *mac_addr, u32 acti
 	struct nes_adapter *nesadapter = nesdev->nesadapter;
 	int arp_index;
 	int err = 0;
+	__be32 tmp_addr = cpu_to_be32(ip_addr);
 
 	for (arp_index = 0; (u32) arp_index < nesadapter->arp_table_size; arp_index++) {
 		if (nesadapter->arp_table[arp_index].ip_addr == ip_addr)
@@ -683,7 +684,7 @@ int nes_arp_table(struct nes_device *nesdev, u32 ip_addr, u8 *mac_addr, u32 acti
 	/* DELETE or RESOLVE */
 	if (arp_index == nesadapter->arp_table_size) {
 		nes_debug(NES_DBG_NETDEV, "MAC for %pI4 not in ARP table - cannot %s\n",
-			  &ip_addr, action == NES_ARP_RESOLVE ? "resolve" : "delete");
+			  &tmp_addr, action == NES_ARP_RESOLVE ? "resolve" : "delete");
 		return -1;
 	}
 
-- 
1.6.1.94.g9388

Re: [PATCH] nes: fix reversed IP address

[Harvey Harrison]

On Fri, 2009-01-09 at 22:24 -0800, Harvey Harrison wrote:
> commit 63779436ab4ad0867bcea53bf853b0004d7b895di (drivers: replace NIPQUAD())
> accidentally replaced two HIPQUAD()s causing IP addresses to be
> printed in reverse order, add two temporary local vars.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---

Sorry missed a git add after catching the other hunks, will get this
right in the morning.

Harvey