[PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[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 <gang.chen@asianux.com>
---
 drivers/video/i740fb.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/video/i740fb.c b/drivers/video/i740fb.c
index 6c48388..e82e767 100644
--- a/drivers/video/i740fb.c
+++ b/drivers/video/i740fb.c
@@ -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;
-- 
1.7.7.6

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On 22/07/13 11:45, Chen Gang wrote:
> 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 <gang.chen@asianux.com>
> ---
>  drivers/video/i740fb.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/i740fb.c b/drivers/video/i740fb.c
> index 6c48388..e82e767 100644
> --- a/drivers/video/i740fb.c
> +++ b/drivers/video/i740fb.c
> @@ -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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Chen Gang]

On 08/30/2013 03:21 PM, Tomi Valkeinen wrote:
> On 22/07/13 11:45, Chen Gang wrote:
>> 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 <gang.chen@asianux.com>
>> ---
>>  drivers/video/i740fb.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/video/i740fb.c b/drivers/video/i740fb.c
>> index 6c48388..e82e767 100644
>> --- a/drivers/video/i740fb.c
>> +++ b/drivers/video/i740fb.c
>> @@ -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. :-)

>  Tomi
> 
> 

Thanks.
-- 
Chen Gang

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

On 30/08/13 11:17, Chen Gang wrote:
> On 08/30/2013 03:21 PM, Tomi Valkeinen wrote:

>> 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).

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Chen Gang]

On 08/30/2013 04:36 PM, Tomi Valkeinen wrote:
> On 30/08/13 11:17, Chen Gang wrote:
>> On 08/30/2013 03:21 PM, Tomi Valkeinen wrote:
> 
>>> 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).
> 
> 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).

>  Tomi
> 
> 

Thanks.
-- 
Chen Gang

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2757 bytes --]

On 30/08/13 11:44, Chen Gang wrote:
> On 08/30/2013 04:36 PM, Tomi Valkeinen wrote:
>> On 30/08/13 11:17, Chen Gang wrote:
>>> On 08/30/2013 03:21 PM, Tomi Valkeinen wrote:
>>
>>>> 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).
>>
>> 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).

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Chen Gang]

On 08/30/2013 05:16 PM, Tomi Valkeinen wrote:
> On 30/08/13 11:44, Chen Gang wrote:
>> On 08/30/2013 04:36 PM, Tomi Valkeinen wrote:
>>> On 30/08/13 11:17, Chen Gang wrote:
>>>> On 08/30/2013 03:21 PM, Tomi Valkeinen wrote:
>>>
>>>>> 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).
>>>
>>> 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).
> 
> 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?
> 

Yes, it is just an e.g. (may also has another samples)

> 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. :-) "

> 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.


> 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.


> 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.


> - 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.

>  Tomi
> 
> 

Thanks.
-- 
Chen Gang

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]

On 30/08/13 12:45, Chen Gang wrote:
> On 08/30/2013 05:16 PM, Tomi Valkeinen wrote:

>> 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.

> 
>> - 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Chen Gang]

On 08/30/2013 06:19 PM, Tomi Valkeinen wrote:
> On 30/08/13 12:45, Chen Gang wrote:
>> On 08/30/2013 05:16 PM, Tomi Valkeinen wrote:
> 
>>> 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.
> 

Hmm... I can understand.  What I have done is only just "satisfy the
compiler".

>>
>>> - 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.
> 

Yeah.

> 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.


> 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).

> 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).

> Here's some old discussion about BUG:
> 
> http://yarchive.net/comp/linux/BUG.html
> 

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.

>  Tomi
> 
> 


Thanks.
-- 
Chen Gang

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On 30/08/13 13:41, Chen Gang wrote:
> On 08/30/2013 06:19 PM, Tomi Valkeinen wrote:

>> Here's some old discussion about BUG:
>>
>> http://yarchive.net/comp/linux/BUG.html
>>
> 
> 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)".

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Chen Gang]

On 08/30/2013 06:52 PM, Tomi Valkeinen wrote:
> On 30/08/13 13:41, Chen Gang wrote:
>> On 08/30/2013 06:19 PM, Tomi Valkeinen wrote:
> 
>>> Here's some old discussion about BUG:
>>>
>>> http://yarchive.net/comp/linux/BUG.html
>>>
>>
>> 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)".
> 
> 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 ?


> 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.

>  Tomi
> 
> 


Thanks.
-- 
Chen Gang

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

On 02/09/13 04:41, Chen Gang wrote:

> 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.

[Chen Gang]

On 09/02/2013 02:45 PM, Tomi Valkeinen wrote:
> On 02/09/13 04:41, Chen Gang wrote:
> 
>> 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.
> 

Thank you for your confirmation.

:-)

>  Tomi
> 
> 


-- 
Chen Gang