| Message ID | f6f32353-bdcc-6dfa-bf52-1b03ebab1eee@dev.mellanox.co.il (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 2017-04-13 at 10:48 -0400, Hal Rosenstock wrote: > %d is not good as LIDs above 32K print as negative numbers. > %#x seems to be the right choice here; %u was used by IB > management tools for unicast LID printing. Hello Hal, Although the patch itself looks fine to me: LIDs are converted from network to host format using be16toh(). That function returns an unsigned 16-bit number. These numbers are converted to type "int" before pr_err() is called. The type "int" is at least 32 bits on all architectures supported by Linux. So how could using %d result in a negative number being printed? > diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c > index 59a6137..4d8476e 100644 > --- a/srp_daemon/srp_daemon.c > +++ b/srp_daemon/srp_daemon.c > @@ -632,7 +632,7 @@ recv: > ret = umad_status(in_mad); > if (ret) { > pr_err( > - "bad MAD status (%u) from lid %d\n", > + "bad MAD status (%u) from lid %#x\n", > ret, (uint16_t) be16toh(out_mad->hdr.addr.lid)); > return -ret; If you have to repost this patch, please remove the (uint16_t) cast since it's superfluous. Thanks, Bart.-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bart, On 4/13/2017 11:29 AM, Bart Van Assche wrote: > On Thu, 2017-04-13 at 10:48 -0400, Hal Rosenstock wrote: >> %d is not good as LIDs above 32K print as negative numbers. >> %#x seems to be the right choice here; %u was used by IB >> management tools for unicast LID printing. > > Hello Hal, > > Although the patch itself looks fine to me: LIDs are converted from network > to host format using be16toh(). That function returns an unsigned 16-bit > number. These numbers are converted to type "int" before pr_err() is called. > The type "int" is at least 32 bits on all architectures supported by Linux. > So how could using %d result in a negative number being printed? I'll fix up the commit description to just say consistent LID printing and remove the negative issue which was only in IB management area. I'll also remove the superfluous cast you mentioned. Thanks. -- Hal >> diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c >> index 59a6137..4d8476e 100644 >> --- a/srp_daemon/srp_daemon.c >> +++ b/srp_daemon/srp_daemon.c >> @@ -632,7 +632,7 @@ recv: >> ret = umad_status(in_mad); >> if (ret) { >> pr_err( >> - "bad MAD status (%u) from lid %d\n", >> + "bad MAD status (%u) from lid %#x\n", >> ret, (uint16_t) be16toh(out_mad->hdr.addr.lid)); >> return -ret; > > If you have to repost this patch, please remove the (uint16_t) cast since it's > superfluous. > > Thanks, > > Bart. > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c index 59a6137..4d8476e 100644 --- a/srp_daemon/srp_daemon.c +++ b/srp_daemon/srp_daemon.c @@ -632,7 +632,7 @@ recv: ret = umad_status(in_mad); if (ret) { pr_err( - "bad MAD status (%u) from lid %d\n", + "bad MAD status (%u) from lid %#x\n", ret, (uint16_t) be16toh(out_mad->hdr.addr.lid)); return -ret; } @@ -935,7 +935,7 @@ static int do_port(struct resources *res, uint16_t pkey, uint16_t dlid, ret = get_iou_info(umad_res, dlid, &iou_info); if (ret < 0) { - pr_err("failed to get iou info for dlid %x\n", dlid); + pr_err("failed to get iou info for dlid %#x\n", dlid); goto out; } @@ -1214,7 +1214,7 @@ static int do_dm_port_list(struct resources *res) num_pkeys = get_shared_pkeys(res, be16toh(port_info->endport_lid), pkeys); if (num_pkeys < 0) { - pr_err("failed to get shared P_Keys with LID %x\n", + pr_err("failed to get shared P_Keys with LID %#x\n", be16toh(port_info->endport_lid)); free(in_mad_buf); return num_pkeys; @@ -1235,7 +1235,7 @@ void handle_port(struct resources *res, uint16_t pkey, uint16_t lid, uint64_t h_ uint64_t subnet_prefix; int isdm; - pr_debug("enter handle_port for lid %d\n", lid); + pr_debug("enter handle_port for lid %#x\n", lid); if (get_port_info(umad_res, lid, &subnet_prefix, &isdm)) return; @@ -1292,7 +1292,7 @@ static int do_full_port_list(struct resources *res) num_pkeys = get_shared_pkeys(res, be16toh(node->lid), pkeys); if (num_pkeys < 0) { - pr_err("failed to get shared P_Keys with LID %x\n", + pr_err("failed to get shared P_Keys with LID %#x\n", be16toh(node->lid)); free(in_mad_buf); return num_pkeys; @@ -2188,7 +2188,7 @@ catas_start: /* unexpected error - do a full rescan */ schedule_rescan(res->sync_res, 0); else { - pr_debug("lid is %d\n", lid); + pr_debug("lid is %#x\n", lid); srp_sleep(0, 100); handle_port(res, pkey, lid, diff --git a/srp_daemon/srp_handle_traps.c b/srp_daemon/srp_handle_traps.c index f219702..6d94634 100644 --- a/srp_daemon/srp_handle_traps.c +++ b/srp_daemon/srp_handle_traps.c @@ -561,9 +561,9 @@ static int register_to_trap(struct sync_resources *sync_res, static uint64_t trans_id = 0x0000FFFF; if (subscribe) - pr_debug("Registering to trap:%d (sm in %d)\n", trap_num, dest_lid); + pr_debug("Registering to trap:%d (sm in %#x)\n", trap_num, dest_lid); else - pr_debug("Deregistering from trap:%d (sm in %d)\n", trap_num, dest_lid); + pr_debug("Deregistering from trap:%d (sm in %#x)\n", trap_num, dest_lid); memset(res->send_buf, 0, SEND_SIZE); diff --git a/srp_daemon/srp_sync.c b/srp_daemon/srp_sync.c index 72c41f0..036fbe5 100644 --- a/srp_daemon/srp_sync.c +++ b/srp_daemon/srp_sync.c @@ -171,7 +171,7 @@ void push_lid_to_list(struct sync_resources *res, uint16_t lid, uint16_t pkey) for (i=0; i < res->next_task; ++i) if (res->tasks[i].lid == lid && res->tasks[i].pkey == pkey) { - pr_debug("lid %d is already in task list\n", lid); + pr_debug("lid %#x is already in task list\n", lid); pthread_mutex_unlock(&res->mutex); return; }
%d is not good as LIDs above 32K print as negative numbers. %#x seems to be the right choice here; %u was used by IB management tools for unicast LID printing. Signed-off-by: Hal Rosenstock <hal@mellanox.com> --- srp_daemon/srp_daemon.c | 12 ++++++------ srp_daemon/srp_handle_traps.c | 4 ++-- srp_daemon/srp_sync.c | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-)