All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH linux-next] debug:kdb: fix unsigned int win compared to less than zero
@ 2021-08-20  2:24 jing yangyang
  2021-08-20 16:46 ` Doug Anderson
  2021-08-21  5:40 ` Christophe JAILLET
  0 siblings, 2 replies; 3+ messages in thread
From: jing yangyang @ 2021-08-20  2:24 UTC (permalink / raw)
  To: Jason Wessel, Daniel Thompson, Douglas Anderson, Sumit Garg,
	Will Deacon, Stephen Zhang, Peter Zijlstra,
	Gustavo A . R . Silva, kgdb-bugreport, linux-kernel
  Cc: jing yangyang, Zeal Robot

Fix coccicheck warning:
./kernel/debug/kdb/kdb_support.c:575:3-10:
WARNING:Unsigned expression compared with zero  p_state < 0

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: jing yangyang <jing.yangyang@zte.com.cn>
---
 kernel/debug/kdb/kdb_support.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index c605b17..fb30801 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -560,7 +560,7 @@ unsigned long kdb_task_state_string(const char *s)
  */
 char kdb_task_state_char (const struct task_struct *p)
 {
-	unsigned int p_state;
+	int p_state;
 	unsigned long tmp;
 	char state;
 	int cpu;
-- 
1.8.3.1



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH linux-next] debug:kdb: fix unsigned int win compared to less than zero
  2021-08-20  2:24 [PATCH linux-next] debug:kdb: fix unsigned int win compared to less than zero jing yangyang
@ 2021-08-20 16:46 ` Doug Anderson
  2021-08-21  5:40 ` Christophe JAILLET
  1 sibling, 0 replies; 3+ messages in thread
From: Doug Anderson @ 2021-08-20 16:46 UTC (permalink / raw)
  To: jing yangyang
  Cc: Jason Wessel, Daniel Thompson, Sumit Garg, Will Deacon,
	Stephen Zhang, Peter Zijlstra, Gustavo A . R . Silva,
	kgdb-bugreport, LKML, jing yangyang, Zeal Robot

Hi,

On Thu, Aug 19, 2021 at 7:25 PM jing yangyang <cgel.zte@gmail.com> wrote:
>
> Fix coccicheck warning:
> ./kernel/debug/kdb/kdb_support.c:575:3-10:
> WARNING:Unsigned expression compared with zero  p_state < 0
>
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: jing yangyang <jing.yangyang@zte.com.cn>
> ---
>  kernel/debug/kdb/kdb_support.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
> index c605b17..fb30801 100644
> --- a/kernel/debug/kdb/kdb_support.c
> +++ b/kernel/debug/kdb/kdb_support.c
> @@ -560,7 +560,7 @@ unsigned long kdb_task_state_string(const char *s)
>   */
>  char kdb_task_state_char (const struct task_struct *p)
>  {
> -       unsigned int p_state;
> +       int p_state;

This was talked about:

https://www.mail-archive.com/kgdb-bugreport@lists.sourceforge.net/msg06159.html

There, Peter Zijlstra said:

> Pre-existing fail that.. but yes that code (and it's carbon copy in
> arch/powerpc/xmon/xmon.c) are clearly bogus and have been for a long
> time afaict.
>
> Ideally someone that cares about this code can replace it with
> get_task_state() or something.

...so while the warning was introduced by commit 2f064a59a11f ("sched:
Change task_struct::state") and your fix papers over of the warning,
it actually doesn't fix the real bug. Apparently the comment
describing the "state" variable before that commit was wrong and "-1"
didn't mean unrunnable.

Maybe you could submit a v2 that does what Peter suggests?

-Doug

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH linux-next] debug:kdb: fix unsigned int win compared to less than zero
  2021-08-20  2:24 [PATCH linux-next] debug:kdb: fix unsigned int win compared to less than zero jing yangyang
  2021-08-20 16:46 ` Doug Anderson
@ 2021-08-21  5:40 ` Christophe JAILLET
  1 sibling, 0 replies; 3+ messages in thread
From: Christophe JAILLET @ 2021-08-21  5:40 UTC (permalink / raw)
  To: jing yangyang, Jason Wessel, Daniel Thompson, Douglas Anderson,
	Sumit Garg, Will Deacon, Stephen Zhang, Peter Zijlstra,
	Gustavo A . R . Silva, kgdb-bugreport, linux-kernel
  Cc: jing yangyang, Zeal Robot

Hi,

Le 20/08/2021 à 04:24, jing yangyang a écrit :
> Fix coccicheck warning:
> ./kernel/debug/kdb/kdb_support.c:575:3-10:
> WARNING:Unsigned expression compared with zero  p_state < 0
> 
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: jing yangyang <jing.yangyang@zte.com.cn>
> ---
>   kernel/debug/kdb/kdb_support.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
> index c605b17..fb30801 100644
> --- a/kernel/debug/kdb/kdb_support.c
> +++ b/kernel/debug/kdb/kdb_support.c
> @@ -560,7 +560,7 @@ unsigned long kdb_task_state_string(const char *s)
>    */
>   char kdb_task_state_char (const struct task_struct *p)
>   {
> -	unsigned int p_state;
> +	int p_state;

When you make changes in variables which are written in the reverse 
Christmas tree style (i.e. long lines at the top, shorter ones below), 
you should keep this style. Many people prefer it that way.

Also, should your fix be correct, it is likely a bugfix, and a "Fixes:" 
would be needed to help backport.


However, I don't think that your patch is correct here.

Unless I missed something, 'p_state' really needs to be an 'unsigned 
int' because 'p->__state' is an 'unsigned int' since 2f064a59a11f 
("sched: Change task_struct::state")

My *guess* is that:
		(p_state < 0) ? 'U' :
should be turned in:
		(p_state & UNRUNNABLE) ? 'U' :

to match the code in 'kdb_task_state_string(()'.

The 'R' case looks also spurious to me, but I've not looked at it deeper.


Should I be right, comment at line 545 ("/* unrunnable is < 0 */") looks 
somewhat misleading or useless. I would drop it.


Finally, you have the same kind of code in 'show_task()' 
(arch/powerpc/xmon/xmon.c). I also guess that whatever the fix it, it 
should be updated the same way here.

CJ

>   	unsigned long tmp;
>   	char state;
>   	int cpu;
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-08-21  5:40 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-20  2:24 [PATCH linux-next] debug:kdb: fix unsigned int win compared to less than zero jing yangyang
2021-08-20 16:46 ` Doug Anderson
2021-08-21  5:40 ` Christophe JAILLET

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.