linux-kernel - Re: [PATCH V4] drm/bridge: ti-sn65dsi86: Add support for DisplayPort mode with HPD

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: <CAD=FV=X531iXsAkoGMd9Lr=wbzUx2MM9uXV_Azkj-ww6jjLaVw@mail.gmail.com>
Date: Fri, 12 Sep 2025 13:02:53 -0700
From: Doug Anderson <dianders@...omium.org>
To: John Ripple <john.ripple@...sight.com>
Cc: Laurent.pinchart@...asonboard.com, airlied@...il.com, 
	andrzej.hajda@...el.com, blake.vermeer@...sight.com, 
	dri-devel@...ts.freedesktop.org, jernej.skrabec@...il.com, jonas@...boo.se, 
	linux-kernel@...r.kernel.org, maarten.lankhorst@...ux.intel.com, 
	matt_laubhan@...sight.com, mripard@...nel.org, neil.armstrong@...aro.org, 
	rfoss@...nel.org, simona@...ll.ch, tzimmermann@...e.de
Subject: Re: [PATCH V4] drm/bridge: ti-sn65dsi86: Add support for DisplayPort
 mode with HPD

Hi,

On Fri, Sep 12, 2025 at 12:25 PM John Ripple <john.ripple@...sight.com> wrote:
>
> @@ -153,6 +164,8 @@
>   * @ln_polrs:     Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
>   * @comms_enabled: If true then communication over the aux channel is enabled.
>   * @comms_mutex:   Protects modification of comms_enabled.
> + * @hpd_enabled:   If true then HPD events are enabled.
> + * @hpd_mutex:     Protects modification of hpd_enabled.

nit: order should match the order of elements in the struct, so
hpd_enabled should be above comms_mutex.


> @@ -1219,11 +1261,33 @@ static void ti_sn_bridge_hpd_enable(struct drm_bridge *bridge)
>          */
>
>         pm_runtime_get_sync(pdata->dev);
> +
> +       if (client->irq) {
> +               /* Enable HPD events. */

nit: the comment probably doesn't buy us too much...


> +               ret = regmap_set_bits(pdata->regmap, SN_IRQ_EVENTS_EN_REG,
> +                                     HPD_REMOVAL_EN | HPD_INSERTION_EN);
> +               if (ret)
> +                       pr_err("Failed to enable HPD events: %d\n", ret);
> +       }
> +       mutex_lock(&pdata->hpd_mutex);
> +       pdata->hpd_enabled = true;
> +       mutex_unlock(&pdata->hpd_mutex);

For enable, I think you want this _above_ the setting of the bits.
Then if an interrupt fires right off the bat you'll see it. That also
matches the intuition that usually enable and disable should be mirror
images of each other (they should do things in the opposite order).


>  }
>
>  static void ti_sn_bridge_hpd_disable(struct drm_bridge *bridge)
>  {
>         struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> +       const struct i2c_client *client = to_i2c_client(pdata->dev);
> +
> +       if (client->irq) {
> +               /* Disable HPD events. */
> +               regmap_write(pdata->regmap, SN_IRQ_EVENTS_EN_REG, 0);

To make it symmetric to ti_sn_bridge_hpd_enable(), you should probably
use regmap_clear_bits(). Then if we later enable other interrupt
sources then they'll stay enabled.


> +               regmap_update_bits(pdata->regmap, SN_IRQ_EN_REG, IRQ_EN, 0);

I don't think you need to clear this. It can just stay enabled but no
interrupts will fire since there are no interrupt sources. This will
make things work right if we later enable other interrupt sources.


> @@ -1309,6 +1373,38 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
>         return 0;
>  }
>
> +static irqreturn_t ti_sn_bridge_interrupt(int irq, void *private)
> +{
> +       struct ti_sn65dsi86 *pdata = private;
> +       struct drm_device *dev = pdata->bridge.dev;
> +       u8 status;
> +       int ret;
> +       bool hpd_event = false;
> +
> +       ret = ti_sn65dsi86_read_u8(pdata, SN_IRQ_STATUS_REG, &status);
> +       if (ret)
> +               pr_err("Failed to read IRQ status: %d\n", ret);

You should not only print but return IRQ_NONE here IMO. If there are
constant errors (for whatever reason) the interrupt will eventually
get disabled by the system which is better than storming. It also
means that later code doesn't access a bogus "status".


> +       else
> +               hpd_event = status & (HPD_REMOVAL_STATUS | HPD_INSERTION_STATUS);
> +
> +       if (status) {
> +               pr_debug("(SN_IRQ_STATUS_REG = %#x)\n", status);
> +               ret = regmap_write(pdata->regmap, SN_IRQ_STATUS_REG, status);
> +               if (ret)
> +                       pr_err("Failed to clear IRQ status: %d\n", ret);
> +       } else {
> +               return IRQ_NONE;
> +       }

FWIW: Linux conventions usually are to handle error cases in the "if"
case to avoid indentation (because you don't need an "else"). AKA:

if (!status)
  return IRQ_NONE;

pr_debug(...)
ret = ...
...