Re: [PATCH] pci: Make DWORD accesses while saving / restoring LTR state

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Dec 21, 2021 at 05:21:05PM -0800, Rajat Jain wrote:
> Some devices have an errata such that they only support DWORD accesses
> to some registers.
> 
> For e.g. this Bayhub O2 device ([VID:DID] = [0x1217:0x8621]) only
> supports DWORD accesses to LTR latency registers and L1 PM substates
> control registers:
> https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf
> 
> Since L1 PM substate control registers are DWORD sized, and hence their
> access in the kernel is already DWORD sized, so we don't need to do
> anything for them.
> 
> However, the LTR registers being WORD sized, are in need of a solution.
> This patch converts the WORD sized accesses to these registers, into
> DWORD sized accesses, while saving and restoring them.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>

Applied to pci/enumeration for v5.17, thanks, Rajat!

The app note suggests that this erratum only affects the registers at
0x234, 0x248, and 0x24c, i.e., the LTR snoop registers and the L1 SS
control registers.

Can you confirm that is true?  Byte and word accesses to other parts
of config space work correctly?

I *assume* the other parts work correctly, because if byte and word
accesses were broken, all sorts of things would not work, like
PCI_COMMAND, PCI_STATUS, searching the capability list, etc.

Bjorn

> ---
>  drivers/pci/pci.c       | 24 ++++++++++++++++--------
>  drivers/pci/pcie/aspm.c |  1 +
>  2 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3d2fb394986a..efa8cd16827f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1556,7 +1556,7 @@ static void pci_save_ltr_state(struct pci_dev *dev)
>  {
>  	int ltr;
>  	struct pci_cap_saved_state *save_state;
> -	u16 *cap;
> +	u32 *cap;
>  
>  	if (!pci_is_pcie(dev))
>  		return;
> @@ -1571,25 +1571,33 @@ static void pci_save_ltr_state(struct pci_dev *dev)
>  		return;
>  	}
>  
> -	cap = (u16 *)&save_state->cap.data[0];
> -	pci_read_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap++);
> -	pci_read_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, cap++);
> +	/*
> +	 * We deliberately do a dword access to save both PCI_LTR_MAX_SNOOP_LAT
> +	 * and PCI_LTR_MAX_NOSNOOP_LAT together since some devices only support
> +	 * dword accesses to these registers.
> +	 */
> +	cap = &save_state->cap.data[0];
> +	pci_read_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap);
>  }
>  
>  static void pci_restore_ltr_state(struct pci_dev *dev)
>  {
>  	struct pci_cap_saved_state *save_state;
>  	int ltr;
> -	u16 *cap;
> +	u32 *cap;
>  
>  	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
>  	ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
>  	if (!save_state || !ltr)
>  		return;
>  
> -	cap = (u16 *)&save_state->cap.data[0];
> -	pci_write_config_word(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap++);
> -	pci_write_config_word(dev, ltr + PCI_LTR_MAX_NOSNOOP_LAT, *cap++);
> +	/*
> +	 * We deliberately do a dword access to restore both
> +	 * PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT together since
> +	 * some devices only support dword accesses to these registers.
> +	 */
> +	cap = &save_state->cap.data[0];
> +	pci_write_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, *cap);
>  }
>  
>  /**
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 52c74682601a..083f47a7b69b 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -496,6 +496,7 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link,
>  	encode_l12_threshold(l1_2_threshold, &scale, &value);
>  	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
>  
> +	/* Always make DWORD sized accesses to these registers */
>  	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
>  	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, &pctl2);
>  	pci_read_config_dword(child, child->l1ss + PCI_L1SS_CTL1, &cctl1);
> -- 
> 2.34.1.307.g9b7440fafd-goog
>