linux-kernel - Re: [PATCH 3/3] PCI: dw-rockchip: Unify link status checks with FIELD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aAeAAhb4R8ya_mBO@ryzen>
Date: Tue, 22 Apr 2025 13:39:46 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Hans Zhang <18255117159@....com>
Cc: lpieralisi@...nel.org, kw@...ux.com, bhelgaas@...gle.com,
	heiko@...ech.de, manivannan.sadhasivam@...aro.org, robh@...nel.org,
	jingoohan1@...il.com, shawn.lin@...k-chips.com,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH 3/3] PCI: dw-rockchip: Unify link status checks with
 FIELD_GET

On Tue, Apr 22, 2025 at 07:28:30PM +0800, Hans Zhang wrote:
> Link-up detection manually checked PCIE_LINKUP bits across RC/EP modes,
> leading to code duplication. Centralize the logic using FIELD_GET. This
> removes redundancy and abstracts hardware-specific bit masking, ensuring
> consistent link state handling.
> 
> Signed-off-by: Hans Zhang <18255117159@....com>
> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index cdc8afc6cfc1..2b26060af5c2 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -196,10 +196,7 @@ static int rockchip_pcie_link_up(struct dw_pcie *pci)
>  	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
>  	u32 val = rockchip_pcie_get_ltssm(rockchip);
>  
> -	if ((val & PCIE_LINKUP) == PCIE_LINKUP)
> -		return 1;
> -
> -	return 0;
> +	return FIELD_GET(PCIE_LINKUP_MASK, val) == 3;

While I like the idea of your patch, here you are replacing something that
is easy to read (PCIE_LINKUP) with a magic value, which IMO is a step in
the wrong direction.


>  }
>  
>  static void rockchip_pcie_enable_l0s(struct dw_pcie *pci)
> @@ -499,7 +496,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
>  	struct dw_pcie *pci = &rockchip->pci;
>  	struct dw_pcie_rp *pp = &pci->pp;
>  	struct device *dev = pci->dev;
> -	u32 reg, val;
> +	u32 reg;
>  
>  	reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
>  	rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
> @@ -508,8 +505,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
>  	dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
>  
>  	if (reg & PCIE_RDLH_LINK_UP_CHGED) {
> -		val = rockchip_pcie_get_ltssm(rockchip);
> -		if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
> +		if (rockchip_pcie_link_up(pci)) {
>  			dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
>  			/* Rescan the bus to enumerate endpoint devices */
>  			pci_lock_rescan_remove();
> @@ -526,7 +522,7 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
>  	struct rockchip_pcie *rockchip = arg;
>  	struct dw_pcie *pci = &rockchip->pci;
>  	struct device *dev = pci->dev;
> -	u32 reg, val;
> +	u32 reg;
>  
>  	reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
>  	rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
> @@ -540,8 +536,7 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
>  	}
>  
>  	if (reg & PCIE_RDLH_LINK_UP_CHGED) {
> -		val = rockchip_pcie_get_ltssm(rockchip);
> -		if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
> +		if (rockchip_pcie_link_up(pci)) {
>  			dev_dbg(dev, "link up\n");
>  			dw_pcie_ep_linkup(&pci->ep);
>  		}
> -- 
> 2.25.1
>