Re: [PATCH v8 06/14] drm/bridge: it6505: bail out of the IRQ handler when status reads fail

[Luca Ceresoli]

Hello Daniel,

> When reading the interrupt status registers fails, the negative error
> codes end up in int_status[], where it6505_test_bit() sees almost all
> bits set: every interrupt sub-handler runs on garbage and the error
> values are even written back to the status registers. Return IRQ_NONE
> instead.
>
> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
> Signed-off-by: Daniel Golle <daniel@xxxxxxxxxxxxxx>
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 6fd8f5ba9acc..f249520ebd41 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2714,6 +2714,11 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
>  	int_status[1] = it6505_read(it6505, INT_STATUS_02);
>  	int_status[2] = it6505_read(it6505, INT_STATUS_03);
>
> +	if (int_status[0] < 0 || int_status[1] < 0 || int_status[2] < 0) {
> +		pm_runtime_put_sync(dev);
> +		return IRQ_NONE;
> +	}

One first question it whether a read failure is realistic or not, and
whether you have experienced it on real hardware.

Theoretical or real, the issue is in the code, but I think a cleaner
solution exists.

Considering that:

 * it6505_read() does 2 things:
   1. check whether it6505->powered is true, else return error
   2. regmap_read()
 * it6505_int_threaded_handler() already checks for it6505->powered
   earlier, so it6505_read() is basically a regmap_read() wrapper
   cramming an error or a register value in the same return value

I would consider replacing all the 3 it6505_read() calls with a single
regmap_bulk_read(), making error management cleaner.

What do you think about this option?

Perhaps the three it6505_write() below could also become a bulk write.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com