* [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
@ 2024-05-21 1:16 Adam Ford
2024-05-21 21:12 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Adam Ford @ 2024-05-21 1:16 UTC (permalink / raw)
To: dri-devel
Cc: dmitry.baryshkov, ictor.liu, sui.jingfeng, aford, Adam Ford,
Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, linux-kernel
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@nxp.com>
Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
Signed-off-by: Adam Ford <aford173@gmail.com>
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
2024-05-21 1:16 [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures Adam Ford
@ 2024-05-21 21:12 ` kernel test robot
2024-05-21 21:51 ` Adam Ford
2024-05-22 9:48 ` Dmitry Baryshkov
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2024-05-21 21:12 UTC (permalink / raw)
To: Adam Ford, dri-devel
Cc: llvm, oe-kbuild-all, dmitry.baryshkov, ictor.liu, sui.jingfeng,
aford, Adam Ford, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Daniel Vetter, linux-kernel
Hi Adam,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-tip/drm-tip linus/master drm-intel/for-linux-next-fixes v6.9 next-20240521]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Adam-Ford/drm-bridge-adv7511-Fix-Intermittent-EDID-failures/20240521-091841
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20240521011614.496421-1-aford173%40gmail.com
patch subject: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
config: i386-buildonly-randconfig-002-20240521 (https://download.01.org/0day-ci/archive/20240522/202405220445.DW2U7DXR-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240522/202405220445.DW2U7DXR-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405220445.DW2U7DXR-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c:507:35: warning: variable 'cec_status' is uninitialized when used here [-Wuninitialized]
507 | if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
| ^~~~~~~~~~
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c:472:16: note: initialize the variable 'cec_status' to silence this warning
472 | int cec_status;
| ^
| = 0
1 warning generated.
vim +/cec_status +507 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
505
506 /* If there is no IRQ to handle, exit indicating no IRQ data */
> 507 if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
508 return IRQ_HANDLED;
509
510 return IRQ_NONE;
511 }
512
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
2024-05-21 1:16 [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures Adam Ford
2024-05-21 21:12 ` kernel test robot
@ 2024-05-21 21:51 ` Adam Ford
2024-05-22 1:58 ` Liu Ying
2024-05-22 9:48 ` Dmitry Baryshkov
2 siblings, 1 reply; 6+ messages in thread
From: Adam Ford @ 2024-05-21 21:51 UTC (permalink / raw)
To: dri-devel
Cc: dmitry.baryshkov, sui.jingfeng, aford, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, linux-kernel,
Liu Ying
On Mon, May 20, 2024 at 8:16 PM Adam Ford <aford173@gmail.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@nxp.com>
> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> Signed-off-by: Adam Ford <aford173@gmail.com>
+ Liu
Sorry about the e-mail address copy-paste error.
>
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
2024-05-21 21:51 ` Adam Ford
@ 2024-05-22 1:58 ` Liu Ying
0 siblings, 0 replies; 6+ messages in thread
From: Liu Ying @ 2024-05-22 1:58 UTC (permalink / raw)
To: Adam Ford, dri-devel
Cc: dmitry.baryshkov, sui.jingfeng, aford, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, linux-kernel
On 5/22/24 05:51, Adam Ford wrote:
> On Mon, May 20, 2024 at 8:16 PM Adam Ford <aford173@gmail.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@nxp.com>
s/Reported by/Reported-by/
>> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
>> Signed-off-by: Adam Ford <aford173@gmail.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@nxp.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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
2024-05-21 1:16 [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures Adam Ford
2024-05-21 21:12 ` kernel test robot
2024-05-21 21:51 ` Adam Ford
@ 2024-05-22 9:48 ` Dmitry Baryshkov
2 siblings, 0 replies; 6+ messages in thread
From: Dmitry Baryshkov @ 2024-05-22 9:48 UTC (permalink / raw)
To: Adam Ford
Cc: dri-devel, ictor.liu, sui.jingfeng, aford, Andrzej Hajda,
Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Daniel Vetter, linux-kernel
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@nxp.com>
> Fixes: f3d9683346d6 ("drm/bridge: adv7511: Allow IRQ to share GPIO pins")
> Signed-off-by: Adam Ford <aford173@gmail.com>
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
@ 2024-05-22 14:55 kernel test robot
0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2024-05-22 14:55 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Dan Carpenter
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240521011614.496421-1-aford173@gmail.com>
References: <20240521011614.496421-1-aford173@gmail.com>
TO: Adam Ford <aford173@gmail.com>
TO: dri-devel@lists.freedesktop.org
CC: dmitry.baryshkov@linaro.org
CC: ictor.liu@nxp.com
CC: sui.jingfeng@linux.dev
CC: aford@beaconembedded.com
CC: Adam Ford <aford173@gmail.com>
CC: Andrzej Hajda <andrzej.hajda@intel.com>
CC: Neil Armstrong <neil.armstrong@linaro.org>
CC: Robert Foss <rfoss@kernel.org>
CC: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
CC: Jonas Karlman <jonas@kwiboo.se>
CC: Jernej Skrabec <jernej.skrabec@gmail.com>
CC: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
CC: Maxime Ripard <mripard@kernel.org>
CC: Thomas Zimmermann <tzimmermann@suse.de>
CC: David Airlie <airlied@gmail.com>
CC: Daniel Vetter <daniel@ffwll.ch>
CC: linux-kernel@vger.kernel.org
Hi Adam,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-tip/drm-tip linus/master drm-intel/for-linux-next-fixes v6.9 next-20240522]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Adam-Ford/drm-bridge-adv7511-Fix-Intermittent-EDID-failures/20240521-091841
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20240521011614.496421-1-aford173%40gmail.com
patch subject: [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: i386-randconfig-141-20240522 (https://download.01.org/0day-ci/archive/20240522/202405222253.p3vRbJDH-lkp@intel.com/config)
compiler: gcc-9 (Ubuntu 9.5.0-4ubuntu2) 9.5.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202405222253.p3vRbJDH-lkp@intel.com/
smatch warnings:
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c:507 adv7511_irq_process() error: uninitialized symbol 'cec_status'.
vim +/cec_status +507 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
3b1b975003e4a3 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c Hans Verkuil 2017-10-07 505
2d0835b065781a drivers/gpu/drm/bridge/adv7511/adv7511_drv.c Adam Ford 2024-05-20 506 /* If there is no IRQ to handle, exit indicating no IRQ data */
2d0835b065781a drivers/gpu/drm/bridge/adv7511/adv7511_drv.c Adam Ford 2024-05-20 @507 if (irq_status == IRQ_HANDLED || cec_status == IRQ_HANDLED)
2d0835b065781a drivers/gpu/drm/bridge/adv7511/adv7511_drv.c Adam Ford 2024-05-20 508 return IRQ_HANDLED;
2d0835b065781a drivers/gpu/drm/bridge/adv7511/adv7511_drv.c Adam Ford 2024-05-20 509
2d0835b065781a drivers/gpu/drm/bridge/adv7511/adv7511_drv.c Adam Ford 2024-05-20 510 return IRQ_NONE;
a5241289c4139f drivers/gpu/drm/i2c/adv7511.c Laurent Pinchart 2015-02-18 511 }
9c8af882bf1230 drivers/gpu/drm/i2c/adv7511.c Lars-Peter Clausen 2012-03-05 512
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-22 14:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-21 1:16 [PATCH] drm/bridge: adv7511: Fix Intermittent EDID failures Adam Ford
2024-05-21 21:12 ` kernel test robot
2024-05-21 21:51 ` Adam Ford
2024-05-22 1:58 ` Liu Ying
2024-05-22 9:48 ` Dmitry Baryshkov
-- strict thread matches above, loose matches on Subject: below --
2024-05-22 14:55 kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.