[V2] drm/bridge: adv7511: Fix Intermittent EDID failures

Commit Message

Adam Ford June 1, 2024, 1:24 p.m. UTC

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 <[e-mail]>
Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
Signed-off-by: Adam Ford <[e-mail]>
Tested-by: Liu Ying <[e-mail]> # 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.

Comments

Adam Ford June 9, 2024, 5:41 p.m. UTC | #1

On Sat, Jun 1, 2024 at 8:25 AM Adam Ford <[e-mail]> 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 <[e-mail]>
> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> Signed-off-by: Adam Ford <[e-mail]>
> Tested-by: Liu Ying <[e-mail]> # i.MX8MP EVK ADV7535 EDID retrieval w/o IRQ

Gentile nudge on this.  It would be nice to get this fix applied since
it caused a regression.

adam

> ---
> 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;
>  }
>
>  /* -----------------------------------------------------------------------------
> --
> 2.43.0
>

Linux regression tracking (Thorsten Leemhuis) June 17, 2024, 1 p.m. UTC | #2

[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?

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 <[e-mail]>
> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> Signed-off-by: Adam Ford <[e-mail]>
> Tested-by: Liu Ying <[e-mail]> # 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;
>  }
>  
>  /* -----------------------------------------------------------------------------

Adam Ford June 17, 2024, 1:14 p.m. UTC | #3

On Mon, Jun 17, 2024 at 8:00 AM Linux regression tracking (Thorsten
Leemhuis) <[e-mail]> 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/[e-mail]/T/
[2] - https://lore.kernel.org/all/[e-mail]/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 <[e-mail]>
> > Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> > Signed-off-by: Adam Ford <[e-mail]>
> > Tested-by: Liu Ying <[e-mail]> # 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;
> >  }
> >
> >  /* -----------------------------------------------------------------------------

Linux regression tracking (Thorsten Leemhuis) June 17, 2024, 1:29 p.m. UTC | #4

On 17.06.24 15:14, Adam Ford wrote:
> On Mon, Jun 17, 2024 at 8:00 AM Linux regression tracking (Thorsten
> Leemhuis) <[e-mail]> 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 [...]

Ohh, I'm very sorry, stupid me somehow missed that the Adam that was
posting the fix was the same Adam that authored the culprit. :-( Seems I
definitely need more coffee (or green tea in my case) or reduce the
number or regressions on the stack. Please accept my apologies.

Thx for the update anyway.

> 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.

So in that case the main question afaics is why this fix did not make
any progress for more than two weeks now (at least afaics -- or did I
miss something in that area, too?).

Ciao, Thorsten

>> 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 <[e-mail]>
>>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
>>> Signed-off-by: Adam Ford <[e-mail]>
>>> Tested-by: Liu Ying <[e-mail]> # 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;
>>>  }
>>>
>>>  /* -----------------------------------------------------------------------------
> 
>

Adam Ford June 17, 2024, 1:41 p.m. UTC | #5

On Mon, Jun 17, 2024 at 8:29 AM Linux regression tracking (Thorsten
Leemhuis) <[e-mail]> wrote:
>
> On 17.06.24 15:14, Adam Ford wrote:
> > On Mon, Jun 17, 2024 at 8:00 AM Linux regression tracking (Thorsten
> > Leemhuis) <[e-mail]> 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 [...]
>
> Ohh, I'm very sorry, stupid me somehow missed that the Adam that was
> posting the fix was the same Adam that authored the culprit. :-( Seems I
> definitely need more coffee (or green tea in my case) or reduce the
> number or regressions on the stack. Please accept my apologies.
>
> Thx for the update anyway.

No problem.  Sent out a few e-mails and/or patches while tired and I
when I read them again when I was awake, I had to ask myself 'what
what was I thinking'

>
> > 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.
>
> So in that case the main question afaics is why this fix did not make
> any progress for more than two weeks now (at least afaics -- or did I
> miss something in that area, too?).

I have not seen anything either which is why I sent out the gentle
nudge last week.

adam
>
> Ciao, Thorsten
>
> >> 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 <[e-mail]>
> >>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> >>> Signed-off-by: Adam Ford <[e-mail]>
> >>> Tested-by: Liu Ying <[e-mail]> # 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;
> >>>  }
> >>>
> >>>  /* -----------------------------------------------------------------------------
> >
> >

Dmitry Baryshkov June 20, 2024, 8:22 p.m. UTC | #6

On Sat, Jun 01, 2024 at 08:24:59AM GMT, 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 <[e-mail]>
> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> Signed-off-by: Adam Ford <[e-mail]>
> Tested-by: Liu Ying <[e-mail]> # 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);

There is no need for the ret variable, regmap_read can return either 0
or a negative error code.

With that fixed:


Reviewed-by: Dmitry Baryshkov <[e-mail]>


> +	if (ret < 0)
> +		return irq_status;
>  
>  	/*
>  	 * ADV7511_REG_CEC_RX_STATUS[5:0] contains the reception order of RX

Patch

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;
 }
 
 /* -----------------------------------------------------------------------------