| 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: <6852eb10-ff39-44d2-ba0f-cb77f1a9aaf8@amd.com>
Date: Thu, 19 Oct 2023 16:47:12 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Raju Rangoju <Raju.Rangoju@....com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, Shyam-sundar.S-k@....com,
Sudheesh Mavila <sudheesh.mavila@....com>
Subject: Re: [PATCH net-next 2/2] amd-xgbe: Add support for AMD Crater
ethernet device
On 10/18/23 09:44, Raju Rangoju wrote:
> The AMD Crater device has new window settings for the XPCS access, add
> support to adopt to the new window settings. There is a hardware bug
> where in the BAR1 registers cannot be accessed directly. As a fallback
> mechanism, access these PCS registers through indirect access via SMN. >
> Co-developed-by: Sudheesh Mavila <sudheesh.mavila@....com>
> Signed-off-by: Sudheesh Mavila <sudheesh.mavila@....com>
> Acked-by: Shyam Sundar S K <Shyam-sundar.S-k@....com>
> Signed-off-by: Raju Rangoju <Raju.Rangoju@....com>
> ---
> drivers/net/ethernet/amd/xgbe/xgbe-common.h | 5 ++++
> drivers/net/ethernet/amd/xgbe/xgbe-dev.c | 33 +++++++++++++++++----
> drivers/net/ethernet/amd/xgbe/xgbe-pci.c | 32 +++++++++++++++-----
> drivers/net/ethernet/amd/xgbe/xgbe.h | 6 ++++
> 4 files changed, 63 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-common.h b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
> index 3b70f6737633..e1f70f0528ef 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-common.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
> @@ -900,6 +900,11 @@
> #define PCS_V2_RV_WINDOW_SELECT 0x1064
> #define PCS_V2_YC_WINDOW_DEF 0x18060
> #define PCS_V2_YC_WINDOW_SELECT 0x18064
> +#define PCS_V2_RN_WINDOW_DEF 0xF8078
> +#define PCS_V2_RN_WINDOW_SELECT 0xF807c
> +
> +#define PCS_RN_SMN_BASE_ADDR 0x11E00000
> +#define PCS_RN_PORT_ADDR_SIZE 0x100000
>
> /* PCS register entry bit positions and sizes */
> #define PCS_V2_WINDOW_DEF_OFFSET_INDEX 6
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> index f393228d41c7..da8ec218282f 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> @@ -1176,8 +1176,17 @@ static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
> offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
>
> spin_lock_irqsave(&pdata->xpcs_lock, flags);
> - XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> - mmd_data = XPCS16_IOREAD(pdata, offset);
> + if (pdata->vdata->is_crater) {
This feels like it should be a new xgbe_read_mmd_regs_v3() and devices
with the this bug should have a new XGBE_XPCS_ACCESS_V3 type for
pdata->vdata->xpcs_access.
Feel free to write a pre-patch, too, that creates helper functions to
calculate the mmd_address for v1/2/3 and the index/offset for v2/3 types.
> + amd_smn_write(0,
> + (pdata->xphy_base + pdata->xpcs_window_sel_reg),
No need for the parentheses here and all the parameters can be on one line
now that there is a 100 character line limit.
> + index);
> + amd_smn_read(0, pdata->xphy_base + offset, &mmd_data);
> + mmd_data = (offset % ALIGNMENT_VAL) ?
> + ((mmd_data >> 16) & 0xffff) : (mmd_data & 0xffff);
> + } else {
> + XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> + mmd_data = XPCS16_IOREAD(pdata, offset);
> + }
> spin_unlock_irqrestore(&pdata->xpcs_lock, flags);
>
> return mmd_data;
> @@ -1186,8 +1195,8 @@ static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
> static void xgbe_write_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
> int mmd_reg, int mmd_data)
> {
> + unsigned int mmd_address, index, offset, crtr_mmd_data;
> unsigned long flags;
> - unsigned int mmd_address, index, offset;
>
> if (mmd_reg & XGBE_ADDR_C45)
> mmd_address = mmd_reg & ~XGBE_ADDR_C45;
> @@ -1208,8 +1217,22 @@ static void xgbe_write_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
> offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
>
> spin_lock_irqsave(&pdata->xpcs_lock, flags);
> - XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> - XPCS16_IOWRITE(pdata, offset, mmd_data);
> + if (pdata->vdata->is_crater) {
> + amd_smn_write(0, (pdata->xphy_base + pdata->xpcs_window_sel_reg), index);
No parentheses needed on "(pdata->xphy_base + pdata->xpcs_window_sel_reg)"
> + amd_smn_read(0, pdata->xphy_base + offset, &crtr_mmd_data);
> + if (offset % ALIGNMENT_VAL) {
> + crtr_mmd_data &= ~GENMASK(31, 16);
> + crtr_mmd_data |= (mmd_data << 16);
> + } else {
> + crtr_mmd_data &= ~GENMASK(15, 0);
> + crtr_mmd_data |= (mmd_data);
> + }
> + amd_smn_write(0, (pdata->xphy_base + pdata->xpcs_window_sel_reg), index);
Ditto on the parentheses here...
> + amd_smn_write(0, (pdata->xphy_base + offset), crtr_mmd_data);
and here.
> + } else {
> + XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> + XPCS16_IOWRITE(pdata, offset, mmd_data);
> + }
Ditto here, having a v3 function will make this much cleaner.
> spin_unlock_irqrestore(&pdata->xpcs_lock, flags);
> }
>
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> index a17359d43b45..90ad520d3c29 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> @@ -279,15 +279,21 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
> pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
> } else if (rdev && (rdev->vendor == PCI_VENDOR_ID_AMD) &&
> - (rdev->device == 0x14b5)) {
> - pdata->xpcs_window_def_reg = PCS_V2_YC_WINDOW_DEF;
> - pdata->xpcs_window_sel_reg = PCS_V2_YC_WINDOW_SELECT;
> -
> - /* Yellow Carp devices do not need cdr workaround */
> + ((rdev->device == 0x14b5) || (rdev->device == 0x1630))) {
At this point it might make sense to move to a switch statement for each
device.
> + /* Yellow Carp and Crater devices
> + * do not need cdr workaround and RRC
Please use the full width available for comments.
> + */
> pdata->vdata->an_cdr_workaround = 0;
> -
> - /* Yellow Carp devices do not need rrc */
> pdata->vdata->enable_rrc = 0;
> +
> + if (rdev->device == 0x1630) {
> + pdata->xpcs_window_def_reg = PCS_V2_RN_WINDOW_DEF;
> + pdata->xpcs_window_sel_reg = PCS_V2_RN_WINDOW_SELECT;
> + pdata->vdata->is_crater = true;
> + } else {
> + pdata->xpcs_window_def_reg = PCS_V2_YC_WINDOW_DEF;
> + pdata->xpcs_window_sel_reg = PCS_V2_YC_WINDOW_SELECT;
> + }
> } else {
> pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
> pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
> @@ -295,7 +301,17 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> pci_dev_put(rdev);
>
> /* Configure the PCS indirect addressing support */
> - reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
> + if (pdata->vdata->is_crater) {
> + reg = XP_IOREAD(pdata, XP_PROP_0);
> + pdata->xphy_base = PCS_RN_SMN_BASE_ADDR +
> + (PCS_RN_PORT_ADDR_SIZE *
> + XP_GET_BITS(reg, XP_PROP_0, PORT_ID));
> + if (netif_msg_probe(pdata))
> + dev_dbg(dev, "xphy_base = %#08x\n", pdata->xphy_base);
> + amd_smn_read(0, pdata->xphy_base + (pdata->xpcs_window_def_reg), ®);
No need for the parentheses around "(pdata->xpcs_window_def_reg)"
> + } else {
> + reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
> + }
> pdata->xpcs_window = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, OFFSET);
> pdata->xpcs_window <<= 6;
> pdata->xpcs_window_size = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, SIZE);
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
> index ad136ed493ed..a161fac35643 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
> @@ -133,6 +133,7 @@
> #include <linux/dcache.h>
> #include <linux/ethtool.h>
> #include <linux/list.h>
> +#include <asm/amd_nb.h>
Why is this needed in this header?
Also, this is buildable for arm64 only, so you'll need to protect the use
of this include with a "#ifdef CONFIG_X86" when you put it in the proper
file(s).
>
> #define XGBE_DRV_NAME "amd-xgbe"
> #define XGBE_DRV_DESC "AMD 10 Gigabit Ethernet Driver"
> @@ -305,6 +306,9 @@
> /* MDIO port types */
> #define XGMAC_MAX_C22_PORT 3
>
> + /* offset alignment */
> +#define ALIGNMENT_VAL 4
This is a bit generic, how about something that describes what this is a
bit more.
> +
> /* Link mode bit operations */
> #define XGBE_ZERO_SUP(_ls) \
> ethtool_link_ksettings_zero_link_mode((_ls), supported)
> @@ -1046,6 +1050,7 @@ struct xgbe_version_data {
> unsigned int rx_desc_prefetch;
> unsigned int an_cdr_workaround;
> unsigned int enable_rrc;
> + bool is_crater;
Probably better to name this "use_smn" or such vs is_crater.
> };
>
> struct xgbe_prv_data {
> @@ -1056,6 +1061,7 @@ struct xgbe_prv_data {
> struct device *dev;
> struct platform_device *phy_platdev;
> struct device *phy_dev;
> + unsigned int xphy_base;
And then probably better to name this smn_base.
Thanks,
Tom
>
> /* Version related data */
> struct xgbe_version_data *vdata;
Powered by blists - more mailing lists