* [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
@ 2012-10-04 9:16 Andrei Emeltchenko
2012-10-04 16:32 ` Joe Perches
0 siblings, 1 reply; 12+ messages in thread
From: Andrei Emeltchenko @ 2012-10-04 9:16 UTC (permalink / raw)
To: linux-wireless; +Cc: bzhao
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Make output more readable and remove unneeded function call.
...
mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
last_cmd_id: 00000000: 28 00 28 00 28 (.(.(
...
would be changed to:
...
mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
mwifiex_sdio mmc0:0001:1: last_cmd_id: 28 00 28 00 28
...
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
drivers/net/wireless/mwifiex/cmdevt.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
index 225c1a4..e6a0600 100644
--- a/drivers/net/wireless/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/mwifiex/cmdevt.c
@@ -917,21 +917,20 @@ mwifiex_cmd_timeout_func(unsigned long function_context)
dev_err(adapter->dev, "last_cmd_index = %d\n",
adapter->dbg.last_cmd_index);
- print_hex_dump_bytes("last_cmd_id: ", DUMP_PREFIX_OFFSET,
- adapter->dbg.last_cmd_id, DBG_CMD_NUM);
- print_hex_dump_bytes("last_cmd_act: ", DUMP_PREFIX_OFFSET,
- adapter->dbg.last_cmd_act, DBG_CMD_NUM);
+ dev_err(adapter->dev, "last_cmd_id: %*ph\n", DBG_CMD_NUM,
+ adapter->dbg.last_cmd_id);
+ dev_err(adapter->dev, "last_cmd_act: %*ph\n", DBG_CMD_NUM,
+ adapter->dbg.last_cmd_act);
dev_err(adapter->dev, "last_cmd_resp_index = %d\n",
adapter->dbg.last_cmd_resp_index);
- print_hex_dump_bytes("last_cmd_resp_id: ", DUMP_PREFIX_OFFSET,
- adapter->dbg.last_cmd_resp_id,
- DBG_CMD_NUM);
+ dev_err(adapter->dev, "last_cmd_resp_id: %*ph\n", DBG_CMD_NUM,
+ adapter->dbg.last_cmd_resp_id);
dev_err(adapter->dev, "last_event_index = %d\n",
adapter->dbg.last_event_index);
- print_hex_dump_bytes("last_event: ", DUMP_PREFIX_OFFSET,
- adapter->dbg.last_event, DBG_CMD_NUM);
+ dev_err(adapter->dev, "last_event: %*ph\n", DBG_CMD_NUM,
+ adapter->dbg.last_event);
dev_err(adapter->dev, "data_sent=%d cmd_sent=%d\n",
adapter->data_sent, adapter->cmd_sent);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-04 9:16 [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes Andrei Emeltchenko
@ 2012-10-04 16:32 ` Joe Perches
2012-10-05 6:53 ` Andy Shevchenko
0 siblings, 1 reply; 12+ messages in thread
From: Joe Perches @ 2012-10-04 16:32 UTC (permalink / raw)
To: Andrei Emeltchenko, Andy Shevchenko; +Cc: linux-wireless, bzhao
On Thu, 2012-10-04 at 12:16 +0300, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Make output more readable and remove unneeded function call.
>
> ...
> mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
> last_cmd_id: 00000000: 28 00 28 00 28 (.(.(
> ...
>
> would be changed to:
>
> ...
> mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
> mwifiex_sdio mmc0:0001:1: last_cmd_id: 28 00 28 00 28
> ...
(added Andy Shevchenko)
Hi Andrei. That's probably a better output style too
as the ascii isn't likely useful.
One non-nit as a style issue for Andy Shevchenko:
> diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
[]
> @@ -917,21 +917,20 @@ mwifiex_cmd_timeout_func(unsigned long function_context)
[]
> + dev_err(adapter->dev, "last_cmd_id: %*ph\n", DBG_CMD_NUM,
> + adapter->dbg.last_cmd_id);
> + dev_err(adapter->dev, "last_cmd_act: %*ph\n", DBG_CMD_NUM,
> + adapter->dbg.last_cmd_act);
When the number of bytes is fixed, it might be nicer
to use a format like "%<num>ph" so that the function
argument stack doesn't have the width pushed but it's
fixed in the format.
This case would definitely not work well though as it's
a #define rather than a simple number and I think
using "%" stringize(some_define) "ph" is not nice.
Maybe this case argues more for always using "%*ph".
At least that's easily greppable.
Andy?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-04 16:32 ` Joe Perches
@ 2012-10-05 6:53 ` Andy Shevchenko
2012-10-05 7:00 ` Andy Shevchenko
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Andy Shevchenko @ 2012-10-05 6:53 UTC (permalink / raw)
To: Joe Perches; +Cc: Andrei Emeltchenko, linux-wireless, bzhao
On Thu, 2012-10-04 at 09:32 -0700, Joe Perches wrote:
> On Thu, 2012-10-04 at 12:16 +0300, Andrei Emeltchenko wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > Make output more readable and remove unneeded function call.
> >
> > ...
> > mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
> > last_cmd_id: 00000000: 28 00 28 00 28 (.(.(
> > ...
> >
> > would be changed to:
> >
> > ...
> > mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
> > mwifiex_sdio mmc0:0001:1: last_cmd_id: 28 00 28 00 28
> > ...
>
> (added Andy Shevchenko)
>
> Hi Andrei. That's probably a better output style too
> as the ascii isn't likely useful.
>
> One non-nit as a style issue for Andy Shevchenko:
>
> > diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
> []
> > @@ -917,21 +917,20 @@ mwifiex_cmd_timeout_func(unsigned long function_context)
> []
> > + dev_err(adapter->dev, "last_cmd_id: %*ph\n", DBG_CMD_NUM,
> > + adapter->dbg.last_cmd_id);
> > + dev_err(adapter->dev, "last_cmd_act: %*ph\n", DBG_CMD_NUM,
> > + adapter->dbg.last_cmd_act);
>
> When the number of bytes is fixed, it might be nicer
> to use a format like "%<num>ph" so that the function
> argument stack doesn't have the width pushed but it's
> fixed in the format.
>
> This case would definitely not work well though as it's
> a #define rather than a simple number and I think
> using "%" stringize(some_define) "ph" is not nice.
>
> Maybe this case argues more for always using "%*ph".
> At least that's easily greppable.
>
> Andy?
First of all, the current linux-next has at least two definitions of
that constant:
drivers/net/wireless/mwifiex/ioctl.h:175:#define DBG_CMD_NUM 5
drivers/net/wireless/mwifiex/main.h:118:#define DBG_CMD_NUM 5
I checked briefly the code and found that the constant is heavily used
as a definition for the length of static byte arrays. So, in that case
probably better to use %*ph, but apply sizeof(cool_var) instead of
putting constant there.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-05 6:53 ` Andy Shevchenko
@ 2012-10-05 7:00 ` Andy Shevchenko
2012-10-05 10:07 ` Andrei Emeltchenko
2012-10-05 19:43 ` Bing Zhao
2012-10-05 10:11 ` Andrei Emeltchenko
2012-10-05 19:35 ` Bing Zhao
2 siblings, 2 replies; 12+ messages in thread
From: Andy Shevchenko @ 2012-10-05 7:00 UTC (permalink / raw)
To: Joe Perches; +Cc: Andrei Emeltchenko, linux-wireless, bzhao
On Fri, 2012-10-05 at 09:53 +0300, Andy Shevchenko wrote:
> On Thu, 2012-10-04 at 09:32 -0700, Joe Perches wrote:
> > On Thu, 2012-10-04 at 12:16 +0300, Andrei Emeltchenko wrote:
> > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > >
> > > Make output more readable and remove unneeded function call.
> > >
> > > ...
> > > mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
> > > last_cmd_id: 00000000: 28 00 28 00 28 (.(.(
> > > ...
> > >
> > > would be changed to:
> > >
> > > ...
> > > mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
> > > mwifiex_sdio mmc0:0001:1: last_cmd_id: 28 00 28 00 28
> > > ...
> >
> > (added Andy Shevchenko)
> >
> > Hi Andrei. That's probably a better output style too
> > as the ascii isn't likely useful.
> >
> > One non-nit as a style issue for Andy Shevchenko:
> >
> > > diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
> > []
> > > @@ -917,21 +917,20 @@ mwifiex_cmd_timeout_func(unsigned long function_context)
> > []
> > > + dev_err(adapter->dev, "last_cmd_id: %*ph\n", DBG_CMD_NUM,
> > > + adapter->dbg.last_cmd_id);
> > > + dev_err(adapter->dev, "last_cmd_act: %*ph\n", DBG_CMD_NUM,
> > > + adapter->dbg.last_cmd_act);
> >
> > When the number of bytes is fixed, it might be nicer
> > to use a format like "%<num>ph" so that the function
> > argument stack doesn't have the width pushed but it's
> > fixed in the format.
> >
> > This case would definitely not work well though as it's
> > a #define rather than a simple number and I think
> > using "%" stringize(some_define) "ph" is not nice.
> >
> > Maybe this case argues more for always using "%*ph".
> > At least that's easily greppable.
> >
> > Andy?
> First of all, the current linux-next has at least two definitions of
> that constant:
> drivers/net/wireless/mwifiex/ioctl.h:175:#define DBG_CMD_NUM 5
> drivers/net/wireless/mwifiex/main.h:118:#define DBG_CMD_NUM 5
>
> I checked briefly the code and found that the constant is heavily used
> as a definition for the length of static byte arrays. So, in that case
> probably better to use %*ph, but apply sizeof(cool_var) instead of
(int)sizeof(cool_var) of course
> putting constant there.
And one finding more: buffers are defined as u16. I'm afraid the both
previous and proposed versions are printing something interesting, like
only half of the defined data.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-05 7:00 ` Andy Shevchenko
@ 2012-10-05 10:07 ` Andrei Emeltchenko
2012-10-05 10:19 ` Andy Shevchenko
2012-10-05 19:43 ` Bing Zhao
1 sibling, 1 reply; 12+ messages in thread
From: Andrei Emeltchenko @ 2012-10-05 10:07 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Joe Perches, linux-wireless, bzhao
Hi Andy,
On Fri, Oct 05, 2012 at 10:00:28AM +0300, Andy Shevchenko wrote:
> On Fri, 2012-10-05 at 09:53 +0300, Andy Shevchenko wrote:
> > On Thu, 2012-10-04 at 09:32 -0700, Joe Perches wrote:
> > > On Thu, 2012-10-04 at 12:16 +0300, Andrei Emeltchenko wrote:
> > > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > >
> > > > Make output more readable and remove unneeded function call.
> > > >
> > > > ...
> > > > mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
> > > > last_cmd_id: 00000000: 28 00 28 00 28 (.(.(
> > > > ...
> > > >
> > > > would be changed to:
> > > >
> > > > ...
> > > > mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
> > > > mwifiex_sdio mmc0:0001:1: last_cmd_id: 28 00 28 00 28
> > > > ...
> > >
> > > (added Andy Shevchenko)
> > >
> > > Hi Andrei. That's probably a better output style too
> > > as the ascii isn't likely useful.
> > >
> > > One non-nit as a style issue for Andy Shevchenko:
> > >
> > > > diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
> > > []
> > > > @@ -917,21 +917,20 @@ mwifiex_cmd_timeout_func(unsigned long function_context)
> > > []
> > > > + dev_err(adapter->dev, "last_cmd_id: %*ph\n", DBG_CMD_NUM,
> > > > + adapter->dbg.last_cmd_id);
> > > > + dev_err(adapter->dev, "last_cmd_act: %*ph\n", DBG_CMD_NUM,
> > > > + adapter->dbg.last_cmd_act);
> > >
> > > When the number of bytes is fixed, it might be nicer
> > > to use a format like "%<num>ph" so that the function
> > > argument stack doesn't have the width pushed but it's
> > > fixed in the format.
> > >
> > > This case would definitely not work well though as it's
> > > a #define rather than a simple number and I think
> > > using "%" stringize(some_define) "ph" is not nice.
> > >
> > > Maybe this case argues more for always using "%*ph".
> > > At least that's easily greppable.
> > >
> > > Andy?
> > First of all, the current linux-next has at least two definitions of
> > that constant:
> > drivers/net/wireless/mwifiex/ioctl.h:175:#define DBG_CMD_NUM 5
> > drivers/net/wireless/mwifiex/main.h:118:#define DBG_CMD_NUM 5
> >
> > I checked briefly the code and found that the constant is heavily used
> > as a definition for the length of static byte arrays. So, in that case
> > probably better to use %*ph, but apply sizeof(cool_var) instead of
> (int)sizeof(cool_var) of course
>
> > putting constant there.
>
> And one finding more: buffers are defined as u16. I'm afraid the both
> previous and proposed versions are printing something interesting, like
> only half of the defined data.
The patch only changes printing format. The other logical change would
come better in other patch, maybe Bing could comment here.
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-05 10:07 ` Andrei Emeltchenko
@ 2012-10-05 10:19 ` Andy Shevchenko
2012-10-05 12:58 ` Andrei Emeltchenko
0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2012-10-05 10:19 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: Joe Perches, linux-wireless, bzhao
On Fri, 2012-10-05 at 13:07 +0300, Andrei Emeltchenko wrote:
> On Fri, Oct 05, 2012 at 10:00:28AM +0300, Andy Shevchenko wrote:
> > And one finding more: buffers are defined as u16. I'm afraid the both
> > previous and proposed versions are printing something interesting, like
> > only half of the defined data.
>
> The patch only changes printing format. The other logical change would
> come better in other patch, maybe Bing could comment here.
What about to fix logic first and then to substitute the
print_hex_dump_bytes() calls?
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-05 10:19 ` Andy Shevchenko
@ 2012-10-05 12:58 ` Andrei Emeltchenko
0 siblings, 0 replies; 12+ messages in thread
From: Andrei Emeltchenko @ 2012-10-05 12:58 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Joe Perches, linux-wireless, bzhao
On Fri, Oct 05, 2012 at 01:19:58PM +0300, Andy Shevchenko wrote:
> On Fri, 2012-10-05 at 13:07 +0300, Andrei Emeltchenko wrote:
> > On Fri, Oct 05, 2012 at 10:00:28AM +0300, Andy Shevchenko wrote:
>
> > > And one finding more: buffers are defined as u16. I'm afraid the both
Looking at the output:
mwifiex_sdio mmc0:0001:1: last_cmd_id: 28 00 28 00 28
I suspect that they meant u8.
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-05 7:00 ` Andy Shevchenko
2012-10-05 10:07 ` Andrei Emeltchenko
@ 2012-10-05 19:43 ` Bing Zhao
1 sibling, 0 replies; 12+ messages in thread
From: Bing Zhao @ 2012-10-05 19:43 UTC (permalink / raw)
To: Andy Shevchenko, Joe Perches
Cc: Andrei Emeltchenko, linux-wireless@vger.kernel.org
SGkgQW5keSwNCg0KPiA+IEkgY2hlY2tlZCBicmllZmx5IHRoZSBjb2RlIGFuZCBmb3VuZCB0aGF0
IHRoZSBjb25zdGFudCBpcyBoZWF2aWx5IHVzZWQNCj4gPiBhcyBhIGRlZmluaXRpb24gZm9yIHRo
ZSBsZW5ndGggb2Ygc3RhdGljIGJ5dGUgYXJyYXlzLiBTbywgaW4gdGhhdCBjYXNlDQo+ID4gcHJv
YmFibHkgYmV0dGVyIHRvIHVzZSAlKnBoLCBidXQgYXBwbHkgc2l6ZW9mKGNvb2xfdmFyKSBpbnN0
ZWFkIG9mDQo+IChpbnQpc2l6ZW9mKGNvb2xfdmFyKSBvZiBjb3Vyc2UNCj4gDQo+ID4gcHV0dGlu
ZyBjb25zdGFudCB0aGVyZS4NCj4gDQo+IEFuZCBvbmUgZmluZGluZyBtb3JlOiBidWZmZXJzIGFy
ZSBkZWZpbmVkIGFzIHUxNi4gSSdtIGFmcmFpZCB0aGUgYm90aA0KPiBwcmV2aW91cyBhbmQgcHJv
cG9zZWQgdmVyc2lvbnMgYXJlIHByaW50aW5nIHNvbWV0aGluZyBpbnRlcmVzdGluZywgbGlrZQ0K
PiBvbmx5IGhhbGYgb2YgdGhlIGRlZmluZWQgZGF0YS4NCg0KWW91IGFyZSByaWdodC4gVGhlIG9y
aWdpbmFsIGNvZGUgc2hvdWxkIGJlIGxpa2UgdGhpczoNCg0KICAgICAgICAgICAgICAgIHByaW50
X2hleF9kdW1wX2J5dGVzKCJsYXN0X2NtZF9pZDogIiwgRFVNUF9QUkVGSVhfT0ZGU0VULA0KLSAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGFkYXB0ZXItPmRiZy5sYXN0X2NtZF9p
ZCwgREJHX0NNRF9OVU0pOw0KKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGFk
YXB0ZXItPmRiZy5sYXN0X2NtZF9pZCwNCisgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICBzaXplb2YoYWRhcHRlci0+ZGJnLmxhc3RfY21kX2lkKSk7DQoNCmFuZCB0aGVuIGluIEFu
ZHJlaSdzIHBhdGNoOg0KDQoJCWRldl9lcnIoYWRhcHRlci0+ZGV2LCAibGFzdF9jbWRfaWQ6ICUq
cGhcbiIsDQoJCQkoaW50KXNpemVvZihhZGFwdGVyLT5kYmcubGFzdF9jbWRfaWQpKSwNCgkJCWFk
YXB0ZXItPmRiZy5sYXN0X2NtZF9pZCk7DQoNCkkgd2lsbCBzZW5kIGEgcGF0Y2ggdG8gZml4IHRo
ZSBvcmlnaW5hbCBjb2RlLg0KDQpUaGFua3MsDQpCaW5nDQoNCg==
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-05 6:53 ` Andy Shevchenko
2012-10-05 7:00 ` Andy Shevchenko
@ 2012-10-05 10:11 ` Andrei Emeltchenko
2012-10-05 10:18 ` Andy Shevchenko
2012-10-05 19:35 ` Bing Zhao
2 siblings, 1 reply; 12+ messages in thread
From: Andrei Emeltchenko @ 2012-10-05 10:11 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Joe Perches, linux-wireless, bzhao
Hi Andy,
On Fri, Oct 05, 2012 at 09:53:16AM +0300, Andy Shevchenko wrote:
> On Thu, 2012-10-04 at 09:32 -0700, Joe Perches wrote:
> > On Thu, 2012-10-04 at 12:16 +0300, Andrei Emeltchenko wrote:
> > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > >
> > > Make output more readable and remove unneeded function call.
> > >
> > > ...
> > > mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
> > > last_cmd_id: 00000000: 28 00 28 00 28 (.(.(
> > > ...
> > >
> > > would be changed to:
> > >
> > > ...
> > > mwifiex_sdio mmc0:0001:1: last_cmd_index = 3
> > > mwifiex_sdio mmc0:0001:1: last_cmd_id: 28 00 28 00 28
> > > ...
> >
> > (added Andy Shevchenko)
> >
> > Hi Andrei. That's probably a better output style too
> > as the ascii isn't likely useful.
> >
> > One non-nit as a style issue for Andy Shevchenko:
> >
> > > diff --git a/drivers/net/wireless/mwifiex/cmdevt.c b/drivers/net/wireless/mwifiex/cmdevt.c
> > []
> > > @@ -917,21 +917,20 @@ mwifiex_cmd_timeout_func(unsigned long function_context)
> > []
> > > + dev_err(adapter->dev, "last_cmd_id: %*ph\n", DBG_CMD_NUM,
> > > + adapter->dbg.last_cmd_id);
> > > + dev_err(adapter->dev, "last_cmd_act: %*ph\n", DBG_CMD_NUM,
> > > + adapter->dbg.last_cmd_act);
> >
> > When the number of bytes is fixed, it might be nicer
> > to use a format like "%<num>ph" so that the function
> > argument stack doesn't have the width pushed but it's
> > fixed in the format.
> >
> > This case would definitely not work well though as it's
> > a #define rather than a simple number and I think
> > using "%" stringize(some_define) "ph" is not nice.
> >
> > Maybe this case argues more for always using "%*ph".
> > At least that's easily greppable.
> >
> > Andy?
> First of all, the current linux-next has at least two definitions of
> that constant:
> drivers/net/wireless/mwifiex/ioctl.h:175:#define DBG_CMD_NUM 5
> drivers/net/wireless/mwifiex/main.h:118:#define DBG_CMD_NUM 5
Please send patch fixing this. This is unrelated to the patch in
question.
> I checked briefly the code and found that the constant is heavily used
> as a definition for the length of static byte arrays. So, in that case
> probably better to use %*ph, but apply sizeof(cool_var) instead of
> putting constant there.
I did not touch logic that print buffer, the buffer might have lots of
data (even significantly more then you may want to print to logs) so
using sizeof is not the best idea in these cases.
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-05 10:11 ` Andrei Emeltchenko
@ 2012-10-05 10:18 ` Andy Shevchenko
2012-10-05 10:27 ` Andrei Emeltchenko
0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2012-10-05 10:18 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: Joe Perches, linux-wireless, bzhao
On Fri, 2012-10-05 at 13:11 +0300, Andrei Emeltchenko wrote:
> On Fri, Oct 05, 2012 at 09:53:16AM +0300, Andy Shevchenko wrote:
> > On Thu, 2012-10-04 at 09:32 -0700, Joe Perches wrote:
> > > When the number of bytes is fixed, it might be nicer
> > > to use a format like "%<num>ph" so that the function
> > > argument stack doesn't have the width pushed but it's
> > > fixed in the format.
> > >
> > > This case would definitely not work well though as it's
> > > a #define rather than a simple number and I think
> > > using "%" stringize(some_define) "ph" is not nice.
> > >
> > > Maybe this case argues more for always using "%*ph".
> > > At least that's easily greppable.
> > I checked briefly the code and found that the constant is heavily used
> > as a definition for the length of static byte arrays. So, in that case
> > probably better to use %*ph, but apply sizeof(cool_var) instead of
> > putting constant there.
>
> I did not touch logic that print buffer, the buffer might have lots of
> data (even significantly more then you may want to print to logs) so
> using sizeof is not the best idea in these cases.
Then you probably need to decide how much you would like to print
exactly and put that constant directly to the %*ph (instead of
asterisk), like %5ph. Otherwise it's unclear what DBG_CMD_NUM stands
for.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-05 10:18 ` Andy Shevchenko
@ 2012-10-05 10:27 ` Andrei Emeltchenko
0 siblings, 0 replies; 12+ messages in thread
From: Andrei Emeltchenko @ 2012-10-05 10:27 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Joe Perches, linux-wireless, bzhao
Hi Andy,
On Fri, Oct 05, 2012 at 01:18:25PM +0300, Andy Shevchenko wrote:
> On Fri, 2012-10-05 at 13:11 +0300, Andrei Emeltchenko wrote:
> > On Fri, Oct 05, 2012 at 09:53:16AM +0300, Andy Shevchenko wrote:
> > > On Thu, 2012-10-04 at 09:32 -0700, Joe Perches wrote:
>
> > > > When the number of bytes is fixed, it might be nicer
> > > > to use a format like "%<num>ph" so that the function
> > > > argument stack doesn't have the width pushed but it's
> > > > fixed in the format.
> > > >
> > > > This case would definitely not work well though as it's
> > > > a #define rather than a simple number and I think
> > > > using "%" stringize(some_define) "ph" is not nice.
> > > >
> > > > Maybe this case argues more for always using "%*ph".
> > > > At least that's easily greppable.
>
>
> > > I checked briefly the code and found that the constant is heavily used
> > > as a definition for the length of static byte arrays. So, in that case
> > > probably better to use %*ph, but apply sizeof(cool_var) instead of
> > > putting constant there.
> >
> > I did not touch logic that print buffer, the buffer might have lots of
> > data (even significantly more then you may want to print to logs) so
> > using sizeof is not the best idea in these cases.
> Then you probably need to decide how much you would like to print
> exactly and put that constant directly to the %*ph (instead of
> asterisk), like %5ph. Otherwise it's unclear what DBG_CMD_NUM stands
I do not like much magic numbers. Defines are the same as those magic
numbers. You always see them with one jump and usually you do reuse them
anyway.
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes
2012-10-05 6:53 ` Andy Shevchenko
2012-10-05 7:00 ` Andy Shevchenko
2012-10-05 10:11 ` Andrei Emeltchenko
@ 2012-10-05 19:35 ` Bing Zhao
2 siblings, 0 replies; 12+ messages in thread
From: Bing Zhao @ 2012-10-05 19:35 UTC (permalink / raw)
To: Andy Shevchenko, Joe Perches
Cc: Andrei Emeltchenko, linux-wireless@vger.kernel.org
SGkgQW5keSwNCg0KPiA+IFdoZW4gdGhlIG51bWJlciBvZiBieXRlcyBpcyBmaXhlZCwgaXQgbWln
aHQgYmUgbmljZXINCj4gPiB0byB1c2UgYSBmb3JtYXQgbGlrZSAiJTxudW0+cGgiIHNvIHRoYXQg
dGhlIGZ1bmN0aW9uDQo+ID4gYXJndW1lbnQgc3RhY2sgZG9lc24ndCBoYXZlIHRoZSB3aWR0aCBw
dXNoZWQgYnV0IGl0J3MNCj4gPiBmaXhlZCBpbiB0aGUgZm9ybWF0Lg0KPiA+DQo+ID4gVGhpcyBj
YXNlIHdvdWxkIGRlZmluaXRlbHkgbm90IHdvcmsgd2VsbCB0aG91Z2ggYXMgaXQncw0KPiA+IGEg
I2RlZmluZSByYXRoZXIgdGhhbiBhIHNpbXBsZSBudW1iZXIgYW5kIEkgdGhpbmsNCj4gPiB1c2lu
ZyAiJSIgc3RyaW5naXplKHNvbWVfZGVmaW5lKSAicGgiIGlzIG5vdCBuaWNlLg0KPiA+DQo+ID4g
TWF5YmUgdGhpcyBjYXNlIGFyZ3VlcyBtb3JlIGZvciBhbHdheXMgdXNpbmcgIiUqcGgiLg0KPiA+
IEF0IGxlYXN0IHRoYXQncyBlYXNpbHkgZ3JlcHBhYmxlLg0KPiA+DQo+ID4gQW5keT8NCj4gRmly
c3Qgb2YgYWxsLCB0aGUgY3VycmVudCBsaW51eC1uZXh0IGhhcyBhdCBsZWFzdCB0d28gZGVmaW5p
dGlvbnMgb2YNCj4gdGhhdCBjb25zdGFudDoNCj4gZHJpdmVycy9uZXQvd2lyZWxlc3MvbXdpZmll
eC9pb2N0bC5oOjE3NTojZGVmaW5lIERCR19DTURfTlVNICAgIDUNCj4gZHJpdmVycy9uZXQvd2ly
ZWxlc3MvbXdpZmlleC9tYWluLmg6MTE4OiNkZWZpbmUgREJHX0NNRF9OVU0gNQ0KDQpJIGNhbiBm
aXggdGhhdC4NCg0KPiANCj4gSSBjaGVja2VkIGJyaWVmbHkgdGhlIGNvZGUgYW5kIGZvdW5kIHRo
YXQgdGhlIGNvbnN0YW50IGlzIGhlYXZpbHkgdXNlZA0KPiBhcyBhIGRlZmluaXRpb24gZm9yIHRo
ZSBsZW5ndGggb2Ygc3RhdGljIGJ5dGUgYXJyYXlzLiBTbywgaW4gdGhhdCBjYXNlDQo+IHByb2Jh
Ymx5IGJldHRlciB0byB1c2UgJSpwaCwgYnV0IGFwcGx5IHNpemVvZihjb29sX3ZhcikgaW5zdGVh
ZCBvZg0KPiBwdXR0aW5nIGNvbnN0YW50IHRoZXJlLg0KDQpBZ3JlZS4gKGludClzaXplb2YoY29v
bF92YXIpIHNob3VsZCBiZSB1c2VkIGZvciB0aGUgJSpwaC4NCg0KSSB3aWxsIHNlbmQgYSBwYXRj
aCBmb3IgdGhpcy4NCg0KVGhhbmtzLA0KQmluZw0K
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-10-05 19:43 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-04 9:16 [PATCH] mwifiex: Using %*phD instead of print_hex_dump_bytes Andrei Emeltchenko
2012-10-04 16:32 ` Joe Perches
2012-10-05 6:53 ` Andy Shevchenko
2012-10-05 7:00 ` Andy Shevchenko
2012-10-05 10:07 ` Andrei Emeltchenko
2012-10-05 10:19 ` Andy Shevchenko
2012-10-05 12:58 ` Andrei Emeltchenko
2012-10-05 19:43 ` Bing Zhao
2012-10-05 10:11 ` Andrei Emeltchenko
2012-10-05 10:18 ` Andy Shevchenko
2012-10-05 10:27 ` Andrei Emeltchenko
2012-10-05 19:35 ` Bing Zhao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox