Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The return value of pm_runtime_get_sync() is ignored in multiple DRM bridge functions, allowing hardware access to proceed even if the device fails to power on. - [High] Display bridge atomic_enable and atomic_pre_enable callbacks call sleeping functions (usleep_range, mutex_lock), violating atomic context invariants during non-blocking commits. -- --- Patch [1]: [PATCH v9 1/2] drm: add <linux/pm_runtime.h> where missing --- commit 480a50b0cdb420e6811072e4517e397225f3a7cf Author: Vladimir Oltean <[email protected]> drm: add <linux/pm_runtime.h> where missing Multiple DRM bridge drivers use runtime PM operations without including the proper header, instead relying on transitive inclusion by <linux/phy/phy.h>. > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index 8d3d4a6e6ca25..a078bdb0b43c5 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -19,6 +19,7 @@ > #include <linux/of.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> [Severity: High] This isn't a bug introduced by this patch, but while looking at runtime PM in this file, I noticed that the return value of pm_runtime_get_sync() is ignored in several places, allowing hardware access to proceed even if the device fails to power on. For example, in analogix_dpaux_transfer(): pm_runtime_get_sync(dp->dev); ret = analogix_dp_detect_hpd(dp); And similarly in analogix_dpaux_wait_hpd_asserted() and analogix_dp_set_bridge(). If pm_runtime_get_sync() returns a negative error code, could proceeding to read or write hardware registers cause a synchronous external abort on ARM platforms? [Severity: High] This is a pre-existing issue, but I also noticed a potential problem with atomic contexts in this file. In analogix_dp_bridge_atomic_enable(), there is a sleep loop: timeout_loop++; usleep_range(10, 11); Since DRM atomic enable callbacks run in atomic context during non-blocking commits, does calling usleep_range() here risk triggering scheduling-while-atomic warnings or deadlocks? > > #include <drm/bridge/analogix_dp.h> > #include <drm/display/drm_dp_aux_bus.h> [ ... ] > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > index 504a3186ebb3a..c99151fdaf03d 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > @@ -32,6 +32,7 @@ > #include <linux/phy/phy.h> > #include <linux/phy/phy-dp.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> [Severity: High] This isn't a bug introduced by this patch, but I noticed a potential problem with atomic contexts in this driver. In cdns_mhdp_atomic_enable(), the driver attempts to acquire a mutex: mutex_lock(&mhdp->link_mutex); Since DRM atomic enable callbacks run in atomic context during non-blocking commits, might this mutex_lock() call result in scheduling-while-atomic issues when userspace triggers a non-blocking mode set? > #include <linux/slab.h> > #include <linux/wait.h> > [ ... ] > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > b/drivers/gpu/drm/bridge/samsung-dsim.c > index e2fc69fc51b60..59eef9697a5e5 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -20,6 +20,7 @@ > #include <linux/of.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> [Severity: High] This is a pre-existing issue, but I noticed a potential problem with atomic contexts in this driver. The samsung_dsim_init_link() function, which is called from samsung_dsim_atomic_pre_enable(), uses usleep_range(): samsung_dsim_write(dsi, DSIM_FIFOCTRL_REG, reg); usleep_range(9000, 11000); Since DRM atomic pre-enable callbacks run in atomic context during non-blocking commits, could this sleep sequence cause system instability or deadlocks? > #include <linux/units.h> > > #include <video/mipi_display.h> [ ... ] > diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c > b/drivers/gpu/drm/rockchip/cdn-dp-core.c > index 68aa66915e8ac..a78de7eabd151 100644 > --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c > +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c > @@ -10,6 +10,7 @@ > #include <linux/firmware.h> > #include <linux/mfd/syscon.h> > #include <linux/phy/phy.h> > +#include <linux/pm_runtime.h> [Severity: High] This isn't a bug introduced by this patch, but I noticed a potential problem with atomic contexts in this driver. In cdn_dp_bridge_atomic_enable(), the driver attempts to acquire a mutex: mutex_lock(&dp->lock); Given that DRM atomic enable callbacks run in atomic context during non-blocking commits, is there a risk of scheduling-while-atomic bugs occurring from this mutex_lock() call? > #include <linux/regmap.h> > #include <linux/reset.h> > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
