netdev - Re: [PATCH V3 1/6] PCI: Use cached Device Capabilities Register

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: <2030c1c1-6060-2b5f-74d9-f53f89e442e6@xs4all.nl>
Date:   Wed, 26 May 2021 12:46:56 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Dongdong Liu <liudongdong3@...wei.com>, helgaas@...nel.org,
        hch@...radead.org, linux-pci@...r.kernel.org, rajur@...lsio.com
Cc:     linux-media@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH V3 1/6] PCI: Use cached Device Capabilities Register

On 12/05/2021 16:37, Dongdong Liu wrote:
> It will make sense to store the pcie_devcap value in the pci_dev
> structure instead of reading Device Capabilities Register multiple
> times. The fisrt place to use pcie_devcap is in set_pcie_port_type(),
> get the pcie_devcap value here, then use cached pcie_devcap in the
> needed place.
> 
> Signed-off-by: Dongdong Liu <liudongdong3@...wei.com>
> ---
>  drivers/media/pci/cobalt/cobalt-driver.c |  4 ++--

For the cobalt driver:

Acked-by: Hans Verkuil <hverkuil-cisco@...all.nl>

Regards,

	Hans

>  drivers/pci/pci.c                        |  5 +----
>  drivers/pci/pcie/aspm.c                  | 11 ++++-------
>  drivers/pci/probe.c                      | 11 +++--------
>  drivers/pci/quirks.c                     |  3 +--
>  include/linux/pci.h                      |  1 +
>  6 files changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/pci/cobalt/cobalt-driver.c b/drivers/media/pci/cobalt/cobalt-driver.c
> index 839503e..04e735f 100644
> --- a/drivers/media/pci/cobalt/cobalt-driver.c
> +++ b/drivers/media/pci/cobalt/cobalt-driver.c
> @@ -193,11 +193,11 @@ void cobalt_pcie_status_show(struct cobalt *cobalt)
>  		return;
> 
>  	/* Device */
> -	pcie_capability_read_dword(pci_dev, PCI_EXP_DEVCAP, &capa);
>  	pcie_capability_read_word(pci_dev, PCI_EXP_DEVCTL, &ctrl);
>  	pcie_capability_read_word(pci_dev, PCI_EXP_DEVSTA, &stat);
>  	cobalt_info("PCIe device capability 0x%08x: Max payload %d\n",
> -		    capa, get_payload_size(capa & PCI_EXP_DEVCAP_PAYLOAD));
> +		    capa,
> +		    get_payload_size(pci_dev->pcie_devcap & PCI_EXP_DEVCAP_PAYLOAD));
>  	cobalt_info("PCIe device control 0x%04x: Max payload %d. Max read request %d\n",
>  		    ctrl,
>  		    get_payload_size((ctrl & PCI_EXP_DEVCTL_PAYLOAD) >> 5),
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b717680..68ccd77 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4620,13 +4620,10 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);
>   */
>  bool pcie_has_flr(struct pci_dev *dev)
>  {
> -	u32 cap;
> -
>  	if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
>  		return false;
> 
> -	pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
> -	return cap & PCI_EXP_DEVCAP_FLR;
> +	return dev->pcie_devcap & PCI_EXP_DEVCAP_FLR;
>  }
>  EXPORT_SYMBOL_GPL(pcie_has_flr);
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index ac0557a..d637564 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -660,7 +660,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
> 
>  	/* Get and check endpoint acceptable latencies */
>  	list_for_each_entry(child, &linkbus->devices, bus_list) {
> -		u32 reg32, encoding;
> +		u32 encoding;
>  		struct aspm_latency *acceptable =
>  			&link->acceptable[PCI_FUNC(child->devfn)];
> 
> @@ -668,12 +668,11 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>  		    pci_pcie_type(child) != PCI_EXP_TYPE_LEG_END)
>  			continue;
> 
> -		pcie_capability_read_dword(child, PCI_EXP_DEVCAP, &reg32);
>  		/* Calculate endpoint L0s acceptable latency */
> -		encoding = (reg32 & PCI_EXP_DEVCAP_L0S) >> 6;
> +		encoding = (child->pcie_devcap & PCI_EXP_DEVCAP_L0S) >> 6;
>  		acceptable->l0s = calc_l0s_acceptable(encoding);
>  		/* Calculate endpoint L1 acceptable latency */
> -		encoding = (reg32 & PCI_EXP_DEVCAP_L1) >> 9;
> +		encoding = (child->pcie_devcap & PCI_EXP_DEVCAP_L1) >> 9;
>  		acceptable->l1 = calc_l1_acceptable(encoding);
> 
>  		pcie_aspm_check_latency(child);
> @@ -808,7 +807,6 @@ static void free_link_state(struct pcie_link_state *link)
>  static int pcie_aspm_sanity_check(struct pci_dev *pdev)
>  {
>  	struct pci_dev *child;
> -	u32 reg32;
> 
>  	/*
>  	 * Some functions in a slot might not all be PCIe functions,
> @@ -831,8 +829,7 @@ static int pcie_aspm_sanity_check(struct pci_dev *pdev)
>  		 * Disable ASPM for pre-1.1 PCIe device, we follow MS to use
>  		 * RBER bit to determine if a function is 1.1 version device
>  		 */
> -		pcie_capability_read_dword(child, PCI_EXP_DEVCAP, &reg32);
> -		if (!(reg32 & PCI_EXP_DEVCAP_RBER) && !aspm_force) {
> +		if (!(child->pcie_devcap & PCI_EXP_DEVCAP_RBER) && !aspm_force) {
>  			pci_info(child, "disabling ASPM on pre-1.1 PCIe device.  You can enable it with 'pcie_aspm=force'\n");
>  			return -EINVAL;
>  		}
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 3a62d09..7963ab2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1497,8 +1497,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
>  	pdev->pcie_cap = pos;
>  	pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
>  	pdev->pcie_flags_reg = reg16;
> -	pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
> -	pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
> +	pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->pcie_devcap);
> +	pdev->pcie_mpss = pdev->pcie_devcap & PCI_EXP_DEVCAP_PAYLOAD;
> 
>  	parent = pci_upstream_bridge(pdev);
>  	if (!parent)
> @@ -2008,18 +2008,13 @@ static void pci_configure_mps(struct pci_dev *dev)
>  int pci_configure_extended_tags(struct pci_dev *dev, void *ign)
>  {
>  	struct pci_host_bridge *host;
> -	u32 cap;
>  	u16 ctl;
>  	int ret;
> 
>  	if (!pci_is_pcie(dev))
>  		return 0;
> 
> -	ret = pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
> -	if (ret)
> -		return 0;
> -
> -	if (!(cap & PCI_EXP_DEVCAP_EXT_TAG))
> +	if (!(dev->pcie_devcap & PCI_EXP_DEVCAP_EXT_TAG))
>  		return 0;
> 
>  	ret = pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &ctl);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index dcb229d..b89b438 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5073,8 +5073,7 @@ static void quirk_intel_qat_vf_cap(struct pci_dev *pdev)
>  		pdev->pcie_cap = pos;
>  		pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
>  		pdev->pcie_flags_reg = reg16;
> -		pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
> -		pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
> +		pdev->pcie_mpss = pdev->pcie_devcap & PCI_EXP_DEVCAP_PAYLOAD;
> 
>  		pdev->cfg_size = PCI_CFG_SPACE_EXP_SIZE;
>  		if (pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, &status) !=
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c20211e..555a3ac 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -340,6 +340,7 @@ struct pci_dev {
>  	u8		rom_base_reg;	/* Config register controlling ROM */
>  	u8		pin;		/* Interrupt pin this device uses */
>  	u16		pcie_flags_reg;	/* Cached PCIe Capabilities Register */
> +	u32		pcie_devcap;	/* Cached Device Capabilities Register */
>  	unsigned long	*dma_alias_mask;/* Mask of enabled devfn aliases */
> 
>  	struct pci_driver *driver;	/* Driver bound to this device */
> --
> 2.7.4
>