Re: [PATCH v4 04/10] drm/mediatek: mtk_dsi: Enable interrupt at component bind time

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/13/26 16:43, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing cleanup of DRM encoder when device_reset_optional() fails in mtk_dsi_bind().
> - [High] Read-modify-write data race on `dsi->irq_data` between process context and hardirq context.
> --
> 
> --- Patch [4]: [PATCH v4 04/10] drm/mediatek: mtk_dsi: Enable interrupt at component bind time ---
> commit 0607d3868a389c4103df3225d2bc7a47d44df3b0
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> drm/mediatek: mtk_dsi: Enable interrupt at component bind time
> 
> This patch requests the DSI interrupt with IRQF_NO_AUTOEN and enables it only
> when binding the DSI to its display controller component master. This prevents
> interrupt storms and delays caused by timeouts during probe.
> 
>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
>> index 8ab5c3431dbbc..e24e7b91a3618 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> [ ... ]
>> @@ -1090,7 +1091,13 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
>>   	if (ret)
>>   		return ret;
>>   
>> -	return device_reset_optional(dev);
>> +	ret = device_reset_optional(dev);
>> +	if (ret)
>> +		return ret;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this error path leak
> the DRM encoder?
> 

That's right. I will send a patch to fix this outside of this series.

This series shall not be blocked by issues that were already present before.

> If device_reset_optional(dev) fails, it returns without calling
> drm_encoder_cleanup(&dsi->encoder). Because the component bind operation
> fails, the component framework will not invoke mtk_dsi_unbind(), meaning
> the encoder is never cleaned up.
> 
> If the DSI driver is subsequently unbound or removed, its locally managed
> memory is freed while the encoder is still linked in the DRM core's list,
> which could lead to a use-after-free or kernel panic when the list is
> traversed.
> 
>> +
>> +	enable_irq(dsi->irq);
>> +
>> +	return 0;
>>   }
> [ ... ]
>> @@ -1375,17 +1383,15 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>>   
>>   	platform_set_drvdata(pdev, dsi);
>>   
>> +	ret = devm_request_irq(&pdev->dev, dsi->irq, mtk_dsi_irq,
>> +			       IRQF_NO_AUTOEN, dev_name(&pdev->dev), dsi);
> 
> [Severity: High]
> This is a pre-existing issue, but is there a read-modify-write data race on
> dsi->irq_data between process context and hardirq context?
> 
> mtk_dsi_irq_data_clear() modifies dsi->irq_data using &= in process context
> (from mtk_dsi_host_transfer()) without disabling interrupts or using atomic
> bitwise operations. Concurrently, the hardirq handler mtk_dsi_irq() modifies
> the same variable using |=.
> 
> If a DSI interrupt fires during the process-context read-modify-write
> sequence, won't the flag set by the interrupt handler be silently
> overwritten and lost when the process context stores its computed value?
> 
> This could lead to hardware interrupt events being lost, causing timeouts
> in mtk_dsi_wait_for_irq_done() and potentially breaking display
> initialization or frame updates.
> 

That doesn't really happen for now, but it's a fair point. Will send a patch
to fix outside of this series.

This was not introduced by this series and shall not block it.

>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "Failed to request DSI irq\n");
>> +
>