linux-kernel - Re: [PATCH] pci/quirks: Add quirk for Bayhub O2 SD controller

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20211215180426.GA702736@bhelgaas>
Date:   Wed, 15 Dec 2021 12:04:26 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Rajat Jain <rajatja@...gle.com>
Cc:     Adrian Hunter <adrian.hunter@...el.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>, linux-mmc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        rajatxjain@...il.com, jsbarnes@...gle.com, gwendal@...gle.com
Subject: Re: [PATCH] pci/quirks: Add quirk for Bayhub O2 SD controller

On Tue, Dec 07, 2021 at 04:09:48PM -0800, Rajat Jain wrote:
> This particular SD controller from O2 / Bayhub only allows dword
> accesses to its LTR max latency registers:
> https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf

What happens if we use a non-dword access?  Unsupported Request?
Invalid data returned?  Writes ignored?

I guess word accesses must not cause PCIe errors, since we still do
them in pci_save_ltr_state() and pci_restore_ltr_state() even with
this patch.

The app note says 0x234 (Max Latency registers), 0x248 (L1 PM
Substates Control 1), and 0x24c (L1 PM Substates Control 2) are all
broken, but the patch only mentions 0x234.

I guess for 0x248 and 0x24c (the L1 PM Substates Control registers),
we're just lucky because those are dword registers, and all current
users already do dword accesses.

What if we instead changed pci_save_ltr_state() and
pci_restore_ltr_state() to do a single dword access instead of two
word accesses?  That kind of sweeps it under the rug, but we're
already doing that for 0x248 and 0x24c.

If we did that, we shouldn't need a quirk at all, but the hardware bug
is still lurking, and we should add a comment about it somewhere.

I guess setpci (and maybe lspci) could still do smaller accesses and
see whatever the bad behavior is.  Hmmm.  Maybe we just have to live
with that.

The app note doesn't actually say how to identify the part -- no
"affected Device ID", for instance.  Are we confident that the other
O2_* devices are unaffected?

> Thus add a quirk that saves and restores these registers
> manually using dword acesses:
> LTR Max Snoop Latency Register
> LTR Max No-Snoop Latency Register
> 
> Signed-off-by: Rajat Jain <rajatja@...gle.com>
> ---
>  drivers/mmc/host/sdhci-pci.h |  1 -
>  drivers/pci/quirks.c         | 39 ++++++++++++++++++++++++++++++++++++
>  include/linux/pci_ids.h      |  1 +
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
> index 5e3193278ff9..d47cc0ba7ca4 100644
> --- a/drivers/mmc/host/sdhci-pci.h
> +++ b/drivers/mmc/host/sdhci-pci.h
> @@ -10,7 +10,6 @@
>  #define PCI_DEVICE_ID_O2_SDS1		0x8421
>  #define PCI_DEVICE_ID_O2_FUJIN2		0x8520
>  #define PCI_DEVICE_ID_O2_SEABIRD0	0x8620
> -#define PCI_DEVICE_ID_O2_SEABIRD1	0x8621
>  
>  #define PCI_DEVICE_ID_INTEL_PCH_SDIO0	0x8809
>  #define PCI_DEVICE_ID_INTEL_PCH_SDIO1	0x880a
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 003950c738d2..b7bd19802744 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5857,3 +5857,42 @@ static void nvidia_ion_ahci_fixup(struct pci_dev *pdev)
>  	pdev->dev_flags |= PCI_DEV_FLAGS_HAS_MSI_MASKING;
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_NVIDIA, 0x0ab8, nvidia_ion_ahci_fixup);
> +
> +/*
> + * Bayhub OZ711LV2 SD controller has an errata that only allows DWORD accesses
> + * to the LTR max latency registers. Thus need to save and restore these
> + * registers manually.
> + */
> +static void o2_seabird1_save_ltr(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u32 *reg32;
> +
> +	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
> +	if (save_state) {
> +		reg32 = &save_state->cap.data[0];
> +		/* Preserve PCI_LTR_MAX_SNOOP_LAT & PCI_LTR_MAX_NOSNOOP_LAT */
> +		pci_read_config_dword(dev, 0x234, reg32);
> +	} else {
> +		pci_err(dev, "quirk can't save LTR snoop latency\n");
> +	}
> +}
> +
> +static void o2_seabird1_restore_ltr(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u32 *reg32;
> +
> +	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
> +	if (save_state) {
> +		reg32 = &save_state->cap.data[0];
> +		/* Restore PCI_LTR_MAX_SNOOP_LAT & PCI_LTR_MAX_NOSNOOP_LAT */
> +		pci_write_config_dword(dev, 0x234, *reg32);
> +	} else {
> +		pci_err(dev, "quirk can't restore LTR snoop latency\n");
> +	}
> +}
> +DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_O2, PCI_DEVICE_ID_O2_SEABIRD1,
> +			       o2_seabird1_save_ltr);
> +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_O2, PCI_DEVICE_ID_O2_SEABIRD1,
> +			       o2_seabird1_restore_ltr);
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 011f2f1ea5bb..6ed16aa38196 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -1717,6 +1717,7 @@
>  #define PCI_DEVICE_ID_O2_8221		0x8221
>  #define PCI_DEVICE_ID_O2_8320		0x8320
>  #define PCI_DEVICE_ID_O2_8321		0x8321
> +#define PCI_DEVICE_ID_O2_SEABIRD1	0x8621
>  
>  #define PCI_VENDOR_ID_3DFX		0x121a
>  #define PCI_DEVICE_ID_3DFX_VOODOO	0x0001
> -- 
> 2.34.1.400.ga245620fadb-goog
>