Re: [PATCH for drm-misc-fixes 2/2] drm/hisilicon/hibmc: Add a flag to indicate whether the OS-side driver has been loaded

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Directly writing to the `HIBMC_ENABLE_FLAG` register clobbers the `GPIO_DATA_DIRECTION` I2C bits and creates a data race.
- [High] The disable flag is set too early in the teardown sequence, allowing the BMC to re-enable DP hardware while the OS driver is still active.
--

--- Patch [2]: [PATCH for drm-misc-fixes 2/2] drm/hisilicon/hibmc: Add a flag to indicate whether the OS-side driver has been loaded ---
commit bfee57e8491146f1282ee8dc01bf699cf1bf50ed
Author: Lin He <helin52@huawei.com>

drm/hisilicon/hibmc: Add a flag to indicate whether the OS-side driver has been loaded

This patch adds a flag to indicate whether the OS-side driver has been
loaded to prevent the BMC from enabling DP if the driver is not loaded,
which could lead to system failure in handling interrupts.

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index e5cca7b63b787..6436263a6c039 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c

[ ... ]

> @@ -424,6 +424,16 @@ static int hibmc_load(struct drm_device *dev)
>  	return ret;
>  }
>  
> +static inline void hibmc_set_enable_flag(struct hibmc_drm_private *priv)
> +{
> +	writel(HIBMC_ENABLE_STATE, priv->mmio + HIBMC_ENABLE_FLAG);
> +}
> +
> +static inline void hibmc_set_disable_flag(struct hibmc_drm_private *priv)
> +{
> +	writel(0, priv->mmio + HIBMC_ENABLE_FLAG);
> +}

[Severity: High]
Does this direct writel() overwrite the I2C bus state?

The HIBMC_ENABLE_FLAG offset is 0x802A4, which aliases the
GPIO_DATA_DIRECTION I2C register used by the driver's I2C bit-banging code.

Writing directly instead of using a read-modify-write clears the I2C
mask bits that might be currently active. Direct writing of 0 clears both
the enable bit and the I2C direction bits.

>  static int hibmc_pci_probe(struct pci_dev *pdev,
>  			   const struct pci_device_id *ent)
>  {

[ ... ]

> @@ -468,6 +478,8 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
>  
>  	drm_client_setup(dev, NULL);
>  
> +	hibmc_set_enable_flag(priv);
> +

[Severity: High]
Can this call to hibmc_set_enable_flag() race with I2C EDID reads?

Calling this immediately after drm_client_setup() means fbdev initialization
can asynchronously probe connectors and trigger I2C bit-banging.

Without locking between the I2C code's read-modify-write cycle and this
direct write, the I2C code might overwrite and clear the enable flag,
reverting the BMC driver status this patch intended to set.

>  	return 0;
>  
>  err_unload:
> @@ -480,6 +492,7 @@ static void hibmc_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
>  
> +	hibmc_set_disable_flag(to_hibmc_drm_private(dev));

[Severity: High]
Is it safe to clear the disable flag before userspace is locked out and the
hardware is quiesced?

By clearing the flag before calling drm_dev_unregister() and hibmc_unload(),
the BMC might attempt to take over the DP hardware or ignore interrupts
while the OS is still actively managing it. This could race with the shutdown
of the display hardware in hibmc_unload().

>  	drm_dev_unregister(dev);
>  	hibmc_unload(dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714132051.993356-1-shiyongbang@huawei.com?part=2