linux-kernel - Re: [PATCH v2 13/13] PCI: tegra194: Refactor code by using dw_pcie_clear_and_set

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: <ci3ojmiiuajwedx3desa7vf5lkk35meaj4kdqnunf3f66knvm5@gtgxoagzl7ek>
Date: Wed, 25 Jun 2025 14:57:34 -0600
From: Manivannan Sadhasivam <mani@...nel.org>
To: Hans Zhang <18255117159@....com>
Cc: lpieralisi@...nel.org, bhelgaas@...gle.com, kwilczynski@...nel.org, 
	robh@...nel.org, jingoohan1@...il.com, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 13/13] PCI: tegra194: Refactor code by using
 dw_pcie_clear_and_set_dword()

On Wed, Jun 18, 2025 at 11:21:12PM +0800, Hans Zhang wrote:
> Tegra194 PCIe driver contains extensive manual bit manipulation across
> interrupt handling, ASPM configuration, and controller initialization.
> The driver implements complex read-modify-write sequences with explicit
> bit masking, leading to verbose and hard-to-maintain code.
> 
> Refactor interrupt handling, ASPM setup, capability configuration, and
> controller initialization using dw_pcie_clear_and_set_dword(). Replace
> multi-step register modifications with single helper calls, eliminating
> intermediate variables and reducing code size by ~100 lines. For CDMA
> error handling, initialize the value variable to zero before setting
> status bits.
> 
> This comprehensive refactoring significantly improves code readability
> and maintainability. Standardizing on the helper ensures consistent
> register access patterns across all driver components and reduces the
> risk of bit manipulation errors in this complex controller driver.
> 
> Signed-off-by: Hans Zhang <18255117159@....com>
> ---
>  drivers/pci/controller/dwc/pcie-tegra194.c | 155 +++++++++------------
>  1 file changed, 64 insertions(+), 91 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 4f26086f25da..c6f5c35a4be4 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -378,9 +378,8 @@ static irqreturn_t tegra_pcie_rp_irq_handler(int irq, void *arg)
>  			val |= APPL_CAR_RESET_OVRD_CYA_OVERRIDE_CORE_RST_N;
>  			appl_writel(pcie, val, APPL_CAR_RESET_OVRD);
>  
> -			val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> -			val |= PORT_LOGIC_SPEED_CHANGE;
> -			dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
> +			dw_pcie_clear_and_set_dword(pci, PCIE_LINK_WIDTH_SPEED_CONTROL,
> +						    0, PORT_LOGIC_SPEED_CHANGE);
>  		}
>  	}
>  
> @@ -412,7 +411,7 @@ static irqreturn_t tegra_pcie_rp_irq_handler(int irq, void *arg)
>  
>  	if (status_l0 & APPL_INTR_STATUS_L0_CDM_REG_CHK_INT) {
>  		status_l1 = appl_readl(pcie, APPL_INTR_STATUS_L1_18);
> -		val = dw_pcie_readl_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS);
> +		val = 0;
>  		if (status_l1 & APPL_INTR_STATUS_L1_18_CDM_REG_CHK_CMPLT) {
>  			dev_info(pci->dev, "CDM check complete\n");
>  			val |= PCIE_PL_CHK_REG_CHK_REG_COMPLETE;
> @@ -425,7 +424,8 @@ static irqreturn_t tegra_pcie_rp_irq_handler(int irq, void *arg)
>  			dev_err(pci->dev, "CDM Logic error\n");
>  			val |= PCIE_PL_CHK_REG_CHK_REG_LOGIC_ERROR;
>  		}
> -		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
> +		dw_pcie_clear_and_set_dword(pci, PCIE_PL_CHK_REG_CONTROL_STATUS,
> +					    PORT_LOGIC_SPEED_CHANGE, val);

I don't know why you are clearing PORT_LOGIC_SPEED_CHANGE here which is not part
of PCIE_PL_CHK_REG_CONTROL_STATUS. Typo?

- Mani

-- 
மணிவண்ணன் சதாசிவம்