| 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: <9b662513-2c71-8829-3ec8-c789a919809a@oss.qualcomm.com>
Date: Mon, 12 May 2025 21:42:59 +0530
From: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Shawn Anastasio <sanastasio@...torengineering.com>
Cc: linux-pci@...r.kernel.org, Lukas Wunner <lukas@...ner.de>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Rob Herring <robh@...nel.o>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
chaitanya chundru <quic_krichai@...cinc.com>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
cros-qcom-dts-watchers@...omium.org, Jingoo Han <jingoohan1@...il.com>,
Bartosz Golaszewski <brgl@...ev.pl>, quic_vbadigan@...cnic.com,
amitk@...nel.org, devicetree@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, linux-arm-msm@...r.kernel.org,
jorge.ramirez@....qualcomm.com, Dmitry Baryshkov <lumag@...nel.org>,
Timothy Pearson <tpearson@...torengineering.com>
Subject: Re: [RESEND PATCH v6] PCI: PCI: Add pcie_link_is_active() to
determine if the PCIe link is active
On 5/12/2025 5:20 PM, Ilpo Järvinen wrote:
> On Fri, 9 May 2025, Shawn Anastasio wrote:
>
> In shortlog:
>
> PCI: PCI: ... -> PCI: ...
>
>> From: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
>>
>> Date: Sat, 12 Apr 2025 07:19:56 +0530
>>
>> Introduce a common API to check if the PCIe link is active, replacing
>> duplicate code in multiple locations.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
>> Signed-off-by: Shawn Anastasio <sanastasio@...torengineering.com>
>> ---
>> (Resent since git-send-email failed to detect the Subject field from the patch
>> file previously -- apologies!)
>>
>> This is an updated patch pulled from Krishna's v5 series:
>> https://patchwork.kernel.org/project/linux-pci/list/?series=952665
>>
>> The following changes to Krishna's v5 were made by me:
>> - Revert pcie_link_is_active return type back to int per Lukas' review
>> comments
>> - Handle non-zero error returns at call site of the new function in
>> pci.c/pci_bridge_wait_for_secondary_bus
>>
>> drivers/pci/hotplug/pciehp.h | 1 -
>> drivers/pci/hotplug/pciehp_ctrl.c | 2 +-
>> drivers/pci/hotplug/pciehp_hpc.c | 33 +++----------------------------
>> drivers/pci/pci.c | 26 +++++++++++++++++++++---
>> include/linux/pci.h | 4 ++++
>> 5 files changed, 31 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
>> index 273dd8c66f4e..acef728530e3 100644
>> --- a/drivers/pci/hotplug/pciehp.h
>> +++ b/drivers/pci/hotplug/pciehp.h
>> @@ -186,7 +186,6 @@ int pciehp_query_power_fault(struct controller *ctrl);
>> int pciehp_card_present(struct controller *ctrl);
>> int pciehp_card_present_or_link_active(struct controller *ctrl);
>> int pciehp_check_link_status(struct controller *ctrl);
>> -int pciehp_check_link_active(struct controller *ctrl);
>> void pciehp_release_ctrl(struct controller *ctrl);
>>
>> int pciehp_sysfs_enable_slot(struct hotplug_slot *hotplug_slot);
>> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
>> index d603a7aa7483..4bb58ba1c766 100644
>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>> @@ -260,7 +260,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>> /* Turn the slot on if it's occupied or link is up */
>> mutex_lock(&ctrl->state_lock);
>> present = pciehp_card_present(ctrl);
>> - link_active = pciehp_check_link_active(ctrl);
>> + link_active = pcie_link_is_active(ctrl->pcie->port);
>> if (present <= 0 && link_active <= 0) {
>> if (ctrl->state == BLINKINGON_STATE) {
>> ctrl->state = OFF_STATE;
>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
>> index 8a09fb6083e2..278bc21d531d 100644
>> --- a/drivers/pci/hotplug/pciehp_hpc.c
>> +++ b/drivers/pci/hotplug/pciehp_hpc.c
>> @@ -221,33 +221,6 @@ static void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask)
>> pcie_do_write_cmd(ctrl, cmd, mask, false);
>> }
>>
>> -/**
>> - * pciehp_check_link_active() - Is the link active
>> - * @ctrl: PCIe hotplug controller
>> - *
>> - * Check whether the downstream link is currently active. Note it is
>> - * possible that the card is removed immediately after this so the
>> - * caller may need to take it into account.
>> - *
>> - * If the hotplug controller itself is not available anymore returns
>> - * %-ENODEV.
>> - */
>> -int pciehp_check_link_active(struct controller *ctrl)
>> -{
>> - struct pci_dev *pdev = ctrl_dev(ctrl);
>> - u16 lnk_status;
>> - int ret;
>> -
>> - ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
>> - if (ret == PCIBIOS_DEVICE_NOT_FOUND || PCI_POSSIBLE_ERROR(lnk_status))
>> - return -ENODEV;
>> -
>> - ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
>> - ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
>> -
>> - return ret;
>> -}
>> -
>> static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
>> {
>> u32 l;
>> @@ -467,7 +440,7 @@ int pciehp_card_present_or_link_active(struct controller *ctrl)
>> if (ret)
>> return ret;
>>
>> - return pciehp_check_link_active(ctrl);
>> + return pcie_link_is_active(ctrl_dev(ctrl));
>> }
>>
>> int pciehp_query_power_fault(struct controller *ctrl)
>> @@ -584,7 +557,7 @@ static void pciehp_ignore_dpc_link_change(struct controller *ctrl,
>> * Synthesize it to ensure that it is acted on.
>> */
>> down_read_nested(&ctrl->reset_lock, ctrl->depth);
>> - if (!pciehp_check_link_active(ctrl))
>> + if (!pcie_link_is_active(ctrl_dev(ctrl)))
>> pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
>> up_read(&ctrl->reset_lock);
>> }
>> @@ -884,7 +857,7 @@ int pciehp_slot_reset(struct pcie_device *dev)
>> pcie_capability_write_word(dev->port, PCI_EXP_SLTSTA,
>> PCI_EXP_SLTSTA_DLLSC);
>>
>> - if (!pciehp_check_link_active(ctrl))
>> + if (!pcie_link_is_active(ctrl_dev(ctrl)))
>> pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
>>
>> return 0;
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index e77d5b53c0ce..3bb8354b14bf 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4926,7 +4926,6 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
>> return 0;
>>
>> if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
>> - u16 status;
>>
>> pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
>> msleep(delay);
>> @@ -4942,8 +4941,7 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
>> if (!dev->link_active_reporting)
>> return -ENOTTY;
>>
>> - pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
>> - if (!(status & PCI_EXP_LNKSTA_DLLLA))
>> + if (pcie_link_is_active(dev) <= 0)
>> return -ENOTTY;
>>
>> return pci_dev_wait(child, reset_type,
>> @@ -6247,6 +6245,28 @@ void pcie_print_link_status(struct pci_dev *dev)
>> }
>> EXPORT_SYMBOL(pcie_print_link_status);
>>
>> +/**
>> + * pcie_link_is_active() - Checks if the link is active or not
>> + * @pdev: PCI device to query
>> + *
>> + * Check whether the link is active or not.
>> + *
>> + * Return: link state, or -ENODEV if the config read failes.
>
> "link state" is bit vague, it would be better to document the values
> returned more precisely.
>
>> + */
>> +int pcie_link_is_active(struct pci_dev *pdev)
>> +{
>> + u16 lnk_status;
>> + int ret;
>> +
>> + ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
>> + if (ret == PCIBIOS_DEVICE_NOT_FOUND || PCI_POSSIBLE_ERROR(lnk_status))
>> + return -ENODEV;
>> +
>> + pci_dbg(pdev, "lnk_status = %x\n", lnk_status);
>> + return !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
>> +}
>> +EXPORT_SYMBOL(pcie_link_is_active);
>> +
>> /**
>> * pci_select_bars - Make BAR mask from the type of resource
>> * @dev: the PCI device for which BAR mask is made
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 51e2bd6405cd..a79a9919320c 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1945,6 +1945,7 @@ pci_release_mem_regions(struct pci_dev *pdev)
>> pci_select_bars(pdev, IORESOURCE_MEM));
>> }
>>
>> +int pcie_link_is_active(struct pci_dev *dev);
>
> Is this really needed in include/linux/pci.h, isn't drivers/pci/pci.h
> enough (for pwrctrl in the Krishna's series)?
>
As this is generic functions, the endpoint drivers can also
use this API to check if link is active or not in future.
- Krishna Chaitanya.
>> #else /* CONFIG_PCI is not enabled */
>>
>> static inline void pci_set_flags(int flags) { }
>> @@ -2093,6 +2094,9 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>> {
>> return -ENOSPC;
>> }
>> +
>> +static inline bool pcie_link_is_active(struct pci_dev *dev)
>> +{ return false; }
>
> This fits on one line just fine.
>
Powered by blists - more mailing lists