| 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
| ||
|
Message-ID: <6aaafcce-0dc4-45b1-aef5-9004f84fd207@163.com>
Date: Thu, 28 Aug 2025 21:47:32 +0800
From: Hans Zhang <18255117159@....com>
To: Manivannan Sadhasivam <mani@...nel.org>
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 2025/8/27 21:51, Manivannan Sadhasivam wrote:
> 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().
>
Dear Mani,
Thank you very much for your reply and for taking the time to point out
the problem. I double-checked today, identified the problem and will fix
it in the next version.
> 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.
My original intention was to hope that the code would become
increasingly concise and that a lot of repetitive code would be removed.
I also believe it will provide convenience for the subsequent
development. I hope there won't be any problems with my next version.
>
>> + 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.
>
Will add dw_pcie_clear_dword and dw_pcie_set_dword.
Best regards,
Hans
> - Mani
>
Powered by blists - more mailing lists