linux-kernel - Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures

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: <7bb4d582-5d90-465e-a241-1ee8439a5057@nxp.com>
Date: Wed, 22 May 2024 09:58:09 +0800
From: Liu Ying <victor.liu@....com>
To: Adam Ford <aford173@...il.com>, dri-devel@...ts.freedesktop.org
Cc: dmitry.baryshkov@...aro.org, sui.jingfeng@...ux.dev,
 aford@...conembedded.com, Andrzej Hajda <andrzej.hajda@...el.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures

On 5/22/24 05:51, Adam Ford wrote:
> On Mon, May 20, 2024 at 8:16 PM Adam Ford <aford173@...il.com> wrote:
>>
>> In the process of adding support for shared IRQ pins, a scenario
>> was accidentally created where adv7511_irq_process returned
>> prematurely causing the EDID to fail randomly.
>>
>> Since the interrupt handler is broken up into two main helper functions,
>> update both of them to treat the helper functions as IRQ handlers. These
>> IRQ routines process their respective tasks as before, but if they
>> determine that actual work was done, mark the respective IRQ status
>> accordingly, and delay the check until everything has been processed.
>>
>> This should guarantee the helper functions don't return prematurely
>> while still returning proper values of either IRQ_HANDLED or IRQ_NONE.
>>
>> Reported by: Liu Ying <victor.liu@....com>

s/Reported by/Reported-by/

>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
>> Signed-off-by: Adam Ford <aford173@...il.com>
> 
> + Liu
> 
> Sorry about the e-mail address copy-paste error.

No worries.

With this patch, it looks EDID retrieval works ok for me without
interrupt requested.

Tested-by: Liu Ying <victor.liu@....com> # i.MX8MP EVK ADV7535 EDID retrieval w/o IRQ

>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> index ea271f62b214..ec0b7f3d889c 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
>> @@ -401,7 +401,7 @@ struct adv7511 {
>>
>>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>>  int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511);
>> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
>> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
>>  #else
>>  static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
>>  {
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>> index 44451a9658a3..4efb2cabf1b5 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
>> @@ -119,7 +119,7 @@ static void adv7511_cec_rx(struct adv7511 *adv7511, int rx_buf)
>>         cec_received_msg(adv7511->cec_adap, &msg);
>>  }
>>
>> -void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>> +int adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>>  {
>>         unsigned int offset = adv7511->info->reg_cec_offset;
>>         const u32 irq_tx_mask = ADV7511_INT1_CEC_TX_READY |
>> @@ -130,17 +130,21 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>>                                 ADV7511_INT1_CEC_RX_READY3;
>>         unsigned int rx_status;
>>         int rx_order[3] = { -1, -1, -1 };
>> -       int i;
>> +       int i, ret = 0;
>> +       int irq_status = IRQ_NONE;
>>
>> -       if (irq1 & irq_tx_mask)
>> +       if (irq1 & irq_tx_mask) {
>>                 adv_cec_tx_raw_status(adv7511, irq1);
>> +               irq_status = IRQ_HANDLED;
>> +       }
>>
>>         if (!(irq1 & irq_rx_mask))
>> -               return;
>> +               return irq_status;
>>
>> -       if (regmap_read(adv7511->regmap_cec,
>> -                       ADV7511_REG_CEC_RX_STATUS + offset, &rx_status))
>> -               return;
>> +       ret = regmap_read(adv7511->regmap_cec,
>> +                       ADV7511_REG_CEC_RX_STATUS + offset, &rx_status);
>> +       if (ret < 0)
>> +               return ret;
>>
>>         /*
>>          * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX
>> @@ -172,6 +176,8 @@ void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1)
>>
>>                 adv7511_cec_rx(adv7511, rx_buf);
>>         }
>> +
>> +       return IRQ_HANDLED;
>>  }
>>
>>  static int adv7511_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index 66ccb61e2a66..56dd2d5a0376 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -469,6 +469,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>>  {
>>         unsigned int irq0, irq1;
>>         int ret;
>> +       int cec_status;
>> +       int irq_status = IRQ_NONE;
>>
>>         ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
>>         if (ret < 0)
>> @@ -478,38 +480,41 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
>>         if (ret < 0)
>>                 return ret;
>>
>> -       /* If there is no IRQ to handle, exit indicating no IRQ data */
>> -       if (!(irq0 & (ADV7511_INT0_HPD | ADV7511_INT0_EDID_READY)) &&
>> -           !(irq1 & ADV7511_INT1_DDC_ERROR))
>> -               return -ENODATA;
>> -
>>         regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
>>         regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
>>
>> -       if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder)
>> +       if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder) {
>>                 schedule_work(&adv7511->hpd_work);
>> +               irq_status = IRQ_HANDLED;
>> +       }
>>
>>         if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
>>                 adv7511->edid_read = true;
>>
>>                 if (adv7511->i2c_main->irq)
>>                         wake_up_all(&adv7511->wq);
>> +               irq_status = IRQ_HANDLED;
>>         }
>>
>>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> -       adv7511_cec_irq_process(adv7511, irq1);
>> +       cec_status = adv7511_cec_irq_process(adv7511, irq1);
>> +
>> +       if (cec_status < 0)
>> +               return cec_status;
>>  #endif
>>
>> -       return 0;
>> +       /* If there is no IRQ to handle, exit indicating no IRQ data */
>> +       if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
>> +               return IRQ_HANDLED;
>> +
>> +       return IRQ_NONE;
>>  }
>>
>>  static irqreturn_t adv7511_irq_handler(int irq, void *devid)
>>  {
>>         struct adv7511 *adv7511 = devid;
>> -       int ret;
>>
>> -       ret = adv7511_irq_process(adv7511, true);
>> -       return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
>> +       return adv7511_irq_process(adv7511, true);
>>  }
>>
>>  /* -----------------------------------------------------------------------------
>> --
>> 2.43.0
>>