diff mbox series

[v1] HID: i2c-hid: use print_hex_dump_debug to print report descriptor

Message ID TYCP286MB260706B19C5E30EE2774784EB129A@TYCP286MB2607.JPNP286.PROD.OUTLOOK.COM (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series [v1] HID: i2c-hid: use print_hex_dump_debug to print report descriptor | expand

Commit Message

Riwen Lu July 3, 2023, 6:01 a.m. UTC
From: Riwen Lu <luriwen@kylinos.cn>

The format '%*ph' print up to 64 bytes long as a hex string with ' '
sepatator. Usually the size of report descriptor is larger than 64
bytes, so consider using print_hex_dump_debug to print out all of it for
better debugging.

Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jiri Kosina Aug. 14, 2023, 9:26 a.m. UTC | #1
On Mon, 3 Jul 2023, Riwen Lu wrote:

> From: Riwen Lu <luriwen@kylinos.cn>
> 
> The format '%*ph' print up to 64 bytes long as a hex string with ' '
> sepatator. Usually the size of report descriptor is larger than 64
> bytes, so consider using print_hex_dump_debug to print out all of it for
> better debugging.
> 
> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index efbba0465eef..8e97fc01c852 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -772,7 +772,9 @@ static int i2c_hid_parse(struct hid_device *hid)
>  		}
>  	}
>  
> -	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
> +	i2c_hid_dbg(ihid, "Report Descriptor\n");
> +	print_hex_dump_debug("  ", DUMP_PREFIX_OFFSET, 16, 1,
> +			rdesc, rsize, false);

But that would dump it unconditionally, while i2c_hid_dbg() is 
conditional.
Riwen Lu Aug. 15, 2023, 6:02 a.m. UTC | #2
在 2023/8/14 17:26, Jiri Kosina 写道:
> On Mon, 3 Jul 2023, Riwen Lu wrote:
> 
>> From: Riwen Lu <luriwen@kylinos.cn>
>>
>> The format '%*ph' print up to 64 bytes long as a hex string with ' '
>> sepatator. Usually the size of report descriptor is larger than 64
>> bytes, so consider using print_hex_dump_debug to print out all of it for
>> better debugging.
>>
>> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
>> ---
>>   drivers/hid/i2c-hid/i2c-hid-core.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>> index efbba0465eef..8e97fc01c852 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>> @@ -772,7 +772,9 @@ static int i2c_hid_parse(struct hid_device *hid)
>>   		}
>>   	}
>>   
>> -	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
>> +	i2c_hid_dbg(ihid, "Report Descriptor\n");
>> +	print_hex_dump_debug("  ", DUMP_PREFIX_OFFSET, 16, 1,
>> +			rdesc, rsize, false);
> 
> But that would dump it unconditionally, while i2c_hid_dbg() is
> conditional.
> 
Function print_hex_dump_debug() dump messages is as conditional as 
i2c_hid_dbg().

The function i2c_hid_dbg() defines as follows:
#define i2c_hid_dbg(ihid, ...) dev_dbg(&(ihid)->client->dev, __VA_ARGS__)

dev_dbg() depends on the same macro as print_hex_dump_debug().
Rahul Rameshbabu Aug. 15, 2023, 6:35 a.m. UTC | #3
On Tue, 15 Aug, 2023 14:02:40 +0800 "Riwen Lu" <luriwen@hotmail.com> wrote:
> 在 2023/8/14 17:26, Jiri Kosina 写道:
>> On Mon, 3 Jul 2023, Riwen Lu wrote:
>>
>>> From: Riwen Lu <luriwen@kylinos.cn>
>>>
>>> The format '%*ph' print up to 64 bytes long as a hex string with ' '
>>> sepatator. Usually the size of report descriptor is larger than 64
>>> bytes, so consider using print_hex_dump_debug to print out all of it for
>>> better debugging.
>>>
>>> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
>>> ---
>>>   drivers/hid/i2c-hid/i2c-hid-core.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>>> index efbba0465eef..8e97fc01c852 100644
>>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>>> @@ -772,7 +772,9 @@ static int i2c_hid_parse(struct hid_device *hid)
>>>   		}
>>>   	}
>>>
>>> -	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
>>> +	i2c_hid_dbg(ihid, "Report Descriptor\n");
>>> +	print_hex_dump_debug("  ", DUMP_PREFIX_OFFSET, 16, 1,
>>> +			rdesc, rsize, false);

Maybe it makes sense to use a prefix for the hex dump that is easy to
trace rather than padding whitespace? This looks good when you do not
see any other kernel message log lines get interlaced when written.
However, if you have a lot of concurrent kernel message output, I think
it can be tough to piece together the lines of the dump with this
prefix. Just my opinion.

>>
>> But that would dump it unconditionally, while i2c_hid_dbg() is
>> conditional.
>>
> Function print_hex_dump_debug() dump messages is as conditional as
> i2c_hid_dbg().
>
> The function i2c_hid_dbg() defines as follows:
> #define i2c_hid_dbg(ihid, ...) dev_dbg(&(ihid)->client->dev, __VA_ARGS__)
>
> dev_dbg() depends on the same macro as print_hex_dump_debug().

I agree with this point. Both dev_dbg and print_hex_dump_debug are noops
if neither CONFIG_DYNAMIC_DEBUG is set or DEBUG is defined.

--
Thanks,

Rahul Rameshbabu
Riwen Lu Aug. 15, 2023, 6:58 a.m. UTC | #4
在 2023/8/15 14:35, Rahul Rameshbabu 写道:
> 
> On Tue, 15 Aug, 2023 14:02:40 +0800 "Riwen Lu" <luriwen@hotmail.com> wrote:
>> 在 2023/8/14 17:26, Jiri Kosina 写道:
>>> On Mon, 3 Jul 2023, Riwen Lu wrote:
>>>
>>>> From: Riwen Lu <luriwen@kylinos.cn>
>>>>
>>>> The format '%*ph' print up to 64 bytes long as a hex string with ' '
>>>> sepatator. Usually the size of report descriptor is larger than 64
>>>> bytes, so consider using print_hex_dump_debug to print out all of it for
>>>> better debugging.
>>>>
>>>> Signed-off-by: Riwen Lu <luriwen@kylinos.cn>
>>>> ---
>>>>    drivers/hid/i2c-hid/i2c-hid-core.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>>>> index efbba0465eef..8e97fc01c852 100644
>>>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>>>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>>>> @@ -772,7 +772,9 @@ static int i2c_hid_parse(struct hid_device *hid)
>>>>    		}
>>>>    	}
>>>>
>>>> -	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
>>>> +	i2c_hid_dbg(ihid, "Report Descriptor\n");
>>>> +	print_hex_dump_debug("  ", DUMP_PREFIX_OFFSET, 16, 1,
>>>> +			rdesc, rsize, false);
> 
> Maybe it makes sense to use a prefix for the hex dump that is easy to
> trace rather than padding whitespace? This looks good when you do not
> see any other kernel message log lines get interlaced when written.
> However, if you have a lot of concurrent kernel message output, I think
> it can be tough to piece together the lines of the dump with this
> prefix. Just my opinion.
> 
That's a good opinion. I'll make a v2 version.

Thanks.
>>>
>>> But that would dump it unconditionally, while i2c_hid_dbg() is
>>> conditional.
>>>
>> Function print_hex_dump_debug() dump messages is as conditional as
>> i2c_hid_dbg().
>>
>> The function i2c_hid_dbg() defines as follows:
>> #define i2c_hid_dbg(ihid, ...) dev_dbg(&(ihid)->client->dev, __VA_ARGS__)
>>
>> dev_dbg() depends on the same macro as print_hex_dump_debug().
> 
> I agree with this point. Both dev_dbg and print_hex_dump_debug are noops
> if neither CONFIG_DYNAMIC_DEBUG is set or DEBUG is defined.
> 
> --
> Thanks,
> 
> Rahul Rameshbabu
>
diff mbox series

Patch

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index efbba0465eef..8e97fc01c852 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -772,7 +772,9 @@  static int i2c_hid_parse(struct hid_device *hid)
 		}
 	}
 
-	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
+	i2c_hid_dbg(ihid, "Report Descriptor\n");
+	print_hex_dump_debug("  ", DUMP_PREFIX_OFFSET, 16, 1,
+			rdesc, rsize, false);
 
 	ret = hid_parse_report(hid, rdesc, rsize);
 	if (!use_override)