Linux PCI subsystem development
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Ryder Lee <ryder.lee@mediatek.com>,
	Jianjun Wang <jianjun.wang@mediatek.com>
Cc: linux-pci@vger.kernel.org
Subject: possible issue in mtk_pcie_power_up()
Date: Tue, 5 May 2026 11:59:12 -0500	[thread overview]
Message-ID: <20260505165912.GA738617@bhelgaas> (raw)

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?

                 reply	other threads:[~2026-05-05 16:59 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260505165912.GA738617@bhelgaas \
    --to=helgaas@kernel.org \
    --cc=jianjun.wang@mediatek.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=ryder.lee@mediatek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox