linux-kernel - Re: [PATCH v4 02/13] PCI: dwc: 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: <wi2mylqrf6szc5iluncle2lj373aoxu46lyy7d2gaqx4dv3abq@sja5aj5mwv3j>
Date: Wed, 27 Aug 2025 19:21:51 +0530
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 v4 02/13] PCI: dwc: Refactor code by using
 dw_pcie_clear_and_set_dword()

On Wed, Aug 13, 2025 at 12:45:20PM GMT, Hans Zhang wrote:
> DesignWare core modules contain multiple instances of manual
> read-modify-write operations for register bit manipulation.
> These patterns duplicate functionality now provided by
> dw_pcie_clear_and_set_dword(), particularly in debugfs, endpoint,
> host, and core initialization paths.
> 
> Replace open-coded bit manipulation sequences with calls to
> dw_pcie_clear_and_set_dword(). Affected areas include debugfs register
> control, endpoint capability configuration, host setup routines, and
> core link initialization logic. The changes simplify power management
> handling, capability masking, and feature configuration.
> 
> Standardizing on the helper function reduces code duplication by ~140
> lines across core modules while improving readability. The refactoring
> also ensures consistent error handling for register operations and
> provides a single point of control for future bit manipulation logi
> updates.
> 
> Signed-off-by: Hans Zhang <18255117159@....com>
> ---
>  .../controller/dwc/pcie-designware-debugfs.c  | 66 +++++++---------
>  .../pci/controller/dwc/pcie-designware-ep.c   | 20 +++--
>  .../pci/controller/dwc/pcie-designware-host.c | 26 +++----
>  drivers/pci/controller/dwc/pcie-designware.c  | 75 +++++++------------
>  drivers/pci/controller/dwc/pcie-designware.h  | 18 +----
>  5 files changed, 76 insertions(+), 129 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.c b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> index 0fbf86c0b97e..ff185b8977f3 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> @@ -213,10 +213,8 @@ static ssize_t lane_detect_write(struct file *file, const char __user *buf,
>  	if (val)
>  		return val;
>  
> -	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
> -	val &= ~(LANE_SELECT);
> -	val |= FIELD_PREP(LANE_SELECT, lane);
> -	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG, val);
> +	dw_pcie_clear_and_set_dword(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG,
> +				    LANE_SELECT, FIELD_PREP(LANE_SELECT, lane));
>  
>  	return count;
>  }
> @@ -309,12 +307,11 @@ static void set_event_number(struct dwc_pcie_rasdes_priv *pdata,
>  {
>  	u32 val;
>  
> -	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
> -	val &= ~EVENT_COUNTER_ENABLE;
> -	val &= ~(EVENT_COUNTER_GROUP_SELECT | EVENT_COUNTER_EVENT_SELECT);
> -	val |= FIELD_PREP(EVENT_COUNTER_GROUP_SELECT, event_list[pdata->idx].group_no);
> +	val = FIELD_PREP(EVENT_COUNTER_GROUP_SELECT, event_list[pdata->idx].group_no);
>  	val |= FIELD_PREP(EVENT_COUNTER_EVENT_SELECT, event_list[pdata->idx].event_no);
> -	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +	dw_pcie_clear_and_set_dword(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG,
> +				    EVENT_COUNTER_ENABLE | EVENT_COUNTER_GROUP_SELECT |
> +				    EVENT_COUNTER_EVENT_SELECT, val);
>  }
>  
>  static ssize_t counter_enable_read(struct file *file, char __user *buf,
> @@ -354,13 +351,10 @@ static ssize_t counter_enable_write(struct file *file, const char __user *buf,
>  
>  	mutex_lock(&rinfo->reg_event_lock);
>  	set_event_number(pdata, pci, rinfo);
> -	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
> -	if (enable)
> -		val |= FIELD_PREP(EVENT_COUNTER_ENABLE, PER_EVENT_ON);
> -	else
> -		val |= FIELD_PREP(EVENT_COUNTER_ENABLE, PER_EVENT_OFF);
>  
> -	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +	val |= FIELD_PREP(EVENT_COUNTER_ENABLE, enable ? PER_EVENT_ON : PER_EVENT_OFF);

So you just added the bitfields to the existing 'val' variable which was storing
the return value of kstrtou32_from_user().

What makes me nervous about this series is these kind of subtle bugs. It is
really hard to spot all with too many drivers/changes :/

I would suggest you to just convert whatever drivers you can test with and leave
the rest to platforms maintainers to convert later. I do not want to regress
platforms for cleanups.

> +	dw_pcie_clear_and_set_dword(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG,
> +				    0, val);

Similar to what Lukas suggested here: https://lore.kernel.org/linux-pci/aKDpIeQgt7I9Ts8F@wunner.de

Please use separate API for just setting the word instead of passing 0 to this
API.

- Mani

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