On Mon, May 20, 2024 at 08:16:14PM -0500, 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@xxxxxxx> > Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > Signed-off-by: Adam Ford <aford173@xxxxxxxxx> > > 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; Ok, maybe I was wrong with my previous suggestion. The code starts to look more and more clumsy. Do we really care about error status at all? Maybe it's enough to return IRQ_NONE here from the IRQ handlers? > > /* > * 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; cec_status ends up being unset if CEC is disabled. > + 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); This should be return ret < 0 ? IRQ_NONE : ret. We should not be returning negative error via irqreturn_t. > } > > /* ----------------------------------------------------------------------------- > -- > 2.43.0 > -- With best wishes Dmitry