On Mon, Jun 17, 2024 at 8:00 AM Linux regression tracking (Thorsten Leemhuis) <regressions@xxxxxxxxxxxxx> 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@xxxxxxx/T/ [2] - https://lore.kernel.org/all/7bb4d582-5d90-465e-a241-1ee8439a5057@xxxxxxx/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@xxxxxxx> > > Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins") > > Signed-off-by: Adam Ford <aford173@xxxxxxxxx> > > Tested-by: Liu Ying <victor.liu@xxxxxxx> # 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; > > } > > > > /* -----------------------------------------------------------------------------
