| 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
| ||
|
Message-ID: <CAHCN7xJfqcN=yqWAURuy-oF8EiwoB5i840Gct65xgqgxNSL_Ug@mail.gmail.com>
Date: Mon, 17 Jun 2024 08:14:41 -0500
From: Adam Ford <aford173@...il.com>
To: Linux regressions mailing list <regressions@...ts.linux.dev>
Cc: dmitry.baryshkov@...aro.org, victor.liu@....com, 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, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH V2] drm/bridge: adv7511: Fix Intermittent EDID failures
On Mon, Jun 17, 2024 at 8:00 AM Linux regression tracking (Thorsten
Leemhuis) <regressions@...mhuis.info> wrote:
>
> [CCing the regression list, as it should be in the loop for regressions:
> https://docs.kernel.org/admin-guide/reporting-regressions.html]
>
> Hi! Top-posting for once, to make this easily accessible to everyone.
>
> Hmm, seem nobody took a look at below fix for a regression that seems to
> be caused by f3d9683346d6b1 ("drm/bridge: adv7511: Allow IRQ to share
> GPIO pins") [which went into v6.10-rc1].
>
> Adam and Dimitry, what are your stances on this patch from Adam? I'm
> asking, as you authored respectively committed the culprit?
>
I learned of the regression from Liu Ying when he posted a proposed
fix [1] which then resulted in some further discussion on how to
better solve the issue. I felt like I should be the one to fix the
issue, since I accidentally caused it when trying to allow for shared
GPIO IRQ's. The shared GPIO IRQ was required on the imx8mp-beacon-kit
because the interrupt GPIO for the hot-plug-detect IRQ is shared with
a GPIO expander which also serves as an interrupt controller.
Dimitry had given me some suggestions, and from that, I posted a V1.
Dmitry had some more followup suggestions [2] which resulted in the
V2.
As far as I know, Liu was satisfied that this addressed the regression
he reported.
adam
[1] - https://lore.kernel.org/linux-kernel/2f41a890-915b-4859-8ac9-5436da14fe22@nxp.com/T/
[2] - https://lore.kernel.org/all/7bb4d582-5d90-465e-a241-1ee8439a5057@nxp.com/T/
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
>
> #regzbot poke
>
> On 01.06.24 15:24, Adam Ford 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>
> > Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> > Signed-off-by: Adam Ford <aford173@...il.com>
> > Tested-by: Liu Ying <victor.liu@....com> # i.MX8MP EVK ADV7535 EDID retrieval w/o IRQ
> > ---
> > V2: Fix uninitialized cec_status
> > Cut back a little on error handling to return either IRQ_NONE or
> > IRQ_HANDLED.
> >
> > 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..651fb1dde780 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 irq_status;
> >
> > /*
> > * 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..c8d2c4a157b2 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 = IRQ_NONE;
> > + int irq_status = IRQ_NONE;
> >
> > ret = regmap_read(adv7511->regmap, ADV7511_REG_INT(0), &irq0);
> > if (ret < 0)
> > @@ -478,29 +480,31 @@ 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);
> > #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)
> > @@ -509,7 +513,7 @@ static irqreturn_t adv7511_irq_handler(int irq, void *devid)
> > int ret;
> >
> > ret = adv7511_irq_process(adv7511, true);
> > - return ret < 0 ? IRQ_NONE : IRQ_HANDLED;
> > + return ret < 0 ? IRQ_NONE : ret;
> > }
> >
> > /* -----------------------------------------------------------------------------
Powered by blists - more mailing lists