* Re: [PATCH 11/12] fbdev: shmobile-hdmi: Convert to clk_prepare/unprepare
From: Tomi Valkeinen @ 2013-11-11 13:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1388008.xjh7MrPrF3@avalon>
[-- Attachment #1: Type: text/plain, Size: 712 bytes --]
On 2013-11-09 16:13, Laurent Pinchart wrote:
> Hi Jean-Christophe,
>
> On Thursday 31 October 2013 11:48:09 Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On 23:49 Mon 28 Oct , Laurent Pinchart wrote:
>>> Turn clk_enable() and clk_disable() calls into clk_prepare_enable() and
>>> clk_disable_unprepare() to get ready for the migration to the common
>>> clock framework.
>>>
>>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>
>> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>
> Thank you. Could you please pick patches 11/12 and 12/12 up for v3.13 or v3.14
> ?
Thanks, picked both for 3.13.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH] ARM: OMAPFB: panel-sony-acx565akm: fix bad unlock balance
From: Tomi Valkeinen @ 2013-11-11 13:37 UTC (permalink / raw)
To: Aaro Koskinen, linux-omap, linux-fbdev; +Cc: Eduardo Valentin, stable
In-Reply-To: <1383773070-15114-1-git-send-email-aaro.koskinen@iki.fi>
[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]
On 2013-11-06 23:24, Aaro Koskinen wrote:
> When booting Nokia N900 smartphone with v3.12 + omap2plus_defconfig
> (LOCKDEP enabled) and CONFIG_DISPLAY_PANEL_SONY_ACX565AKM enabled,
> the following BUG is seen during the boot:
>
> [ 7.302154] =====================================
> [ 7.307128] [ BUG: bad unlock balance detected! ]
> [ 7.312103] 3.12.0-los.git-2093492-00120-g5e01dc7 #3 Not tainted
> [ 7.318450] -------------------------------------
> [ 7.323425] kworker/u2:1/12 is trying to release lock (&ddata->mutex) at:
> [ 7.330657] [<c031b760>] acx565akm_enable+0x12c/0x18c
> [ 7.335998] but there are no more locks to release!
>
> Fix by removing the extra mutex_unlock().
>
> Reported-by: Eduardo Valentin <eduardo.valentin@ti.com>
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Cc: stable@vger.kernel.org
> ---
> drivers/video/omap2/displays-new/panel-sony-acx565akm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
> index e6d56f7..72fe2a8 100644
> --- a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
> +++ b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
> @@ -616,7 +616,7 @@ static int acx565akm_enable(struct omap_dss_device *dssdev)
>
> mutex_lock(&ddata->mutex);
> r = acx565akm_panel_power_on(dssdev);
> - mutex_unlock(&ddata->mutex);
> + /* NOTE: acx565akm_panel_power_on() will unlock the mutex. */
>
> if (r)
> return r;
>
Hm why would you fix it like this? Why not remove the mutex_unlock from
acx565akm_panel_power_on()? Looks to me like that one is the buggy one.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.
From: Dr. H. Nikolaus Schaller @ 2013-11-11 13:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5280DBA6.50303@ti.com>
Hi Tomi,
Am 11.11.2013 um 14:29 schrieb Tomi Valkeinen:
> Hi,
>
> On 2013-11-05 09:24, Belisko Marek wrote:
>> Hi,
>>
>> ping.
>>
>> On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek@goldelico.com> wrote:
>>> This patches is adding bypass and acbias functionality to omapdss venc driver.
>>> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
>>> add handling for updating in venc driver and last patch add driver for opa362 which
>>> is used on gta04 board and set bypass + acbias.
>> Is there a chance to get this series to 3.13? Thanks.
>
> Sorry, I haven't had time to do much reviewing.
>
> The code in omap3-tvout.c should be included in the display.c file,
> which already contains some things like muxing.
Ok, that might be better - but we were not sure where to place code to
modify the DEVCONF1 registers. It is not directly DSS related but a special
control of the OMAP3 SoC. Therefore we think it is better located in omap3-tvout.c
> Also, func(bool, bool)
> style functions are rather confusing to read. Maybe an enum would be
> better, so you'd instead have something like:
>
> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN)
We have no special preference on that.
>
> But the main issue is: while this series probably works well, I really
> don't like it that the OPA driver needs to pass bypass and acbias. It
> shouldn't know anything about such things. I'm just not certain how to
> implement that with the current omapdss driver.
Well, it must know bypass and acbias because it requires the OMAP
CPU to be configured accordingly. I.e. it is the one who pushes the
button. Or we get a problem that people might misconfigure it.
I would see it similar as a driver must be able to control power. Here
it must be able to control bypass and acbias in a special way that it works.
> I'll try to find time to think about this more, but I don't think I can
> merge this for 3.13.
Please take your time.
And please also consider the approach to merge it into 3.13 as it is
and we can then fix it in the weeks while release candidates are pushed.
But that is your decision.
BR and thanks,
Nikolaus
^ permalink raw reply
* Re: [RESEND PATCH 1/2] fb: reorder the lock sequence to fix potential dead lock
From: Tomi Valkeinen @ 2013-11-11 13:59 UTC (permalink / raw)
To: Gu Zheng, Jean-Christophe PLAGNIOL-VILLARD
Cc: Linux Fbdev development list, linux-kernel
In-Reply-To: <5278C1D9.9030101@cn.fujitsu.com>
[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]
On 2013-11-05 12:00, Gu Zheng wrote:
> Following commits:
> 50e244cc79 fb: rework locking to fix lock ordering on takeover
> e93a9a8687 fb: Yet another band-aid for fixing lockdep mess
> 054430e773 fbcon: fix locking harder
> reworked locking to fix related lock ordering on takeover, and introduced console_lock
> into fbmem, but it seems that the new lock sequence(fb_info->lock ---> console_lock)
> is against with the one in console_callback(console_lock ---> fb_info->lock), and leads to
> a potential dead lock as following:
<snip>
> so we reorder the lock sequence the same as it in console_callback() to
> avoid this issue. And following Tomi's suggestion, fix these similar
> issues all in fb subsystem.
>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
> drivers/video/fbmem.c | 50 ++++++++++++++++++++++++-------------
> drivers/video/fbsysfs.c | 19 ++++++++++----
> drivers/video/sh_mobile_lcdcfb.c | 10 ++++---
> 3 files changed, 51 insertions(+), 28 deletions(-)
I'll apply this for 3.13. It's a bit difficult to verify if the locking
is now correct, but looks fine to me. And we can revert this easily if
things break badly.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.
From: Tomi Valkeinen @ 2013-11-11 14:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <FB10035C-B13C-4A03-BE9C-0576806F2577@goldelico.com>
[-- Attachment #1: Type: text/plain, Size: 3610 bytes --]
On 2013-11-11 15:57, Dr. H. Nikolaus Schaller wrote:
> Hi Tomi,
>
> Am 11.11.2013 um 14:29 schrieb Tomi Valkeinen:
>
>> Hi,
>>
>> On 2013-11-05 09:24, Belisko Marek wrote:
>>> Hi,
>>>
>>> ping.
>>>
>>> On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek@goldelico.com> wrote:
>>>> This patches is adding bypass and acbias functionality to omapdss venc driver.
>>>> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
>>>> add handling for updating in venc driver and last patch add driver for opa362 which
>>>> is used on gta04 board and set bypass + acbias.
>>> Is there a chance to get this series to 3.13? Thanks.
>>
>> Sorry, I haven't had time to do much reviewing.
>>
>> The code in omap3-tvout.c should be included in the display.c file,
>> which already contains some things like muxing.
>
> Ok, that might be better - but we were not sure where to place code to
> modify the DEVCONF1 registers. It is not directly DSS related but a special
> control of the OMAP3 SoC. Therefore we think it is better located in omap3-tvout.c
The display.c file is not strictly DSS stuff, but DSS related "glue" for
the SoC. For example, the muxing of the DSI pads is also done on the
CONTROL module, and it's also in display.c.
The file is getting a bit large, so I'm not against splitting it. But I
don't think there's point to add omap3-tvout.c file, which most likely
will ever contain only that one function.
>> Also, func(bool, bool)
>> style functions are rather confusing to read. Maybe an enum would be
>> better, so you'd instead have something like:
>>
>> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN)
>
> We have no special preference on that.
It's just about readability. Which one tells you better what the code does:
func(true, true);
or
func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN);
>> But the main issue is: while this series probably works well, I really
>> don't like it that the OPA driver needs to pass bypass and acbias. It
>> shouldn't know anything about such things. I'm just not certain how to
>> implement that with the current omapdss driver.
>
> Well, it must know bypass and acbias because it requires the OMAP
> CPU to be configured accordingly. I.e. it is the one who pushes the
> button. Or we get a problem that people might misconfigure it.
While the omapdss display drivers are currently OMAP specific, I want to
(or at least try to) keep them platform independent. Afaik, acbias and
bypass are OMAP VENC specific things, not something that every SoC with
an analog TV-out have. Thus, the OPA driver should not know about them,
and it should be something that only the DSS glue code in mach-omap2/
and the venc driver know about.
> I would see it similar as a driver must be able to control power. Here
> it must be able to control bypass and acbias in a special way that it works.
The difference is that on all possible SoCs where you could add OPA
chip, you'll need to manage the power for OPA, and it's done in a common
way. Whereas the bypass and acbias are OMAP specific things.
>> I'll try to find time to think about this more, but I don't think I can
>> merge this for 3.13.
>
> Please take your time.
>
> And please also consider the approach to merge it into 3.13 as it is
> and we can then fix it in the weeks while release candidates are pushed.
If it was purely omapdss code, I could possibly take it. But it contains
arch/arm code also, and I don't want to cause needless changes on code
that I do not maintain.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.
From: Dr. H. Nikolaus Schaller @ 2013-11-11 14:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5280E617.3000004@ti.com>
Am 11.11.2013 um 15:13 schrieb Tomi Valkeinen:
> On 2013-11-11 15:57, Dr. H. Nikolaus Schaller wrote:
>> Hi Tomi,
>>
>> Am 11.11.2013 um 14:29 schrieb Tomi Valkeinen:
>>
>>> Hi,
>>>
>>> On 2013-11-05 09:24, Belisko Marek wrote:
>>>> Hi,
>>>>
>>>> ping.
>>>>
>>>> On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek@goldelico.com> wrote:
>>>>> This patches is adding bypass and acbias functionality to omapdss venc driver.
>>>>> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
>>>>> add handling for updating in venc driver and last patch add driver for opa362 which
>>>>> is used on gta04 board and set bypass + acbias.
>>>> Is there a chance to get this series to 3.13? Thanks.
>>>
>>> Sorry, I haven't had time to do much reviewing.
>>>
>>> The code in omap3-tvout.c should be included in the display.c file,
>>> which already contains some things like muxing.
>>
>> Ok, that might be better - but we were not sure where to place code to
>> modify the DEVCONF1 registers. It is not directly DSS related but a special
>> control of the OMAP3 SoC. Therefore we think it is better located in omap3-tvout.c
>
> The display.c file is not strictly DSS stuff, but DSS related "glue" for
> the SoC. For example, the muxing of the DSI pads is also done on the
> CONTROL module, and it's also in display.c.
>
> The file is getting a bit large, so I'm not against splitting it. But I
> don't think there's point to add omap3-tvout.c file, which most likely
> will ever contain only that one function.
Yes that is very likely true.
The problem is that there is no other official API to modify the DEFCONF1
register. Therefore we introduced this (propsal).
Our first idea was a readDEFCONF1() and writeDEFCONF1() and use the
constants (bit patterns) you suggested below.
But we thought that it is not robust enough because a VENC driver or other
one could then change bits it is not responsible for.
>
>>> Also, func(bool, bool)
>>> style functions are rather confusing to read. Maybe an enum would be
>>> better, so you'd instead have something like:
>>>
>>> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN)
>>
>> We have no special preference on that.
>
> It's just about readability. Which one tells you better what the code does:
>
> func(true, true);
>
> or
>
> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN);
Hm. Depends on. If the func is explaining enough it is clear. Or if I have
access to some header file. If I don't, the enum values are probably more
readable.
>
>>> But the main issue is: while this series probably works well, I really
>>> don't like it that the OPA driver needs to pass bypass and acbias. It
>>> shouldn't know anything about such things. I'm just not certain how to
>>> implement that with the current omapdss driver.
>>
>> Well, it must know bypass and acbias because it requires the OMAP
>> CPU to be configured accordingly. I.e. it is the one who pushes the
>> button. Or we get a problem that people might misconfigure it.
>
> While the omapdss display drivers are currently OMAP specific, I want to
> (or at least try to) keep them platform independent. Afaik, acbias and
> bypass are OMAP VENC specific things, not something that every SoC with
> an analog TV-out have. Thus, the OPA driver should not know about them,
> and it should be something that only the DSS glue code in mach-omap2/
> and the venc driver know about.
Well, there must be a method how the OPA can tell the VENC that it exists
and the VENC can tell the SoC DEFCONF1 to enable bias and bypass.
>
>> I would see it similar as a driver must be able to control power. Here
>> it must be able to control bypass and acbias in a special way that it works.
>
> The difference is that on all possible SoCs where you could add OPA
> chip, you'll need to manage the power for OPA, and it's done in a common
> way. Whereas the bypass and acbias are OMAP specific things.
Maybe it looks as if it is an unsolvable problem. The OPA works only if acbias
and bypass are enabled, but is not allowed to tell that it is there.
>
>>> I'll try to find time to think about this more, but I don't think I can
>>> merge this for 3.13.
>>
>> Please take your time.
>>
>> And please also consider the approach to merge it into 3.13 as it is
>> and we can then fix it in the weeks while release candidates are pushed.
>
> If it was purely omapdss code, I could possibly take it. But it contains
> arch/arm code also,
because we think that it is unavoidable (no API to control the DEFCONF1
register exists yet).
Maybe one of the omap3 core maintainers can comment as well?
> and I don't want to cause needless changes on code
> that I do not maintain.
Yes, this should be avoided of course.
BR,
Nikolaus
^ permalink raw reply
* Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.
From: Dr. H. Nikolaus Schaller @ 2013-11-11 14:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5280E617.3000004@ti.com>
Am 11.11.2013 um 15:13 schrieb Tomi Valkeinen:
> On 2013-11-11 15:57, Dr. H. Nikolaus Schaller wrote:
>> Hi Tomi,
>>
>> Am 11.11.2013 um 14:29 schrieb Tomi Valkeinen:
>>
>>> Hi,
>>>
>>> On 2013-11-05 09:24, Belisko Marek wrote:
>>>> Hi,
>>>>
>>>> ping.
>>>>
>>>> On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek@goldelico.com> wrote:
>>>>> This patches is adding bypass and acbias functionality to omapdss venc driver.
>>>>> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
>>>>> add handling for updating in venc driver and last patch add driver for opa362 which
>>>>> is used on gta04 board and set bypass + acbias.
>>>> Is there a chance to get this series to 3.13? Thanks.
>>>
>>> Sorry, I haven't had time to do much reviewing.
>>>
>>> The code in omap3-tvout.c should be included in the display.c file,
>>> which already contains some things like muxing.
>>
>> Ok, that might be better - but we were not sure where to place code to
>> modify the DEVCONF1 registers. It is not directly DSS related but a special
>> control of the OMAP3 SoC. Therefore we think it is better located in omap3-tvout.c
>
> The display.c file is not strictly DSS stuff, but DSS related "glue" for
> the SoC. For example, the muxing of the DSI pads is also done on the
> CONTROL module, and it's also in display.c.
>
> The file is getting a bit large, so I'm not against splitting it. But I
> don't think there's point to add omap3-tvout.c file, which most likely
> will ever contain only that one function.
>
>>> Also, func(bool, bool)
>>> style functions are rather confusing to read. Maybe an enum would be
>>> better, so you'd instead have something like:
>>>
>>> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN)
>>
>> We have no special preference on that.
>
> It's just about readability. Which one tells you better what the code does:
>
> func(true, true);
>
> or
>
> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN);
>
>>> But the main issue is: while this series probably works well, I really
>>> don't like it that the OPA driver needs to pass bypass and acbias. It
>>> shouldn't know anything about such things. I'm just not certain how to
>>> implement that with the current omapdss driver.
>>
>> Well, it must know bypass and acbias because it requires the OMAP
>> CPU to be configured accordingly. I.e. it is the one who pushes the
>> button. Or we get a problem that people might misconfigure it.
>
> While the omapdss display drivers are currently OMAP specific, I want to
> (or at least try to) keep them platform independent. Afaik, acbias and
> bypass are OMAP VENC specific things, not something that every SoC with
> an analog TV-out have. Thus, the OPA driver should not know about them,
> and it should be something that only the DSS glue code in mach-omap2/
> and the venc driver know about.
What about calling it prepare_venc_for_external_amplifier (or similar). Then,
venc.c can hide that really has to be done and call whatever SoC API is
needed to program DEFCONF1?
>
>> I would see it similar as a driver must be able to control power. Here
>> it must be able to control bypass and acbias in a special way that it works.
>
> The difference is that on all possible SoCs where you could add OPA
> chip, you'll need to manage the power for OPA, and it's done in a common
> way. Whereas the bypass and acbias are OMAP specific things.
>
>>> I'll try to find time to think about this more, but I don't think I can
>>> merge this for 3.13.
>>
>> Please take your time.
>>
>> And please also consider the approach to merge it into 3.13 as it is
>> and we can then fix it in the weeks while release candidates are pushed.
>
> If it was purely omapdss code, I could possibly take it. But it contains
> arch/arm code also, and I don't want to cause needless changes on code
> that I do not maintain.
>
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.
From: Tony Lindgren @ 2013-11-11 16:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1381784555-18344-2-git-send-email-marek@goldelico.com>
* Marek Belisko <marek@goldelico.com> [131014 14:11]:
> devconf1 reg access is localized only in mach-omap2 and we need to export
> updating of devconf1 from omapdss venc driver (bypass and acbias bits).
> Add simple api call which update only necessary bits.
...
> +void update_bypass_acbias(bool bypass, bool acbias)
> +{
> +#ifdef CONFIG_ARCH_OMAP3
> + int val = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
> +
> + if (bypass)
> + val |= OMAP2_TVOUTBYPASS;
> + else
> + val &= ~OMAP2_TVOUTBYPASS;
> +
> + if (acbias)
> + val |= OMAP2_TVACEN;
> + else
> + val &= ~OMAP2_TVACEN;
> +
> + omap_ctrl_writel(val, OMAP343X_CONTROL_DEVCONF1);
> +#endif
If this is truly a pinmux, you could already access this
using pinctrl-single,bits device tree driver.
But I guess that won't work yet, so it's best to set this
up as a separate driver like we've done for the USB PHY
registers.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.
From: Tony Lindgren @ 2013-11-11 16:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <F97741D2-FC6F-4DBD-A0DD-E35304501993@goldelico.com>
* Dr. H. Nikolaus Schaller <hns@goldelico.com> [131111 06:30]:
> Am 11.11.2013 um 15:13 schrieb Tomi Valkeinen:
> > On 2013-11-11 15:57, Dr. H. Nikolaus Schaller wrote:
> >
> > The display.c file is not strictly DSS stuff, but DSS related "glue" for
> > the SoC. For example, the muxing of the DSI pads is also done on the
> > CONTROL module, and it's also in display.c.
> >
> > The file is getting a bit large, so I'm not against splitting it. But I
> > don't think there's point to add omap3-tvout.c file, which most likely
> > will ever contain only that one function.
>
> Yes that is very likely true.
>
> The problem is that there is no other official API to modify the DEFCONF1
> register. Therefore we introduced this (propsal).
>
> Our first idea was a readDEFCONF1() and writeDEFCONF1() and use the
> constants (bit patterns) you suggested below.
I posted something about accessing the ctrl module regs to the first
patch in the series. It's best to set it up as a separate driver for
now and then maybe handle it with pinctrl-single,bits when DSS is
device tree enabled.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH] ARM: OMAPFB: panel-sony-acx565akm: fix bad unlock balance
From: Aaro Koskinen @ 2013-11-11 18:37 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, Eduardo Valentin, stable
In-Reply-To: <5280DDA1.304@ti.com>
Hi,
On Mon, Nov 11, 2013 at 03:37:37PM +0200, Tomi Valkeinen wrote:
> On 2013-11-06 23:24, Aaro Koskinen wrote:
> > When booting Nokia N900 smartphone with v3.12 + omap2plus_defconfig
> > (LOCKDEP enabled) and CONFIG_DISPLAY_PANEL_SONY_ACX565AKM enabled,
> > the following BUG is seen during the boot:
> >
> > [ 7.302154] ==================> > [ 7.307128] [ BUG: bad unlock balance detected! ]
> > [ 7.312103] 3.12.0-los.git-2093492-00120-g5e01dc7 #3 Not tainted
> > [ 7.318450] -------------------------------------
> > [ 7.323425] kworker/u2:1/12 is trying to release lock (&ddata->mutex) at:
> > [ 7.330657] [<c031b760>] acx565akm_enable+0x12c/0x18c
> > [ 7.335998] but there are no more locks to release!
> >
> > Fix by removing the extra mutex_unlock().
> >
> > Reported-by: Eduardo Valentin <eduardo.valentin@ti.com>
> > Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > Cc: stable@vger.kernel.org
> > ---
> > drivers/video/omap2/displays-new/panel-sony-acx565akm.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
> > index e6d56f7..72fe2a8 100644
> > --- a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
> > +++ b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
> > @@ -616,7 +616,7 @@ static int acx565akm_enable(struct omap_dss_device *dssdev)
> >
> > mutex_lock(&ddata->mutex);
> > r = acx565akm_panel_power_on(dssdev);
> > - mutex_unlock(&ddata->mutex);
> > + /* NOTE: acx565akm_panel_power_on() will unlock the mutex. */
> >
> > if (r)
> > return r;
> >
>
> Hm why would you fix it like this? Why not remove the mutex_unlock from
> acx565akm_panel_power_on()? Looks to me like that one is the buggy one.
The unlock needs to be there because acx565akm_bl_update_status()
also locks the mutex. I'll send a new version where the mutex_lock()
is done inside acx565akm_panel_power_on().
A.
^ permalink raw reply
* [PATCH v2] ARM: OMAPFB: panel-sony-acx565akm: fix bad unlock balance
From: Aaro Koskinen @ 2013-11-11 18:41 UTC (permalink / raw)
To: Tomi Valkeinen, linux-omap, linux-fbdev
Cc: Eduardo Valentin, Aaro Koskinen, stable
When booting Nokia N900 smartphone with v3.12 + omap2plus_defconfig
(LOCKDEP enabled) and CONFIG_DISPLAY_PANEL_SONY_ACX565AKM enabled,
the following BUG is seen during the boot:
[ 7.302154] ==================[ 7.307128] [ BUG: bad unlock balance detected! ]
[ 7.312103] 3.12.0-los.git-2093492-00120-g5e01dc7 #3 Not tainted
[ 7.318450] -------------------------------------
[ 7.323425] kworker/u2:1/12 is trying to release lock (&ddata->mutex) at:
[ 7.330657] [<c031b760>] acx565akm_enable+0x12c/0x18c
[ 7.335998] but there are no more locks to release!
Fix by removing double unlock and handling the locking completely inside
acx565akm_panel_power_on() when doing the power on.
Reported-by: Eduardo Valentin <eduardo.valentin@ti.com>
Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Cc: stable@vger.kernel.org
---
drivers/video/omap2/displays-new/panel-sony-acx565akm.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
index e6d56f7..d94f35d 100644
--- a/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
+++ b/drivers/video/omap2/displays-new/panel-sony-acx565akm.c
@@ -526,6 +526,8 @@ static int acx565akm_panel_power_on(struct omap_dss_device *dssdev)
struct omap_dss_device *in = ddata->in;
int r;
+ mutex_lock(&ddata->mutex);
+
dev_dbg(&ddata->spi->dev, "%s\n", __func__);
in->ops.sdi->set_timings(in, &ddata->videomode);
@@ -614,10 +616,7 @@ static int acx565akm_enable(struct omap_dss_device *dssdev)
if (omapdss_device_is_enabled(dssdev))
return 0;
- mutex_lock(&ddata->mutex);
r = acx565akm_panel_power_on(dssdev);
- mutex_unlock(&ddata->mutex);
-
if (r)
return r;
--
1.8.4.2
^ permalink raw reply related
* Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.
From: Belisko Marek @ 2013-11-11 22:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20131111164917.GN15154@atomide.com>
Hi Tony,
On Mon, Nov 11, 2013 at 5:49 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Marek Belisko <marek@goldelico.com> [131014 14:11]:
>> devconf1 reg access is localized only in mach-omap2 and we need to export
>> updating of devconf1 from omapdss venc driver (bypass and acbias bits).
>> Add simple api call which update only necessary bits.
> ...
>
>> +void update_bypass_acbias(bool bypass, bool acbias)
>> +{
>> +#ifdef CONFIG_ARCH_OMAP3
>> + int val = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
>> +
>> + if (bypass)
>> + val |= OMAP2_TVOUTBYPASS;
>> + else
>> + val &= ~OMAP2_TVOUTBYPASS;
>> +
>> + if (acbias)
>> + val |= OMAP2_TVACEN;
>> + else
>> + val &= ~OMAP2_TVACEN;
>> +
>> + omap_ctrl_writel(val, OMAP343X_CONTROL_DEVCONF1);
>> +#endif
>
> If this is truly a pinmux, you could already access this
> using pinctrl-single,bits device tree driver.
>
> But I guess that won't work yet, so it's best to set this
> up as a separate driver like we've done for the USB PHY
> registers.
Can you please point me to that driver directly? I cannot see benefit
of new driver as as it will be only dummy driver
with export_symbol_gpl of upper function. Can it sit then in dss
directory or somewhere else? Thanks.
>
> Regards,
>
> Tony
BR,
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 1/3] arm: omap2: Export devconf1 bypass and acbias.
From: Belisko Marek @ 2013-11-11 22:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20131111164917.GN15154@atomide.com>
Hi Tony,
On Mon, Nov 11, 2013 at 5:49 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Marek Belisko <marek@goldelico.com> [131014 14:11]:
>> devconf1 reg access is localized only in mach-omap2 and we need to export
>> updating of devconf1 from omapdss venc driver (bypass and acbias bits).
>> Add simple api call which update only necessary bits.
> ...
>
>> +void update_bypass_acbias(bool bypass, bool acbias)
>> +{
>> +#ifdef CONFIG_ARCH_OMAP3
>> + int val = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
>> +
>> + if (bypass)
>> + val |= OMAP2_TVOUTBYPASS;
>> + else
>> + val &= ~OMAP2_TVOUTBYPASS;
>> +
>> + if (acbias)
>> + val |= OMAP2_TVACEN;
>> + else
>> + val &= ~OMAP2_TVACEN;
>> +
>> + omap_ctrl_writel(val, OMAP343X_CONTROL_DEVCONF1);
>> +#endif
>
> If this is truly a pinmux, you could already access this
> using pinctrl-single,bits device tree driver.
AFAIK it's not pinmux it's register where we update bits. Or am I
missing something?
>
> But I guess that won't work yet, so it's best to set this
> up as a separate driver like we've done for the USB PHY
> registers.
>
> Regards,
>
> Tony
BR,
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 1/3] arm: omap2: Export devconf1 bypass and acbias.
From: Tony Lindgren @ 2013-11-11 23:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAAfyv37LRb_Q=UZVgtKdOAz_r6_qThewAU90W7nrqHdB5fDA_g@mail.gmail.com>
* Belisko Marek <marek.belisko@gmail.com> [131111 14:01]:
> Hi Tony,
>
> On Mon, Nov 11, 2013 at 5:49 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Marek Belisko <marek@goldelico.com> [131014 14:11]:
> >> devconf1 reg access is localized only in mach-omap2 and we need to export
> >> updating of devconf1 from omapdss venc driver (bypass and acbias bits).
> >> Add simple api call which update only necessary bits.
> > ...
> >
> >> +void update_bypass_acbias(bool bypass, bool acbias)
> >> +{
> >> +#ifdef CONFIG_ARCH_OMAP3
> >> + int val = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
> >> +
> >> + if (bypass)
> >> + val |= OMAP2_TVOUTBYPASS;
> >> + else
> >> + val &= ~OMAP2_TVOUTBYPASS;
> >> +
> >> + if (acbias)
> >> + val |= OMAP2_TVACEN;
> >> + else
> >> + val &= ~OMAP2_TVACEN;
> >> +
> >> + omap_ctrl_writel(val, OMAP343X_CONTROL_DEVCONF1);
> >> +#endif
> >
> > If this is truly a pinmux, you could already access this
> > using pinctrl-single,bits device tree driver.
> >
> > But I guess that won't work yet, so it's best to set this
> > up as a separate driver like we've done for the USB PHY
> > registers.
>
> Can you please point me to that driver directly? I cannot see benefit
> of new driver as as it will be only dummy driver
> with export_symbol_gpl of upper function. Can it sit then in dss
> directory or somewhere else? Thanks.
You can take a look at drivers/usb/phy/phy-omap-control.c.
Then there's a device tree using example for control_devconf0 in
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt.
Looks like CONTROL_DEVCONF1 contains a bunch of misc registers,
and arch/arm/mach-omap2/hsmmc.c seems to be tinkering with it too.
It would be best to set it up as omap-ctrl.c driver under drivers
somewhere with few functions exported for DSS and MMC drivers.
The reason why it should be a separate driver is that the system
control module can live a separate runtime PM life from the
drivers using the CONTROL_DEVCONF register bits.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.
From: Tony Lindgren @ 2013-11-11 23:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAAfyv34CyviSt_Jaygr-CYSQdZOBHz14awT3U8=2ZBT14s896g@mail.gmail.com>
* Belisko Marek <marek.belisko@gmail.com> [131111 14:21]:
> AFAIK it's not pinmux it's register where we update bits. Or am I
> missing something?
Most of the omap control module registers seem to affect either
external pin configuration or internal signal muxes. So a big
chunk of them can already be handled with pinctrl-single,bits
for device tree.
But some of the control module registers also have regulator
and clock like features, and those are better exposed as
regulators and clocks to the consumer drivers.
Regards,
Tony
^ permalink raw reply
* [PATCH] video: backlight: Remove backlight sysfs uevent
From: Kyungmin Park @ 2013-11-11 23:57 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han; +Cc: linux-fbdev, linux-kernel, kay
From: Kyungmin Park <kyungmin.park@samsung.com>
The most mobile phones have Ambient Light Sensors and it changes brightness according lux.
It means it changes backlight brightness frequently by just writing sysfs node, so it generates uevent.
Usually there's no user to use this backlight changes. But it forks udev worker threads and it takes about 5ms. The main problem is that it hurts other process activities. so remove it.
Kay said
"Uevents are for the major, low-frequent, global device state-changes,
not for carrying-out any sort of measurement data. Subsystems which
need that should use other facilities like poll()-able sysfs file or
any other subscription-based, client-tracking interface which does not
cause overhead if it isn't used. Uevents are not the right thing to
use here, and upstream udev should not paper-over broken kernel
subsystems."
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 94a403a..441272d 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -88,9 +88,6 @@ static void backlight_generate_event(struct backlight_device *bd,
char *envp[2];
switch (reason) {
- case BACKLIGHT_UPDATE_SYSFS:
- envp[0] = "SOURCE=sysfs";
- break;
case BACKLIGHT_UPDATE_HOTKEY:
envp[0] = "SOURCE=hotkey";
break;
@@ -172,8 +169,6 @@ static ssize_t brightness_store(struct device *dev,
}
mutex_unlock(&bd->ops_lock);
- backlight_generate_event(bd, BACKLIGHT_UPDATE_SYSFS);
-
return rc;
}
static DEVICE_ATTR_RW(brightness);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 53b7794..d2a27dd 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -29,7 +29,6 @@
enum backlight_update_reason {
BACKLIGHT_UPDATE_HOTKEY,
- BACKLIGHT_UPDATE_SYSFS,
};
enum backlight_type {
^ permalink raw reply related
* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Jingoo Han @ 2013-11-12 0:18 UTC (permalink / raw)
To: 'Kyungmin Park', 'Henrique de Moraes Holschuh'
Cc: linux-fbdev, linux-kernel, kay, 'Richard Purdie',
'Jingoo Han', ibm-acpi-devel, platform-driver-x86
In-Reply-To: <20131111235700.GA29987@july>
On Tuesday, November 12, 2013 8:57 AM, Kyungmin Park wrote:
>
> From: Kyungmin Park <kyungmin.park@samsung.com>
>
> The most mobile phones have Ambient Light Sensors and it changes brightness according lux.
> It means it changes backlight brightness frequently by just writing sysfs node, so it generates uevent.
>
> Usually there's no user to use this backlight changes. But it forks udev worker threads and it takes
> about 5ms. The main problem is that it hurts other process activities. so remove it.
>
> Kay said
> "Uevents are for the major, low-frequent, global device state-changes,
> not for carrying-out any sort of measurement data. Subsystems which
> need that should use other facilities like poll()-able sysfs file or
> any other subscription-based, client-tracking interface which does not
> cause overhead if it isn't used. Uevents are not the right thing to
> use here, and upstream udev should not paper-over broken kernel
> subsystems."
>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 94a403a..441272d 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -88,9 +88,6 @@ static void backlight_generate_event(struct backlight_device *bd,
> char *envp[2];
>
> switch (reason) {
> - case BACKLIGHT_UPDATE_SYSFS:
> - envp[0] = "SOURCE=sysfs";
> - break;
> case BACKLIGHT_UPDATE_HOTKEY:
> envp[0] = "SOURCE=hotkey";
> break;
> @@ -172,8 +169,6 @@ static ssize_t brightness_store(struct device *dev,
> }
> mutex_unlock(&bd->ops_lock);
>
> - backlight_generate_event(bd, BACKLIGHT_UPDATE_SYSFS);
> -
> return rc;
> }
> static DEVICE_ATTR_RW(brightness);
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 53b7794..d2a27dd 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -29,7 +29,6 @@
>
> enum backlight_update_reason {
> BACKLIGHT_UPDATE_HOTKEY,
> - BACKLIGHT_UPDATE_SYSFS,
+cc Henrique de Moraes Holschuh (Maintainer of thinkpad_acpi)
Hi Henrique de Moraes Holschuh,
'thinkpad_acpi.c' uses the 'BACKLIGHT_UPDATE_SYSFS'.
Henrique, can we remove it?
drivers/platform/x86/thinkpad_acpi.c
if (!rc && ibm_backlight_device)
backlight_force_update(ibm_backlight_device,
BACKLIGHT_UPDATE_SYSFS);
Best regards,
Jingoo Han
> };
>
> enum backlight_type {
^ permalink raw reply
* [PATCH] video: backlight: Remove backlight sysfs uevent
From: Kyungmin Park @ 2013-11-12 0:44 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han; +Cc: linux-fbdev, linux-kernel, kay
In-Reply-To: <20131111235700.GA29987@july>
From: Kyungmin Park <kyungmin.park@samsung.com>
The most mobile phones have Ambient Light Sensors and it changes brightness according lux.
It means it changes backlight brightness frequently by just writing sysfs node, so it generates uevent.
Usually there's no user to use this backlight changes. But it forks udev worker threads and it takes about 5ms. The main problem is that it hurts other process activities. so remove it.
Kay said
"Uevents are for the major, low-frequent, global device state-changes,
not for carrying-out any sort of measurement data. Subsystems which
need that should use other facilities like poll()-able sysfs file or
any other subscription-based, client-tracking interface which does not
cause overhead if it isn't used. Uevents are not the right thing to
use here, and upstream udev should not paper-over broken kernel
subsystems."
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 94a403a..441272d 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -88,9 +88,6 @@ static void backlight_generate_event(struct backlight_device *bd,
char *envp[2];
switch (reason) {
- case BACKLIGHT_UPDATE_SYSFS:
- envp[0] = "SOURCE=sysfs";
- break;
case BACKLIGHT_UPDATE_HOTKEY:
envp[0] = "SOURCE=hotkey";
break;
@@ -172,8 +169,6 @@ static ssize_t brightness_store(struct device *dev,
}
mutex_unlock(&bd->ops_lock);
- backlight_generate_event(bd, BACKLIGHT_UPDATE_SYSFS);
-
return rc;
}
static DEVICE_ATTR_RW(brightness);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 53b7794..d2a27dd 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -29,7 +29,6 @@
enum backlight_update_reason {
BACKLIGHT_UPDATE_HOTKEY,
- BACKLIGHT_UPDATE_SYSFS,
};
enum backlight_type {
^ permalink raw reply related
* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Kyungmin Park @ 2013-11-12 0:54 UTC (permalink / raw)
To: Richard Purdie, Jingoo Han; +Cc: linux-fbdev, linux-kernel, kay
In-Reply-To: <20131112004436.GA30125@july>
Please ignore it. strange mail system.
After feedback from previous mail, I'll re-send it.
Thank you,
Kyungmin Park
On Tue, Nov 12, 2013 at 9:44 AM, Kyungmin Park <kmpark@infradead.org> wrote:
> From: Kyungmin Park <kyungmin.park@samsung.com>
>
> The most mobile phones have Ambient Light Sensors and it changes brightness according lux.
> It means it changes backlight brightness frequently by just writing sysfs node, so it generates uevent.
>
> Usually there's no user to use this backlight changes. But it forks udev worker threads and it takes about 5ms. The main problem is that it hurts other process activities. so remove it.
>
> Kay said
> "Uevents are for the major, low-frequent, global device state-changes,
> not for carrying-out any sort of measurement data. Subsystems which
> need that should use other facilities like poll()-able sysfs file or
> any other subscription-based, client-tracking interface which does not
> cause overhead if it isn't used. Uevents are not the right thing to
> use here, and upstream udev should not paper-over broken kernel
> subsystems."
>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 94a403a..441272d 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -88,9 +88,6 @@ static void backlight_generate_event(struct backlight_device *bd,
> char *envp[2];
>
> switch (reason) {
> - case BACKLIGHT_UPDATE_SYSFS:
> - envp[0] = "SOURCE=sysfs";
> - break;
> case BACKLIGHT_UPDATE_HOTKEY:
> envp[0] = "SOURCE=hotkey";
> break;
> @@ -172,8 +169,6 @@ static ssize_t brightness_store(struct device *dev,
> }
> mutex_unlock(&bd->ops_lock);
>
> - backlight_generate_event(bd, BACKLIGHT_UPDATE_SYSFS);
> -
> return rc;
> }
> static DEVICE_ATTR_RW(brightness);
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 53b7794..d2a27dd 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -29,7 +29,6 @@
>
> enum backlight_update_reason {
> BACKLIGHT_UPDATE_HOTKEY,
> - BACKLIGHT_UPDATE_SYSFS,
> };
>
> enum backlight_type {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Henrique de Moraes Holschuh @ 2013-11-12 0:56 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Kyungmin Park', 'Henrique de Moraes Holschuh',
linux-fbdev, linux-kernel, kay, 'Richard Purdie',
ibm-acpi-devel, platform-driver-x86
In-Reply-To: <002901cedf3c$a7e77a00$f7b66e00$%han@samsung.com>
On Tue, 12 Nov 2013, Jingoo Han wrote:
> On Tuesday, November 12, 2013 8:57 AM, Kyungmin Park wrote:
> > From: Kyungmin Park <kyungmin.park@samsung.com>
> >
> > The most mobile phones have Ambient Light Sensors and it changes brightness according lux.
> > It means it changes backlight brightness frequently by just writing sysfs node, so it generates uevent.
> >
> > Usually there's no user to use this backlight changes. But it forks udev worker threads and it takes
> > about 5ms. The main problem is that it hurts other process activities. so remove it.
> >
> > Kay said
> > "Uevents are for the major, low-frequent, global device state-changes,
> > not for carrying-out any sort of measurement data. Subsystems which
> > need that should use other facilities like poll()-able sysfs file or
> > any other subscription-based, client-tracking interface which does not
> > cause overhead if it isn't used. Uevents are not the right thing to
> > use here, and upstream udev should not paper-over broken kernel
> > subsystems."
True.
Now, let's take a look at reality: should you poll()/select() on a sysfs
node that doesn't suport it, it will wait until the poll/select timeout
happens (or EINTR happens), and userspace has absolutely NO way to detect
whether a sysfs node has poll/select support.
What happens if the sysfs interface did not provide poll/select support
since day one, but rather added it later? Nobody will use it for a *long*
time, if ever... unless you actually took pains to version the sysfs
interface, and people actually care.
> 'thinkpad_acpi.c' uses the 'BACKLIGHT_UPDATE_SYSFS'.
> Henrique, can we remove it?
Can't you fix this by rate-limiting, or otherwise adding an attribute that
backlight devices should set when they need to supress change events?
Is there a proper on-screen-display support path for the backlight class
nowadays? Otherwise, you'd be removing the only way userspace ever had to
do proper OSD of backlight changes...
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
^ permalink raw reply
* Re: [RESEND PATCH 1/2] fb: reorder the lock sequence to fix potential dead lock
From: Gu Zheng @ 2013-11-12 1:06 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jean-Christophe PLAGNIOL-VILLARD, Linux Fbdev development list,
linux-kernel
In-Reply-To: <5280E2BF.7040602@ti.com>
Hi Tomi,
On 11/11/2013 09:59 PM, Tomi Valkeinen wrote:
> On 2013-11-05 12:00, Gu Zheng wrote:
>> Following commits:
>> 50e244cc79 fb: rework locking to fix lock ordering on takeover
>> e93a9a8687 fb: Yet another band-aid for fixing lockdep mess
>> 054430e773 fbcon: fix locking harder
>> reworked locking to fix related lock ordering on takeover, and introduced console_lock
>> into fbmem, but it seems that the new lock sequence(fb_info->lock ---> console_lock)
>> is against with the one in console_callback(console_lock ---> fb_info->lock), and leads to
>> a potential dead lock as following:
>
> <snip>
>
>> so we reorder the lock sequence the same as it in console_callback() to
>> avoid this issue. And following Tomi's suggestion, fix these similar
>> issues all in fb subsystem.
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>> drivers/video/fbmem.c | 50 ++++++++++++++++++++++++-------------
>> drivers/video/fbsysfs.c | 19 ++++++++++----
>> drivers/video/sh_mobile_lcdcfb.c | 10 ++++---
>> 3 files changed, 51 insertions(+), 28 deletions(-)
>
> I'll apply this for 3.13. It's a bit difficult to verify if the locking
> is now correct, but looks fine to me. And we can revert this easily if
> things break badly.
Thanks very munch.:)
Regards,
Gu
>
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Kyungmin Park @ 2013-11-12 1:07 UTC (permalink / raw)
To: Henrique de Moraes Holschuh
Cc: Jingoo Han, Henrique de Moraes Holschuh, linux-fbdev,
linux-kernel, kay, Richard Purdie, ibm-acpi-devel,
platform-driver-x86
In-Reply-To: <20131112005628.GA2914@khazad-dum.debian.net>
On Tue, Nov 12, 2013 at 9:56 AM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Tue, 12 Nov 2013, Jingoo Han wrote:
>> On Tuesday, November 12, 2013 8:57 AM, Kyungmin Park wrote:
>> > From: Kyungmin Park <kyungmin.park@samsung.com>
>> >
>> > The most mobile phones have Ambient Light Sensors and it changes brightness according lux.
>> > It means it changes backlight brightness frequently by just writing sysfs node, so it generates uevent.
>> >
>> > Usually there's no user to use this backlight changes. But it forks udev worker threads and it takes
>> > about 5ms. The main problem is that it hurts other process activities. so remove it.
>> >
>> > Kay said
>> > "Uevents are for the major, low-frequent, global device state-changes,
>> > not for carrying-out any sort of measurement data. Subsystems which
>> > need that should use other facilities like poll()-able sysfs file or
>> > any other subscription-based, client-tracking interface which does not
>> > cause overhead if it isn't used. Uevents are not the right thing to
>> > use here, and upstream udev should not paper-over broken kernel
>> > subsystems."
>
> True.
>
> Now, let's take a look at reality: should you poll()/select() on a sysfs
> node that doesn't suport it, it will wait until the poll/select timeout
> happens (or EINTR happens), and userspace has absolutely NO way to detect
> whether a sysfs node has poll/select support.
>
> What happens if the sysfs interface did not provide poll/select support
> since day one, but rather added it later? Nobody will use it for a *long*
> time, if ever... unless you actually took pains to version the sysfs
> interface, and people actually care.
>
>> 'thinkpad_acpi.c' uses the 'BACKLIGHT_UPDATE_SYSFS'.
>> Henrique, can we remove it?
>
> Can't you fix this by rate-limiting, or otherwise adding an attribute that
> backlight devices should set when they need to supress change events?
other way is that just remove sysfs node store update then you can use
current API, force_update as is.
are there any other good ideas?
Thank you,
Kyungmin Park
>
> Is there a proper on-screen-display support path for the backlight class
> nowadays? Otherwise, you'd be removing the only way userspace ever had to
> do proper OSD of backlight changes...
>
> --
> "One disk to rule them all, One disk to find them. One disk to bring
> them all and in the darkness grind them. In the Land of Redmond
> where the shadows lie." -- The Silicon Valley Tarot
> Henrique Holschuh
^ permalink raw reply
* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Kay Sievers @ 2013-11-12 1:19 UTC (permalink / raw)
To: Henrique de Moraes Holschuh
Cc: Jingoo Han, Kyungmin Park, Henrique de Moraes Holschuh,
linux-fbdev, LKML, Richard Purdie, ibm-acpi-devel,
platform-driver-x86
In-Reply-To: <20131112005628.GA2914@khazad-dum.debian.net>
On Tue, Nov 12, 2013 at 1:56 AM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Tue, 12 Nov 2013, Jingoo Han wrote:
>> On Tuesday, November 12, 2013 8:57 AM, Kyungmin Park wrote:
>> > From: Kyungmin Park <kyungmin.park@samsung.com>
>> >
>> > The most mobile phones have Ambient Light Sensors and it changes brightness according lux.
>> > It means it changes backlight brightness frequently by just writing sysfs node, so it generates uevent.
>> >
>> > Usually there's no user to use this backlight changes. But it forks udev worker threads and it takes
>> > about 5ms. The main problem is that it hurts other process activities. so remove it.
>> >
>> > Kay said
>> > "Uevents are for the major, low-frequent, global device state-changes,
>> > not for carrying-out any sort of measurement data. Subsystems which
>> > need that should use other facilities like poll()-able sysfs file or
>> > any other subscription-based, client-tracking interface which does not
>> > cause overhead if it isn't used. Uevents are not the right thing to
>> > use here, and upstream udev should not paper-over broken kernel
>> > subsystems."
>
> True.
>
> Now, let's take a look at reality: should you poll()/select() on a sysfs
> node that doesn't suport it, it will wait until the poll/select timeout
> happens (or EINTR happens), and userspace has absolutely NO way to detect
> whether a sysfs node has poll/select support.
>
> What happens if the sysfs interface did not provide poll/select support
> since day one, but rather added it later? Nobody will use it for a *long*
> time, if ever... unless you actually took pains to version the sysfs
> interface, and people actually care.
If that's an issue, we can add a new "event" file, just for that.
>> 'thinkpad_acpi.c' uses the 'BACKLIGHT_UPDATE_SYSFS'.
>> Henrique, can we remove it?
>
> Can't you fix this by rate-limiting, or otherwise adding an attribute that
> backlight devices should set when they need to supress change events?
Yeah, great idea, fix a bad hack with another bad one on top. :)
Passing measurement data through uevents is just an utterly broken
idea which cannot be fixed.
> Is there a proper on-screen-display support path for the backlight class
> nowadays? Otherwise, you'd be removing the only way userspace ever had to
> do proper OSD of backlight changes...
OSD drawing and event sounds usually happen as a fedback for
keypresses of brightness control, it would be weird to show up when
something else, like a light-sensor, adjusts the brightness in the
background.
Anyway, there might be the need for coordination and a new interface,
but uevents for measurement data need to die entirely; they make no
sense, never made any; and the sooner they are gone the better.
Kay
^ permalink raw reply
* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Kyungmin Park @ 2013-11-12 2:08 UTC (permalink / raw)
To: Kay Sievers
Cc: Henrique de Moraes Holschuh, Jingoo Han,
Henrique de Moraes Holschuh, linux-fbdev, LKML, Richard Purdie,
ibm-acpi-devel, platform-driver-x86
In-Reply-To: <CAPXgP11oHZXbzCt72yutmeF-P=GudzPb9whTpWe5n9LDqOMM+g@mail.gmail.com>
On Tue, Nov 12, 2013 at 10:19 AM, Kay Sievers <kay@vrfy.org> wrote:
> On Tue, Nov 12, 2013 at 1:56 AM, Henrique de Moraes Holschuh
> <hmh@hmh.eng.br> wrote:
>> On Tue, 12 Nov 2013, Jingoo Han wrote:
>>> On Tuesday, November 12, 2013 8:57 AM, Kyungmin Park wrote:
>>> > From: Kyungmin Park <kyungmin.park@samsung.com>
>>> >
>>> > The most mobile phones have Ambient Light Sensors and it changes brightness according lux.
>>> > It means it changes backlight brightness frequently by just writing sysfs node, so it generates uevent.
>>> >
>>> > Usually there's no user to use this backlight changes. But it forks udev worker threads and it takes
>>> > about 5ms. The main problem is that it hurts other process activities. so remove it.
>>> >
>>> > Kay said
>>> > "Uevents are for the major, low-frequent, global device state-changes,
>>> > not for carrying-out any sort of measurement data. Subsystems which
>>> > need that should use other facilities like poll()-able sysfs file or
>>> > any other subscription-based, client-tracking interface which does not
>>> > cause overhead if it isn't used. Uevents are not the right thing to
>>> > use here, and upstream udev should not paper-over broken kernel
>>> > subsystems."
>>
>> True.
>>
>> Now, let's take a look at reality: should you poll()/select() on a sysfs
>> node that doesn't suport it, it will wait until the poll/select timeout
>> happens (or EINTR happens), and userspace has absolutely NO way to detect
>> whether a sysfs node has poll/select support.
>>
>> What happens if the sysfs interface did not provide poll/select support
>> since day one, but rather added it later? Nobody will use it for a *long*
>> time, if ever... unless you actually took pains to version the sysfs
>> interface, and people actually care.
>
> If that's an issue, we can add a new "event" file, just for that.
>
>>> 'thinkpad_acpi.c' uses the 'BACKLIGHT_UPDATE_SYSFS'.
>>> Henrique, can we remove it?
>>
>> Can't you fix this by rate-limiting, or otherwise adding an attribute that
>> backlight devices should set when they need to supress change events?
>
> Yeah, great idea, fix a bad hack with another bad one on top. :)
> Passing measurement data through uevents is just an utterly broken
> idea which cannot be fixed.
>
>> Is there a proper on-screen-display support path for the backlight class
>> nowadays? Otherwise, you'd be removing the only way userspace ever had to
>> do proper OSD of backlight changes...
>
> OSD drawing and event sounds usually happen as a fedback for
> keypresses of brightness control, it would be weird to show up when
> something else, like a light-sensor, adjusts the brightness in the
> background.
>
> Anyway, there might be the need for coordination and a new interface,
> but uevents for measurement data need to die entirely; they make no
> sense, never made any; and the sooner they are gone the better.
Now power_supply, especially battery uses this scheme. it passes
battery data using uevent.
do you have any idea to kill it?
Thank you,
Kyungmin Park
^ permalink raw reply
* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Kay Sievers @ 2013-11-12 2:22 UTC (permalink / raw)
To: Kyungmin Park
Cc: Henrique de Moraes Holschuh, Jingoo Han,
Henrique de Moraes Holschuh, linux-fbdev, LKML, Richard Purdie,
ibm-acpi-devel, platform-driver-x86
In-Reply-To: <CAH9JG2VuqQoo3Xv9ED94w34Te97hmz-4tNxUQdbBJTLMM4OLOg@mail.gmail.com>
On Tue, Nov 12, 2013 at 3:08 AM, Kyungmin Park <kmpark@infradead.org> wrote:
> On Tue, Nov 12, 2013 at 10:19 AM, Kay Sievers <kay@vrfy.org> wrote:
>> On Tue, Nov 12, 2013 at 1:56 AM, Henrique de Moraes Holschuh
>> <hmh@hmh.eng.br> wrote:
>>> On Tue, 12 Nov 2013, Jingoo Han wrote:
>>>> On Tuesday, November 12, 2013 8:57 AM, Kyungmin Park wrote:
>>>> > From: Kyungmin Park <kyungmin.park@samsung.com>
>>>> >
>>>> > The most mobile phones have Ambient Light Sensors and it changes brightness according lux.
>>>> > It means it changes backlight brightness frequently by just writing sysfs node, so it generates uevent.
>>>> >
>>>> > Usually there's no user to use this backlight changes. But it forks udev worker threads and it takes
>>>> > about 5ms. The main problem is that it hurts other process activities. so remove it.
>>>> >
>>>> > Kay said
>>>> > "Uevents are for the major, low-frequent, global device state-changes,
>>>> > not for carrying-out any sort of measurement data. Subsystems which
>>>> > need that should use other facilities like poll()-able sysfs file or
>>>> > any other subscription-based, client-tracking interface which does not
>>>> > cause overhead if it isn't used. Uevents are not the right thing to
>>>> > use here, and upstream udev should not paper-over broken kernel
>>>> > subsystems."
>>>
>>> True.
>>>
>>> Now, let's take a look at reality: should you poll()/select() on a sysfs
>>> node that doesn't suport it, it will wait until the poll/select timeout
>>> happens (or EINTR happens), and userspace has absolutely NO way to detect
>>> whether a sysfs node has poll/select support.
>>>
>>> What happens if the sysfs interface did not provide poll/select support
>>> since day one, but rather added it later? Nobody will use it for a *long*
>>> time, if ever... unless you actually took pains to version the sysfs
>>> interface, and people actually care.
>>
>> If that's an issue, we can add a new "event" file, just for that.
>>
>>>> 'thinkpad_acpi.c' uses the 'BACKLIGHT_UPDATE_SYSFS'.
>>>> Henrique, can we remove it?
>>>
>>> Can't you fix this by rate-limiting, or otherwise adding an attribute that
>>> backlight devices should set when they need to supress change events?
>>
>> Yeah, great idea, fix a bad hack with another bad one on top. :)
>> Passing measurement data through uevents is just an utterly broken
>> idea which cannot be fixed.
>>
>>> Is there a proper on-screen-display support path for the backlight class
>>> nowadays? Otherwise, you'd be removing the only way userspace ever had to
>>> do proper OSD of backlight changes...
>>
>> OSD drawing and event sounds usually happen as a fedback for
>> keypresses of brightness control, it would be weird to show up when
>> something else, like a light-sensor, adjusts the brightness in the
>> background.
>>
>> Anyway, there might be the need for coordination and a new interface,
>> but uevents for measurement data need to die entirely; they make no
>> sense, never made any; and the sooner they are gone the better.
>
> Now power_supply, especially battery uses this scheme. it passes
> battery data using uevent.
> do you have any idea to kill it?
It should be removed too, the same applies to power_supply as to everything
else; uevents are a broken interface for any kind of device data which
is not meant as a trigger to re-configure the device itself.
But power_supply events are at least not as unfixable as backlight,
the number of events can be kept relatively low during normal
operation. So it can happen after the backlight thing is sorted out.
Note: The same rule as for generating uevents applies also to device
properties exported in the environment too; measurement data has no
place there. Reading the "uevent" file of a battery in sysfs (we need to
do that at bootup) sometimes synchronously blocks 1 second to return,
just because it tries to add measurement data reading it live from the
hardware to add it to the event itself.
Kay
^ 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