| Message ID | 1349342161-27008-1-git-send-email-Andrei.Emeltchenko.news@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy, > > 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. You are right. The original code should be like this: print_hex_dump_bytes("last_cmd_id: ", DUMP_PREFIX_OFFSET, - adapter->dbg.last_cmd_id, DBG_CMD_NUM); + adapter->dbg.last_cmd_id, + sizeof(adapter->dbg.last_cmd_id)); and then in Andrei's patch: dev_err(adapter->dev, "last_cmd_id: %*ph\n", (int)sizeof(adapter->dbg.last_cmd_id)), adapter->dbg.last_cmd_id); I will send a patch to fix the original code. Thanks, Bing
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);