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] [day] [month] [year] [list]
Message-ID: <accbe6ad-52d7-9e4f-c5cb-de7f67b5c990@oss.qualcomm.com>
Date: Tue, 13 May 2025 06:21:45 +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 10:54 PM, Ilpo Järvinen wrote:
> On Mon, 12 May 2025, Ilpo Järvinen wrote:
> 
>> On Mon, 12 May 2025, Krishna Chaitanya Chundru wrote:
>>
>>>
>>>
>>> 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.
>>
>> Hi again,
>>
>> Shouldn't the endpoint drivers use the generic RPM interfaces, not mess
>> with the Link state themselves? If Link is found to be up using this
>> function, how does that prove what's the state of the link the next
>> moment? To me this does not look like a very safe interface to be used by
>> the endpoint drivers.
>
I looked to net drivers, I see there are places which use this [1].

> Now I even noticed that little detail got removed from the kerneldoc while
> moving the function. Why?
Shawn, can you add the removed details back.

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wwan/iosm/iosm_ipc_pcie.c?h=v6.15-rc6#n178

- Krishna Chaitanya.
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ