linux-kernel - Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3450618.hbnl5lRf5h@avalon>
Date:   Tue, 22 Nov 2016 20:23:38 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        David Airlie <airlied@...ux.ie>,
        Archit Taneja <architt@...eaurora.org>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>
Subject: Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

Hi John,

(CC'ing Daniel)

On Tuesday 22 Nov 2016 10:07:53 John Stultz wrote:
> On Tue, Nov 22, 2016 at 9:38 AM, John Stultz <john.stultz@...aro.org> wrote:
> > Interestingly, without the msleep added in this patch, removing the
> > wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
> > and using the polling loop seems to make things just as reliable. So
> > maybe something is off with the irq handling here instead?
> 
> Ahhhh.. So I think the trouble here is the that when we fail waiting
> for the irq, the backtrace is as follows:
> 
> [    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
> [    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
> [    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
> [    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
> [    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
> [    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
> [    8.318700] [<ffffff8008534794>] adv7511_connector_get_modes+0x14/0x20
> [    8.318710] [<ffffff8008500a54>]
> drm_helper_probe_single_connector_modes+0x2bc/0x500
> [    8.318718] [<ffffff800850e400>] drm_fb_helper_hotplug_event+0x130/0x188
> [    8.318726] [<ffffff800850ee68>] drm_fbdev_cma_hotplug_event+0x10/0x20
> [    8.318733] [<ffffff8008535718>]
> kirin_fbdev_output_poll_changed+0x20/0x58
> [    8.318740] [<ffffff8008500cc0>] drm_kms_helper_hotplug_event+0x28/0x38
> [    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
> [    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
> [    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
> [    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
> [    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
> [    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
> [    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50
> 
> So we're actually in irq handling the hotplug interrupt, which is why
> we never get the irq notification when the edid is read.
> 
> I suspect we need to use a workqueue to do the hotplug handling out of irq.

Lovely :-)

Quoting the DRM documentation:

/**
 * drm_helper_hpd_irq_event - hotplug processing
 * @dev: drm_device
 *
 * Drivers can use this helper function to run a detect cycle on all 
connectors
 * which have the DRM_CONNECTOR_POLL_HPD flag set in their &polled member. All
 * other connectors are ignored, which is useful to avoid reprobing fixed
 * panels.
 *
 * This helper function is useful for drivers which can't or don't track 
hotplug
 * interrupts for each connector.
 *
 * Drivers which support hotplug interrupts for each connector individually 
and
 * which have a more fine-grained detect logic should bypass this code and
 * directly call drm_kms_helper_hotplug_event() in case the connector state
 * changed.
 *
 * This function must be called from process context with no mode
 * setting locks held.
 *
 * Note that a connector can be both polled and probed from the hotplug 
handler,
 * in case the hotplug interrupt is known to be unreliable.
 */

So it looks like we should use drm_kms_helper_hotplug_event() instead.

/**
 * drm_kms_helper_hotplug_event - fire off KMS hotplug events
 * @dev: drm_device whose connector state changed
 *
 * This function fires off the uevent for userspace and also calls the
 * output_poll_changed function, which is most commonly used to inform the 
fbdev
 * emulation code and allow it to update the fbcon output configuration.
 *
 * Drivers should call this from their hotplug handling code when a change is
 * detected. Note that this function does not do any output detection of its
 * own, like drm_helper_hpd_irq_event() does - this is assumed to be done by 
the
 * driver already.
 *
 * This function must be called from process context with no mode
 * setting locks held.
 */

The function suffers from the same problem though, that it must be called from 
process context.

Daniel, why do we have an API the is clearly related to interrupt handling but 
requires the caller to implement a workqueue ?

-- 
Regards,

Laurent Pinchart