possible issue in mtk_pcie_power_up()

possible issue in mtk_pcie_power_up()

[Bjorn Helgaas]

Sashiko noticed this when reviewing another patchset:
https://sashiko.dev/#/patchset/20260430162420.42839-1-18255117159%40163.com
I'm just forwarding to you as an FYI.

  Author: Hans Zhang <18255117159@163.com>
  PCI: mediatek-gen3: Use FIELD_MODIFY()

  This commit replaces open-coded bit manipulation with FIELD_MODIFY() in the
  Mediatek Gen3 PCIe driver. It intends to introduce no functional changes.

  > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
  > index b0accd8285892..f36a616a8b52e 100644
  > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
  > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
  > @@ -494,8 +494,7 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
  >  	/* Set Link Control 2 (LNKCTL2) speed restriction, if any */
  >  	if (pcie->max_link_speed) {
  >  		val = readl_relaxed(pcie->base + PCIE_CONF_LINK2_CTL_STS);
  > -		val &= ~PCIE_CONF_LINK2_LCR2_LINK_SPEED;
  > -		val |= FIELD_PREP(PCIE_CONF_LINK2_LCR2_LINK_SPEED, pcie->max_link_speed);
  > +		FIELD_MODIFY(PCIE_CONF_LINK2_LCR2_LINK_SPEED, &val, pcie->max_link_speed);
  >  		writel_relaxed(val, pcie->base + PCIE_CONF_LINK2_CTL_STS);
  >  	}

  This isn't an issue introduced by this patch, but while reviewing
  the driver I noticed a pre-existing issue in mtk_pcie_power_up() and
  mtk_pcie_en7581_power_up().

  Both functions call pm_runtime_get_sync(dev) without checking its
  return value. If the device fails to resume, the function continues
  execution and accesses hardware registers via readl_relaxed() and
  writel_relaxed().

  Could accessing unclocked registers here cause a synchronous
  external abort and a kernel panic?

  Additionally, pm_runtime_get_sync() increments the device usage
  count even on failure. Since the current error paths do not call
  pm_runtime_put_noidle(), does this leak the PM reference count?

  Would it be better to use pm_runtime_resume_and_get(dev) instead,
  and cleanly abort the initialization on failure?