* Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Chen Gang @ 2013-09-02 1:41 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECF12D.8060903@asianux.com>
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
^ permalink raw reply
* [PATCH] video: xilinxfb: use devm_ioremap_resource() instead of devm_request_and_ioremap()
From: Jingoo Han @ 2013-09-02 1:33 UTC (permalink / raw)
To: linux-fbdev
Use devm_ioremap_resource() because devm_request_and_ioremap() is
obsoleted by devm_ioremap_resource().
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/xilinxfb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index 6629b29..ed90051 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -260,9 +260,9 @@ static int xilinxfb_assign(struct platform_device *pdev,
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
drvdata->regs_phys = res->start;
- drvdata->regs = devm_request_and_ioremap(&pdev->dev, res);
- if (!drvdata->regs) {
- rc = -EADDRNOTAVAIL;
+ drvdata->regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(drvdata->regs)) {
+ rc = PTR_ERR(drvdata->regs);
goto err_region;
}
}
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH] omap2: panel-generic: Added panel parameters for ortus-com37h3m05dtc/99dtc and sharp-lq0
From: Belisko Marek @ 2013-09-01 12:32 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jean-Christophe PLAGNIOL-VILLARD, linux-omap@vger.kernel.org,
linux-fbdev, LKML, H. Nikolaus Schaller
In-Reply-To: <52205056.8080908@ti.com>
Hi Tomi,
On Fri, Aug 30, 2013 at 9:57 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 29/08/13 15:35, Marek Belisko wrote:
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Signed-off-by: Marek Belisko <marek@goldelico.com>
>> ---
>> drivers/video/omap2/displays/panel-generic-dpi.c | 53 ++++++++++++++++++++++++
>> 1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/video/omap2/displays/panel-generic-dpi.c b/drivers/video/omap2/displays/panel-generic-dpi.c
>> index bebebd4..d573291 100644
>> --- a/drivers/video/omap2/displays/panel-generic-dpi.c
>> +++ b/drivers/video/omap2/displays/panel-generic-dpi.c
>> @@ -107,6 +107,33 @@ static struct panel_config generic_dpi_panels[] = {
>> .name = "sharp_ls",
>> },
>
> The drivers in drivers/video/omap2/displays/ are on their way out, and
> will probably be removed for 3.12. Please look at the new one at
> drivers/video/omap2/displays-new/panel-dpi.c.
I've probably miss series (OMAPDSS: remove old panel model code)
and in time when sent patch it wasn't in linux-next yes. AFAIU generic
panel settings
was moved to board files which is not case for our patch as gta04
(other) board with panel isn't yes mainlined.
>
> Tomi
>
>
Regards,
marek
--
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer
Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com
^ permalink raw reply
* RE: [PATCH 0/4] video: da8xx-fb: numerous bugfixes
From: Etheridge, Darren @ 2013-08-30 15:25 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1377294773-25678-1-git-send-email-detheridge@ti.com>
> From: Valkeinen, Tomi
> Sent: Friday, August 30, 2013 6:53 AM
> >
> > Darren Etheridge (4):
> > video: da8xx-fb fixing incorrect porch mappings
> > video: da8xx-fb: fixing timing off by one errors
> > video: da8xx-fb: support lcdc v2 timing register expansion
> > video: da8xx-fb: fix the polarities of the hsync/vsync pulse
> >
> > drivers/video/da8xx-fb.c | 35 +++++++++++++++++++++++++----------
> > 1 files changed, 25 insertions(+), 10 deletions(-)
> >
>
> These look good to me, queuing for 3.12.
>
> There was a space missing between 'if' and '(' in one of the patches, I fixed
> that.
>
> Tomi
>
Tomi,
Thank you.
Darren
^ permalink raw reply
* Re: Bochs VBE & Virtualbox framebuffer driver
From: Tomi Valkeinen @ 2013-08-30 11:58 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <5211D634.3010204@castus.tv>
[-- Attachment #1: Type: text/plain, Size: 638 bytes --]
Hi,
On 19/08/13 11:24, Jonathan Campbell wrote:
> This is an experimental framebuffer driver for VirtualBox and Bochs VBE
> displays. It started off as a proof of concept and grew into the code it
> is now. When testing Linux in Bochs or VirtualBox this allows greater
> control over the display and bit depth, though no h/w acceleration.
>
> What do you think? Would this be something that could make it into the
> mainline kernel?
I don't see why not, although I know nothing about VirtualBox nor Bochs.
But with a very quick glance, it at least needs to be changed to match
the kernel coding conventions.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 0/4] video: da8xx-fb: numerous bugfixes
From: Tomi Valkeinen @ 2013-08-30 11:52 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1377294773-25678-1-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]
Hi,
On 24/08/13 00:52, Darren Etheridge wrote:
> In developing a driver for an HDMI encoder to be attached to the LCD controller
> on TI AM335x SoC I ran across a number of bugs in the da8xx-fb.c fbdev driver.
> Two of them appear to have been in the driver for a while (02 and 04), one of
> them is a deficiency where some LCD controller version 2 features have not been
> utilized (03) and the last one was introduced in the last patch series that I
> submitted for this driver (01).
>
> These patches do not change the behavior of the AM335x EVM's platforms LCD
> panel even though the current values set by the driver are wrong. I guess LCD
> panels (at least the one used on AM335x EVM) must be fairly immune to bad
> timings coming from the LCD controller. However the HDMI encoder simply will
> not work without these fixes, and quite possibly other LCD panels could behave
> the same way.
>
> These patches apply on top of the patch series called:
> "video/da8xx-fb fbdev driver enhance to support TI am335x SoC"
> that was submitted by me.
>
> Darren Etheridge (4):
> video: da8xx-fb fixing incorrect porch mappings
> video: da8xx-fb: fixing timing off by one errors
> video: da8xx-fb: support lcdc v2 timing register expansion
> video: da8xx-fb: fix the polarities of the hsync/vsync pulse
>
> drivers/video/da8xx-fb.c | 35 +++++++++++++++++++++++++----------
> 1 files changed, 25 insertions(+), 10 deletions(-)
>
These look good to me, queuing for 3.12.
There was a space missing between 'if' and '(' in one of the patches, I
fixed that.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Tomi Valkeinen @ 2013-08-30 10:52 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECF12D.8060903@asianux.com>
[-- 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 --]
^ permalink raw reply
* Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Chen Gang @ 2013-08-30 10:41 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECF12D.8060903@asianux.com>
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
^ permalink raw reply
* Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Tomi Valkeinen @ 2013-08-30 10:19 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECF12D.8060903@asianux.com>
[-- 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 --]
^ permalink raw reply
* Re: [RFC 00/22] OMAPDSS: DT support
From: Tomi Valkeinen @ 2013-08-30 9:47 UTC (permalink / raw)
To: Tony Lindgren
Cc: linux-omap, linux-fbdev, devicetree, Archit Taneja,
Laurent Pinchart, Nishanth Menon, Felipe Balbi, Santosh Shilimkar
In-Reply-To: <20130813075449.GS7656@atomide.com>
[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]
On 13/08/13 10:54, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [130809 01:46]:
>>
>> So as is evident, I have things in my mind that should be improved. Maybe
>> the most important question for short term future is:
>>
>> Can we add DSS DT bindings for OMAP4 as unstable bindings? It would give us
>> some proper testing of the related code, and would also allow us to remove
>> the related hacks (which don't even work quite right). However, I have no
>> idea yet when the unstable DSS bindings would turn stable.
>>
>> If we shouldn't add the bindings as unstable, when should the bindings be
>> added? Wait until CDF is in the mainline, and use that?
>
> I don't think we should add any temporary bindings as it's going to be
> a pain to support those in the long run. I suggest you initially just
> stick to established bindings for the basic hardware IO address and
> interrupts etc, then those should still be valid with the generic panel
> bindings later on.
I don't understand what does it matter if the bindings are temporary, or
basic established bindings. In both cases the DT data needs to be
changed when the CDF is taken into use.
Well, one difference is that the temporary bindings would give us
working display, but having only basic bindings would not. So I don't
see any reason to add only the basic bindings. Or how would it work?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Chen Gang @ 2013-08-30 9:45 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECF12D.8060903@asianux.com>
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
^ permalink raw reply
* Re: [PATCH 6/7] video: xilinxfb: replace devm_request_and_ioremap by devm_ioremap_resource
From: Tomi Valkeinen @ 2013-08-30 9:42 UTC (permalink / raw)
To: Julia Lawall
Cc: Jean-Christophe Plagniol-Villard, kernel-janitors, linux-fbdev,
linux-kernel
In-Reply-To: <1376911241-27720-7-git-send-email-Julia.Lawall@lip6.fr>
[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]
On 19/08/13 14:20, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Use devm_ioremap_resource instead of devm_request_and_ioremap.
>
> This was done using the semantic patch
> scripts/coccinelle/api/devm_ioremap_resource.cocci
>
> The initialization of drvdata->regs_phys was manually moved lower, to take
> advantage of the NULL test on res performed by devm_ioremap_resource.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
> drivers/video/xilinxfb.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
> index 6629b29..84c664e 100644
> --- a/drivers/video/xilinxfb.c
> +++ b/drivers/video/xilinxfb.c
> @@ -259,12 +259,12 @@ static int xilinxfb_assign(struct platform_device *pdev,
> struct resource *res;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - drvdata->regs_phys = res->start;
> - drvdata->regs = devm_request_and_ioremap(&pdev->dev, res);
> - if (!drvdata->regs) {
> - rc = -EADDRNOTAVAIL;
> + drvdata->regs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(drvdata->regs)) {
> + rc = PTR_ERR(drvdata->regs);
> goto err_region;
> }
> + drvdata->regs_phys = res->start;
> }
>
> /* Allocate the framebuffer memory */
Thanks, queued for 3.12.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Tomi Valkeinen @ 2013-08-30 9:16 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECF12D.8060903@asianux.com>
[-- 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 --]
^ permalink raw reply
* Re: [PATCH v2] drivers: video: fbcmap: remove the redundency and incorrect checkings
From: Chen Gang @ 2013-08-30 8:47 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <5212D594.3020802@asianux.com>
On 08/30/2013 04:41 PM, Tomi Valkeinen wrote:
> On 20/08/13 05:33, Chen Gang wrote:
>> fb_set_cmap() already checks the parameters, so need remove the
>> redundancy checking.
>>
>> This redundancy checking is also incorrect, the related warning:
>>
>> drivers/video/fbcmap.c:288:2: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>> drivers/video/fbcmap.c | 7 +------
>> 1 files changed, 1 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
>> index 5c3960d..f89245b 100644
>> --- a/drivers/video/fbcmap.c
>> +++ b/drivers/video/fbcmap.c
>> @@ -285,13 +285,8 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
>> rc = -ENODEV;
>> goto out;
>> }
>> - if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
>> - !info->fbops->fb_setcmap)) {
>> - rc = -EINVAL;
>> - goto out1;
>> - }
>> +
>> rc = fb_set_cmap(&umap, info);
>> -out1:
>> unlock_fb_info(info);
>> out:
>> fb_dealloc_cmap(&umap);
>>
>
> Thanks, queued this for 3.12.
>
Thank you too.
> Tomi
>
>
--
Chen Gang
^ permalink raw reply
* Re: [PATCH] fbmem: move EXPORT_SYMBOL annotation next to symbol declarations
From: Tomi Valkeinen @ 2013-08-30 8:44 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1376575975-13215-1-git-send-email-zonque@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 335 bytes --]
On 15/08/13 17:12, Daniel Mack wrote:
> Just a cosmetic thing to bring that file in line with others in the
> tree.
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
> drivers/video/fbmem.c | 29 +++++++++++++----------------
> 1 file changed, 13 insertions(+), 16 deletions(-)
Thanks, queued for 3.12.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Chen Gang @ 2013-08-30 8:44 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECF12D.8060903@asianux.com>
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
^ permalink raw reply
* Re: [PATCH v2] drivers: video: fbcmap: remove the redundency and incorrect checkings
From: Tomi Valkeinen @ 2013-08-30 8:41 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <5212D594.3020802@asianux.com>
[-- Attachment #1: Type: text/plain, Size: 1088 bytes --]
On 20/08/13 05:33, Chen Gang wrote:
> fb_set_cmap() already checks the parameters, so need remove the
> redundancy checking.
>
> This redundancy checking is also incorrect, the related warning:
>
> drivers/video/fbcmap.c:288:2: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
> drivers/video/fbcmap.c | 7 +------
> 1 files changed, 1 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
> index 5c3960d..f89245b 100644
> --- a/drivers/video/fbcmap.c
> +++ b/drivers/video/fbcmap.c
> @@ -285,13 +285,8 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
> rc = -ENODEV;
> goto out;
> }
> - if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
> - !info->fbops->fb_setcmap)) {
> - rc = -EINVAL;
> - goto out1;
> - }
> +
> rc = fb_set_cmap(&umap, info);
> -out1:
> unlock_fb_info(info);
> out:
> fb_dealloc_cmap(&umap);
>
Thanks, queued this for 3.12.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Tomi Valkeinen @ 2013-08-30 8:36 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECF12D.8060903@asianux.com>
[-- 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 --]
^ permalink raw reply
* Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Chen Gang @ 2013-08-30 8:17 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECF12D.8060903@asianux.com>
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
^ permalink raw reply
* Re: [PATCH] omap2: panel-generic: Added panel parameters for ortus-com37h3m05dtc/99dtc and sharp-lq0
From: Tomi Valkeinen @ 2013-08-30 7:57 UTC (permalink / raw)
To: Marek Belisko
Cc: plagnioj, linux-omap, linux-fbdev, linux-kernel,
H. Nikolaus Schaller
In-Reply-To: <1377779724-30648-1-git-send-email-marek@goldelico.com>
[-- Attachment #1: Type: text/plain, Size: 862 bytes --]
On 29/08/13 15:35, Marek Belisko wrote:
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> Signed-off-by: Marek Belisko <marek@goldelico.com>
> ---
> drivers/video/omap2/displays/panel-generic-dpi.c | 53 ++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/drivers/video/omap2/displays/panel-generic-dpi.c b/drivers/video/omap2/displays/panel-generic-dpi.c
> index bebebd4..d573291 100644
> --- a/drivers/video/omap2/displays/panel-generic-dpi.c
> +++ b/drivers/video/omap2/displays/panel-generic-dpi.c
> @@ -107,6 +107,33 @@ static struct panel_config generic_dpi_panels[] = {
> .name = "sharp_ls",
> },
The drivers in drivers/video/omap2/displays/ are on their way out, and
will probably be removed for 3.12. Please look at the new one at
drivers/video/omap2/displays-new/panel-dpi.c.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 2/29] video: mxsfb: simplify use of devm_ioremap_resource
From: Tomi Valkeinen @ 2013-08-30 7:51 UTC (permalink / raw)
To: Julia Lawall
Cc: Jean-Christophe Plagniol-Villard, kernel-janitors, linux-fbdev,
linux-kernel
In-Reply-To: <1376471493-22215-3-git-send-email-Julia.Lawall@lip6.fr>
[-- Attachment #1: Type: text/plain, Size: 1991 bytes --]
On 14/08/13 12:11, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Remove unneeded error handling on the result of a call to
> platform_get_resource when the value is passed to devm_ioremap_resource.
>
> Move the call to platform_get_resource adjacent to the call to
> devm_ioremap_resource to make the connection between them more clear.
>
> A simplified version of the semantic patch that makes this change is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression pdev,res,n,e,e1;
> expression ret != 0;
> identifier l;
> @@
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, n);
> ... when != res
> - if (res == NULL) { ... \(goto l;\|return ret;\) }
> ... when != res
> + res = platform_get_resource(pdev, IORESOURCE_MEM, n);
> e = devm_ioremap_resource(e1, res);
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
> drivers/video/mxsfb.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
> index c2d3514..d250ed0 100644
> --- a/drivers/video/mxsfb.c
> +++ b/drivers/video/mxsfb.c
> @@ -855,12 +855,6 @@ static int mxsfb_probe(struct platform_device *pdev)
> if (of_id)
> pdev->id_entry = of_id->data;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(&pdev->dev, "Cannot get memory IO resource\n");
> - return -ENODEV;
> - }
> -
> fb_info = framebuffer_alloc(sizeof(struct mxsfb_info), &pdev->dev);
> if (!fb_info) {
> dev_err(&pdev->dev, "Failed to allocate fbdev\n");
> @@ -869,6 +863,7 @@ static int mxsfb_probe(struct platform_device *pdev)
>
> host = to_imxfb_host(fb_info);
>
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> host->base = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(host->base)) {
> ret = PTR_ERR(host->base);
>
Thanks, queued for 3.12.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH] Release efifb's colormap in efifb_destroy()
From: Tomi Valkeinen @ 2013-08-30 7:47 UTC (permalink / raw)
To: Peter Jones
Cc: Catalin Marinas, Alexandra N. Kossovsky, linux-fbdev,
linux-kernel
In-Reply-To: <1374767291-2874-1-git-send-email-pjones@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]
On 25/07/13 18:48, Peter Jones wrote:
> This was found by Alexandra Kossovsky, who noted this traceback from
> kmemleak:
>
>> unreferenced object 0xffff880216fcfe00 (size 512):
>> comm "swapper/0", pid 1, jiffies 4294895429 (age 1415.320s)
>> hex dump (first 32 bytes):
>> 00 00 00 00 00 00 00 00 aa aa aa aa aa aa aa aa ................
>> 55 55 55 55 55 55 55 55 ff ff ff ff ff ff ff ff UUUUUUUU........
>> backtrace:
>> [<ffffffff813e415c>] kmemleak_alloc+0x21/0x3e
>> [<ffffffff8111c17f>]
>> kmemleak_alloc_recursive.constprop.57+0x16/0x18
>> [<ffffffff8111e63b>] __kmalloc+0xf9/0x144
>> [<ffffffff8123d9cf>] fb_alloc_cmap_gfp+0x47/0xe1
>> [<ffffffff8123da77>] fb_alloc_cmap+0xe/0x10
>> [<ffffffff81aff40a>] efifb_probe+0x3e9/0x48f
>> [<ffffffff812c566f>] platform_drv_probe+0x34/0x5e
>> [<ffffffff812c3e6d>] driver_probe_device+0x98/0x1b4
>> [<ffffffff812c3fd7>] __driver_attach+0x4e/0x6f
>> [<ffffffff812c25bf>] bus_for_each_dev+0x57/0x8a
>> [<ffffffff812c3984>] driver_attach+0x19/0x1b
>> [<ffffffff812c362b>] bus_add_driver+0xde/0x201
>> [<ffffffff812c453f>] driver_register+0x8c/0x110
>> [<ffffffff812c510d>] platform_driver_register+0x41/0x43
>> [<ffffffff812c5127>] platform_driver_probe+0x18/0x8a
>> [<ffffffff81aff002>] efifb_init+0x276/0x295
> ---
> drivers/video/efifb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c
> index 390b61b..1f3eab3 100644
> --- a/drivers/video/efifb.c
> +++ b/drivers/video/efifb.c
> @@ -289,6 +289,7 @@ static void efifb_destroy(struct fb_info *info)
> if (request_mem_succeeded)
> release_mem_region(info->apertures->ranges[0].base,
> info->apertures->ranges[0].size);
> + fb_dealloc_cmap(&info->cmap);
> framebuffer_release(info);
> }
>
Thanks, queued for 3.12.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Tomi Valkeinen @ 2013-08-30 7:21 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECF12D.8060903@asianux.com>
[-- 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 --]
^ permalink raw reply
* Re: [PATCH v4 3/5] at91/avr32/atmel_lcdfb: prepare clk before calling enable
From: Tomi Valkeinen @ 2013-08-30 7:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374133225-19141-1-git-send-email-b.brezillon@overkiz.com>
[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]
On 18/07/13 10:40, Boris BREZILLON wrote:
> Replace clk_enable/disable with clk_prepare_enable/disable_unprepare to
> avoid common clk framework warnings.
>
> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> drivers/video/atmel_lcdfb.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
> index ece49d5..bf9c5d0 100644
> --- a/drivers/video/atmel_lcdfb.c
> +++ b/drivers/video/atmel_lcdfb.c
> @@ -954,14 +954,14 @@ static int __init atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo)
>
> static void atmel_lcdfb_start_clock(struct atmel_lcdfb_info *sinfo)
> {
> - clk_enable(sinfo->bus_clk);
> - clk_enable(sinfo->lcdc_clk);
> + clk_prepare_enable(sinfo->bus_clk);
> + clk_prepare_enable(sinfo->lcdc_clk);
> }
>
> static void atmel_lcdfb_stop_clock(struct atmel_lcdfb_info *sinfo)
> {
> - clk_disable(sinfo->bus_clk);
> - clk_disable(sinfo->lcdc_clk);
> + clk_disable_unprepare(sinfo->bus_clk);
> + clk_disable_unprepare(sinfo->lcdc_clk);
> }
>
> #ifdef CONFIG_OF
Thanks, queued this for 3.12.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH] video: exynos: Ensure definitions match prototypes
From: Tomi Valkeinen @ 2013-08-30 7:15 UTC (permalink / raw)
To: Mark Brown
Cc: Jingoo Han, linux-fbdev, linux-samsung-soc, linaro-kernel,
Mark Brown
In-Reply-To: <1372764414-9102-1-git-send-email-broonie@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 355 bytes --]
On 02/07/13 14:26, Mark Brown wrote:
> From: Mark Brown <broonie@linaro.org>
>
> Ensure that the definitions of functions match the prototypes used by
> other modules by including the header with the prototypes in the files
> with the definitions.
>
> Signed-off-by: Mark Brown <broonie@linaro.org>
Thanks, queued this for 3.12.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox