drivers: video: i740fb: add 'default' processing contents for 'switch'.
PATCH 1/1 drivers: video: i740fb: add 'default' processing contents for 'switch'.
Need add related 'default' processing contents for 'switch', or may report 'wm' uninitialized warning.
The related warning:
drivers/video/i740fb.c:662:26: warning: ‘wm’ may be used uninitialized in this function [-Wmaybe-uninitialized]
--- drivers/video/i740fb.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/video/i740fb.c +3 -0
@@ -336,6 +336,9 @@ static u32 i740_calc_fifo(struct i740fb_par *par, u32 freq, int bpp) wm =0x16110000 ; }break ; +default : + wm =0 ; +BUG (); }return wm;
Signature · 1 line
1.7.7.6
Quoted 18 lines · Chen Gang
Need add related 'default' processing contents for 'switch', or may report 'wm' uninitialized warning.
The related warning:
drivers/video/i740fb.c:662:26: warning: ‘wm’ may be used uninitialized in this function [-Wmaybe-uninitialized]Signed-off-by Chen Gang <[email protected]>--- drivers/video/i740fb.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/video/i740fb.c +0
-0
drivers/video/i740fb.c static u32 i740_calc_fifo(struct i740fb_par *par, u32 freq, int bpp) · line 342
Quoted 7 lines
drivers/video/i740fb.c +3
-0@@ -336,6 +336,9 @@ static u32 i740_calc_fifo(struct i740fb_par *par, u32 freq, int bpp) wm =0x16110000 ; }break ; +default : + wm =0 ; +BUG (); }
I don't think you should use BUG there. BUG should be used when there's not really a good way to continue. Here you could have just a WARN, and return some default FIFO watermark value.
Tomi
Quoted 14 lines · Tomi Valkeinen
Quoted 13 lines · Chen Gang
Need add related 'default' processing contents for 'switch', or may report 'wm' uninitialized warning.
The related warning:
drivers/video/i740fb.c:662:26: warning: �wm� may be used uninitialized in this function [-Wmaybe-uninitialized]Signed-off-by Chen Gang <[email protected]>--- drivers/video/i740fb.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
Quoted 17 lines
drivers/video/i740fb.c static u32 i740_calc_fifo(struct i740fb_par *par, u32 freq, int bpp) · line 342
Quoted 12 lines
drivers/video/i740fb.c +3
-0@@ -336,6 +336,9 @@ static u32 i740_calc_fifo(struct i740fb_par *par, u32 freq, int bpp) wm =0x16110000 ; }break ; +default : + wm =0 ; +BUG (); }I don't think you should use BUG there. BUG should be used when there's not really a good way to continue. Here you could have just a WARN, and return some default FIFO watermark value.
i740_calc_fifo() is a static function, so we can check its caller's information to find the suitable fixing ways (for extern function, we almost can not do like this).
it has only one caller i740fb_decode_var(), which has already let 'bpp' within the values (8, 15, 16, 24, 32). So if another values occurs, it must be a BUG (e.g. the stack may override under ia32).
Hmm... it really has quite a few same cases in kernel wide like our case, but at least they do not report warnings (this does not like our case), so let just fix our case to satisfy the compiler. :-)
Quoted 3 lines
Tomi
Thanks.
Signature · 1 line
Chen Gang
Quoted 7 lines · Chen Gang
Quoted 4 lines · Tomi Valkeinen
I don't think you should use BUG there. BUG should be used when there's not really a good way to continue. Here you could have just a WARN, and return some default FIFO watermark value.
Quoted 7 lines
i740_calc_fifo()is a static function, so we can check its caller's information to find the suitable fixing ways (for extern function, we almost can not do like this).it has only one caller
i740fb_decode_var(), which has already let 'bpp' within the values (8, 15, 16, 24, 32). So if another values occurs, it must be a BUG (e.g. the stack may override under ia32).
My point was that there should almost never be need for BUG in a normal driver. BUG means that the whole kernel will probably halt. Even if an fb driver encounters a problem that should never happen, it should maybe give a WARN, and continue or fail in a controlled manner.
Tomi
Quoted 12 lines · Tomi Valkeinen
Quoted 11 lines · Chen Gang
Quoted 4 lines · Tomi Valkeinen
I don't think you should use BUG there. BUG should be used when there's not really a good way to continue. Here you could have just a WARN, and return some default FIFO watermark value.
i740_calc_fifo()is a static function, so we can check its caller's information to find the suitable fixing ways (for extern function, we almost can not do like this).
Quoted 9 lines
Quoted 3 lines
it has only one caller
i740fb_decode_var(), which has already let 'bpp' within the values (8, 15, 16, 24, 32). So if another values occurs, it must be a BUG (e.g. the stack may override under ia32).My point was that there should almost never be need for BUG in a normal driver. BUG means that the whole kernel will probably halt. Even if an fb driver encounters a problem that should never happen, it should maybe give a WARN, and continue or fail in a controlled manner.
e.g when the stack is override under ia32, it is better to stop continue as soon as possible to try to avoid the kernel continue blindly, that may let the coredump/KDB analyzers' work much easier.
Hmm... when driver cause issue, it has effect with the whole kernel (kernel may die soon), so BUG() is used under the whole kernel wide (include normal drivers).
Quoted 3 lines
Tomi
Thanks.
Signature · 1 line
Chen Gang
Quoted 23 lines · Chen Gang
Quoted 12 lines · Tomi Valkeinen
Quoted 11 lines · Chen Gang
Quoted 4 lines · Tomi Valkeinen
I don't think you should use BUG there. BUG should be used when there's not really a good way to continue. Here you could have just a WARN, and return some default FIFO watermark value.
i740_calc_fifo()is a static function, so we can check its caller's information to find the suitable fixing ways (for extern function, we almost can not do like this).Quoted 9 lines
Quoted 3 lines
it has only one caller
i740fb_decode_var(), which has already let 'bpp' within the values (8, 15, 16, 24, 32). So if another values occurs, it must be a BUG (e.g. the stack may override under ia32).My point was that there should almost never be need for BUG in a normal driver. BUG means that the whole kernel will probably halt. Even if an fb driver encounters a problem that should never happen, it should maybe give a WARN, and continue or fail in a controlled manner.
Quoted 7 lines
e.g when the stack is override under ia32, it is better to stop continue as soon as possible to try to avoid the kernel continue blindly, that may let the coredump/KDB analyzers' work much easier.
Hmm... when driver cause issue, it has effect with the whole kernel (kernel may die soon), so
BUG()is used under the whole kernel wide (include normal drivers).
You want i740_calc_fifo() to check the bpp parameter and issue a BUG if it's not a valid bpp-value, because in the current driver i740_calc_fifo() is never called with a non-valid bpp, and thus a wrong bpp indicates a stack corruption?
How about the freq parameter? In the current driver freq can never be higher than 1000000. If it is, it's stack corruption. Maybe there should be a BUG for that case also?
As I see it, you're just checking a single arbitrary value in an arbitrary place in the driver, and protecting against stack corruption there. Why not check all the values in all the functions of the driver as well, looking for stack corruptions?
And the bigger issue is that you're only talking about the current driver. The driver could be changed tomorrow, maybe calling i740_calc_fifo() from some other place, where a wrong bpp could just possibly happen. In that case it wouldn't be a stack corruption, but a "normal" driver bug.
So, in my opinion:
- Normally we should presume the the stack is not corrupted, or otherwise we'll end up with lots of checks all over.
- Even if i740_calc_fifo() is a static function, and we can analyze the _current_ situation, we don't know how the driver will evolve in the future.
Tomi
Quoted 28 lines · Tomi Valkeinen
Quoted 27 lines · Chen Gang
Quoted 12 lines · Tomi Valkeinen
Quoted 11 lines · Chen Gang
Quoted 4 lines · Tomi Valkeinen
I don't think you should use BUG there. BUG should be used when there's not really a good way to continue. Here you could have just a WARN, and return some default FIFO watermark value.
i740_calc_fifo()is a static function, so we can check its caller's information to find the suitable fixing ways (for extern function, we almost can not do like this).Quoted 9 lines
Quoted 3 lines
it has only one caller
i740fb_decode_var(), which has already let 'bpp' within the values (8, 15, 16, 24, 32). So if another values occurs, it must be a BUG (e.g. the stack may override under ia32).My point was that there should almost never be need for BUG in a normal driver. BUG means that the whole kernel will probably halt. Even if an fb driver encounters a problem that should never happen, it should maybe give a WARN, and continue or fail in a controlled manner.
e.g when the stack is override under ia32, it is better to stop continue as soon as possible to try to avoid the kernel continue blindly, that may let the coredump/KDB analyzers' work much easier.
Quoted 9 lines
Quoted 3 lines
Hmm... when driver cause issue, it has effect with the whole kernel (kernel may die soon), so
BUG()is used under the whole kernel wide (include normal drivers).You want
i740_calc_fifo()to check the bpp parameter and issue a BUG if it's not a valid bpp-value, because in the current driveri740_calc_fifo()is never called with a non-valid bpp, and thus a wrong bpp indicates a stack corruption?
Yes, it is just an e.g. (may also has another samples)
Quoted 4 lines
How about the freq parameter? In the current driver freq can never be higher than 1000000. If it is, it's stack corruption. Maybe there should be a BUG for that case also?
So I said:
"Hmm... it really has quite a few same cases in kernel wide like our case, but at least they do not report warnings (this does not like our case), so let just fix our case to satisfy the compiler. :-) "
Quoted 5 lines
As I see it, you're just checking a single arbitrary value in an arbitrary place in the driver, and protecting against stack corruption there. Why not check all the values in all the functions of the driver as well, looking for stack corruptions?
BUG() can not protect issues, it can try to protect the kernel continue blindly after kernel has issue.
Quoted 6 lines
And the bigger issue is that you're only talking about the current driver. The driver could be changed tomorrow, maybe calling
i740_calc_fifo()from some other place, where a wrong bpp could just possibly happen. In that case it wouldn't be a stack corruption, but a "normal" driver bug.
Yeah, BUG() is for bugs in kernel wide, ('stack corruption' is just one of sample).
And BUG() only report the direct cause, not report the root cause, so when one sub system report BUG, it does not mean this sub system must has this BUG.
Quoted 5 lines
So, in my opinion:
- Normally we should presume the the stack is not corrupted, or otherwise we'll end up with lots of checks all over.
It seems the above reply "Hmm... it really has quite a few same cases ..." is suitable for discussing with your opinion.
Quoted 3 lines
- Even if
i740_calc_fifo()is a static function, and we can analyze the _current_ situation, we don't know how the driver will evolve in the future.
For normal static function (not callback function) it can be in control within inside (ourselves), don't like extern function, it is out of control with inside.
So we can assume something in the future.
Quoted 3 lines
Tomi
Thanks.
Signature · 1 line
Chen Gang
Quoted 10 lines · Chen Gang
Quoted 5 lines · Tomi Valkeinen
So, in my opinion:
- Normally we should presume the the stack is not corrupted, or otherwise we'll end up with lots of checks all over.
It seems the above reply "Hmm... it really has quite a few same cases ..." is suitable for discussing with your opinion.
Well, if other drivers do silly things, it's no reason to do it here also.
Quoted 10 lines
Quoted 4 lines
- Even if
i740_calc_fifo()is a static function, and we can analyze the _current_ situation, we don't know how the driver will evolve in the future.For normal static function (not callback function) it can be in control within inside (ourselves), don't like extern function, it is out of control with inside.
So we can assume something in the future.
We can assume that only if we presume that the future changes are bug-free. But more often than not there are bugs in the kernel drivers.
Now, say, if such a bug is introduced, and somebody is running the kernel in a remote server, the driver will call BUG(). This will cause the server to halt. The user won't see what happened, the connections are just lost and the error is not written to a hard disk.
If, instead, the driver would do WARN, and continue or fail in a controlled manner, the user can find out about the issue easily. Even if there is a stack corruption, it's most likely the driver in question that has it, and thus not really fatal.
Sure, there's the possibility that there's a bigger memory corruption, which would go unnoticed by, say, the filesystem layer, resulting in a corrupted filesystem. And we would catch this corruption by checking the bpp variable. But, really, I find that very unlikely scenario.
Here's some old discussion about BUG:
http://yarchive.net/comp/linux/BUG.html
Tomi
Quoted 5 lines · Tomi Valkeinen
Quoted 4 lines · Chen Gang
Quoted 2 lines · Tomi Valkeinen
So, in my opinion:
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
- Normally we should presume the the stack is not corrupted, or otherwise we'll end up with lots of checks all over.
It seems the above reply "Hmm... it really has quite a few same cases ..." is suitable for discussing with your opinion.
Well, if other drivers do silly things, it's no reason to do it here also.
Hmm... I can understand. What I have done is only just "satisfy the compiler".
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
- Even if
i740_calc_fifo()is a static function, and we can analyze the _current_ situation, we don't know how the driver will evolve in the future.
Quoted 9 lines
Quoted 5 lines
For normal static function (not callback function) it can be in control within inside (ourselves), don't like extern function, it is out of control with inside.
So we can assume something in the future.
We can assume that only if we presume that the future changes are bug-free. But more often than not there are bugs in the kernel drivers.
Yeah.
Quoted 5 lines
Now, say, if such a bug is introduced, and somebody is running the kernel in a remote server, the driver will call
BUG(). This will cause the server to halt. The user won't see what happened, the connections are just lost and the error is not written to a hard disk.
For our case, it we don't call BUG(), the kernel will continue blindly, and in most cases, at last it will die too (or zombie, or some other critical issues).
So BUG() is only for trying to protect the kernel continue blindly.
Quoted 5 lines
If, instead, the driver would do WARN, and continue or fail in a controlled manner, the user can find out about the issue easily. Even if there is a stack corruption, it's most likely the driver in question that has it, and thus not really fatal.
Hmm, in my opinion, four our case, it is fatal, need try to stop kernel as soon as possible (don't let it continue blindly).
Quoted 5 lines
Sure, there's the possibility that there's a bigger memory corruption, which would go unnoticed by, say, the filesystem layer, resulting in a corrupted filesystem. And we would catch this corruption by checking the bpp variable. But, really, I find that very unlikely scenario.
Yeah. In fact the reason why I focus on 'bpp' is because of "satisfying the compiler" (not let it report warning).
Quoted 4 lines
Here's some old discussion about BUG:
Yeah, if it is not a real bug (can handle it), we should not use BUG(), but when we are sure it is a kernel bug, and the kernel will continue blindly, we need use BUG() to stop it.
Just like the Linus Torvalds said in the link which you provide:
"Rule of thumb: BUG() is only good for something that never happens and that we really have no other option for (ie state is so corrupt that continuing is deadly)".
In fact, for our case, when "default" happens: it is just match the contents above.
Quoted 3 lines
Tomi
Thanks.
Signature · 1 line
Chen Gang
Quoted 11 lines · Chen Gang
Quoted 4 lines · Tomi Valkeinen
Here's some old discussion about BUG:
Yeah, if it is not a real bug (can handle it), we should not use
BUG(), but when we are sure it is a kernel bug, and the kernel will continue blindly, we need useBUG()to stop it.
Quoted 5 lines
Just like the Linus Torvalds said in the link which you provide:
"Rule of thumb:
BUG()is only good for something that never happens and that we really have no other option for (ie state is so corrupt that continuing is deadly)".
I guess this is where we disagree. I don't see having a corrupt bpp value in a fb driver's internal function as "so corrupt that continuing is deadly".
Anyway, if you insist on the BUG(), I'll leave this patch to Jean-Christophe. I'm only taking small-ish patches that have no open issues or disagreements.
Tomi
Quoted 14 lines · Tomi Valkeinen
Quoted 13 lines · Chen Gang
Quoted 4 lines · Tomi Valkeinen
Here's some old discussion about BUG:
Yeah, if it is not a real bug (can handle it), we should not use
BUG(), but when we are sure it is a kernel bug, and the kernel will continue blindly, we need useBUG()to stop it.Just like the Linus Torvalds said in the link which you provide:
Quoted 8 lines
Quoted 3 lines
"Rule of thumb:
BUG()is only good for something that never happens and that we really have no other option for (ie state is so corrupt that continuing is deadly)".I guess this is where we disagree. I don't see having a corrupt bpp value in a fb driver's internal function as "so corrupt that continuing is deadly".
Hmm... at least we are agree with each other
"at least now, it should be never happen", is it correct ?
And we are disagree with each other:
"if it really happens, I think it is a critical bug, but you have different opinion", is it correct ?
Quoted 4 lines
Anyway, if you insist on the
BUG(), I'll leave this patch to Jean-Christophe. I'm only taking small-ish patches that have no open issues or disagreements.
OK, thank you for spending your time resources to provide your suggestion and opinion.
And welcome another members' suggestion and completions.
Quoted 3 lines
Tomi
Thanks.
Signature · 1 line
Chen Gang
Quoted 8 lines · Chen Gang
Hmm... at least we are agree with each other
"at least now, it should be never happen", is it correct ?
And we are disagree with each other:
"if it really happens, I think it is a critical bug, but you have different opinion", is it correct ?
Yes for both, if "critical" means that the kernel has to be halted immediately.
Tomi
Quoted 7 lines · Tomi Valkeinen
Quoted 5 lines · Chen Gang
Hmm... at least we are agree with each other
"at least now, it should be never happen", is it correct ?
Quoted 7 lines
Quoted 3 lines
And we are disagree with each other:
"if it really happens, I think it is a critical bug, but you have different opinion", is it correct ?
Yes for both, if "critical" means that the kernel has to be halted immediately.
Thank you for your confirmation.
:-)
Quoted 3 lines
Tomi
Signature · 1 line
Chen Gang