[RESEND PATCH 0/2] Add retimer interfaces support for Intel MAX 10 BMC

[RESEND PATCH 0/2] Add retimer interfaces support for Intel MAX 10 BMC

[Xu Yilun]

I resend this patchset to loop in networking developers for comments. This
is the previous thread. I'll fix other comments when I have a v2.

https://lore.kernel.org/lkml/X%2FV9hvXYlUOT9U2n@kroah.com/


The patchset is for the retimers connected to Intel MAX 10 BMC on Intel
PAC (Programmable Acceleration Card) N3000 Card. The network part of the
N3000 card is like the following:

                       +----------------------------------------+
                       |                  FPGA                  |
  +----+   +-------+   +-----------+  +----------+  +-----------+   +----------+
  |QSFP|---|retimer|---|Line Side  |--|User logic|--|Host Side  |---|XL710     |
  +----+   +-------+   |Ether Group|  |          |  |Ether Group|   |Ethernet  |
                       |(PHY + MAC)|  |wiring &  |  |(MAC + PHY)|   |Controller|
                       +-----------+  |offloading|  +-----------+   +----------+
                       |              +----------+              |
                       |                                        |
                       +----------------------------------------+

I had sent some RFC patches to expose the Line Side Ether Group + retimer +
QSFP as a netdev, and got some comments from netdev Maintainers.

https://lore.kernel.org/netdev/1603442745-13085-2-git-send-email-yilun.xu@intel.com/

The blocking issues I have is that physically the QSFP & retimer is
managed by the BMC and host could only get the retimer link states. This
is not enough to support some necessary netdev ops.  E.g. host cannot
realize the type/speed of the SFP by "ethtool -m", then users could not
configure the various layers accordingly.

This means the existing net tool can not work with it, so this patch just
expose the link states as custom sysfs attrs.


This patchset supports the ethernet retimers (C827) for the Intel PAC
(Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.

The 2 retimer chips connect to the Intel MAX 10 BMC on the card. They are
managed by the BMC firmware. Host could query their link states and
firmware version information via retimer interfaces (Shared registers) on
the BMC. The driver creates sysfs interfaces for users to query these
information.

The Intel OPAE (Open Programmable Acceleration Engine) lib provides tools
to read these attributes.

This is the source of the OPAE lib.

https://github.com/OPAE/opae-sdk/

Generally it facilitate the development on all the DFL (Device Feature
List) based FPGA Cards, including the management of static region &
dynamic region reprogramming, accelerators accessing and the board
specific peripherals.


Xu Yilun (2):
  mfd: intel-m10-bmc: specify the retimer sub devices
  misc: add support for retimers interfaces on Intel MAX 10 BMC

 .../ABI/testing/sysfs-driver-intel-m10-bmc-retimer |  32 +++++
 drivers/mfd/intel-m10-bmc.c                        |  19 ++-
 drivers/misc/Kconfig                               |  10 ++
 drivers/misc/Makefile                              |   1 +
 drivers/misc/intel-m10-bmc-retimer.c               | 158 +++++++++++++++++++++
 include/linux/mfd/intel-m10-bmc.h                  |   7 +
 6 files changed, 226 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
 create mode 100644 drivers/misc/intel-m10-bmc-retimer.c

-- 
2.7.4

[RESEND PATCH 1/2] mfd: intel-m10-bmc: specify the retimer sub devices

[Xu Yilun]

The patch specifies the 2 retimer sub devices and their resources in the
parent driver's mfd_cell. It also adds the register definition of the
retimer sub devices.

There are 2 ethernet retimer chips (C827) connected to the Intel MAX 10
BMC. They are managed by the BMC firmware, and host could query them via
retimer interfaces (shared registers) on the BMC. The 2 retimers have
identical register interfaces in different register addresses or fields,
so it is better we define 2 retimer devices and handle them with the same
driver.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 drivers/mfd/intel-m10-bmc.c       | 19 ++++++++++++++++++-
 include/linux/mfd/intel-m10-bmc.h |  7 +++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
index b84579b..e0a99a0 100644
--- a/drivers/mfd/intel-m10-bmc.c
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -17,9 +17,26 @@ enum m10bmc_type {
 	M10_N3000,
 };
 
+static struct resource retimer0_resources[] = {
+	{M10BMC_PKVL_A_VER, M10BMC_PKVL_A_VER, "version", IORESOURCE_REG, },
+};
+
+static struct resource retimer1_resources[] = {
+	{M10BMC_PKVL_B_VER, M10BMC_PKVL_B_VER, "version", IORESOURCE_REG, },
+};
+
 static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
 	{ .name = "n3000bmc-hwmon" },
-	{ .name = "n3000bmc-retimer" },
+	{
+		.name = "n3000bmc-retimer",
+		.num_resources = ARRAY_SIZE(retimer0_resources),
+		.resources = retimer0_resources,
+	},
+	{
+		.name = "n3000bmc-retimer",
+		.num_resources = ARRAY_SIZE(retimer1_resources),
+		.resources = retimer1_resources,
+	},
 	{ .name = "n3000bmc-secure" },
 };
 
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index c8ef2f1..d6216f9 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -21,6 +21,13 @@
 #define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
 #define M10BMC_VER_LEGACY_INVALID	0xffffffff
 
+/* Retimer related registers, in system register region */
+#define M10BMC_PKVL_LSTATUS		0x164
+#define M10BMC_PKVL_A_VER		0x254
+#define M10BMC_PKVL_B_VER		0x258
+#define M10BMC_PKVL_SERDES_VER		GENMASK(15, 0)
+#define M10BMC_PKVL_SBUS_VER		GENMASK(31, 16)
+
 /**
  * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
  * @dev: this device
-- 
2.7.4

[RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Xu Yilun]

This driver supports the ethernet retimers (C827) for the Intel PAC
(Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.

C827 is an Intel(R) Ethernet serdes transceiver chip that supports
up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
10G/25G retimer mode. Host could query their link states and firmware
version information via retimer interfaces (Shared registers) on Intel
MAX 10 BMC. The driver creates sysfs interfaces for users to query these
information.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 .../ABI/testing/sysfs-driver-intel-m10-bmc-retimer |  32 +++++
 drivers/misc/Kconfig                               |  10 ++
 drivers/misc/Makefile                              |   1 +
 drivers/misc/intel-m10-bmc-retimer.c               | 158 +++++++++++++++++++++
 4 files changed, 201 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
 create mode 100644 drivers/misc/intel-m10-bmc-retimer.c

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
new file mode 100644
index 0000000..528712a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-retimer
@@ -0,0 +1,32 @@
+What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/tag
+Date:		Jan 2021
+KernelVersion:	5.12
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the tag of the retimer chip. Now there are 2
+		retimer chips on Intel PAC N3000, they are tagged as
+		'retimer_A' and 'retimer_B'.
+		Format: "retimer_%c".
+
+What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/sbus_version
+Date:		Jan 2021
+KernelVersion:	5.12
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the Transceiver bus firmware version of
+		the retimer chip.
+		Format: "0x%04x".
+
+What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/serdes_version
+Date:		Jan 2021
+KernelVersion:	5.12
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the SERDES firmware version of the retimer
+		chip.
+		Format: "0x%04x".
+
+What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
+Date:		Jan 2021
+KernelVersion:	5.12
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the status of each line side link. "1" for
+		link up, "0" for link down.
+		Format: "%u".
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index fafa8b0..7cb9433 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -466,6 +466,16 @@ config HISI_HIKEY_USB
 	  switching between the dual-role USB-C port and the USB-A host ports
 	  using only one USB controller.
 
+config INTEL_M10_BMC_RETIMER
+	tristate "Intel(R) MAX 10 BMC ethernet retimer interface support"
+	depends on MFD_INTEL_M10_BMC
+	help
+	  This driver supports the ethernet retimer (C827) on Intel(R) MAX 10
+	  BMC, which is used by Intel PAC N3000 FPGA based Smart NIC.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called intel-m10-bmc-retimer.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d23231e..67883cf 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_HABANA_AI)		+= habanalabs/
 obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
 obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
+obj-$(CONFIG_INTEL_M10_BMC_RETIMER)	+= intel-m10-bmc-retimer.o
diff --git a/drivers/misc/intel-m10-bmc-retimer.c b/drivers/misc/intel-m10-bmc-retimer.c
new file mode 100644
index 0000000..d845342b
--- /dev/null
+++ b/drivers/misc/intel-m10-bmc-retimer.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Max10 BMC Retimer Interface Driver
+ *
+ * Copyright (C) 2021 Intel Corporation, Inc.
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define N3000BMC_RETIMER_DEV_NAME "n3000bmc-retimer"
+
+struct m10bmc_retimer {
+	struct device *dev;
+	struct intel_m10bmc *m10bmc;
+	u32 ver_reg;
+	u32 id;
+};
+
+static ssize_t tag_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "retimer_%c\n", 'A' + retimer->id);
+}
+static DEVICE_ATTR_RO(tag);
+
+static ssize_t sbus_version_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(retimer->m10bmc, retimer->ver_reg, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "0x%04x\n",
+			  (u16)FIELD_GET(M10BMC_PKVL_SBUS_VER, val));
+}
+static DEVICE_ATTR_RO(sbus_version);
+
+static ssize_t serdes_version_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(retimer->m10bmc, retimer->ver_reg, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "0x%04x\n",
+			  (u16)FIELD_GET(M10BMC_PKVL_SERDES_VER, val));
+}
+static DEVICE_ATTR_RO(serdes_version);
+
+struct link_attr {
+	struct device_attribute attr;
+	u32 index;
+};
+
+#define to_link_attr(dev_attr) \
+	container_of(dev_attr, struct link_attr, attr)
+
+static ssize_t
+link_status_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
+	struct link_attr *lattr = to_link_attr(attr);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(retimer->m10bmc, M10BMC_PKVL_LSTATUS, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n",
+			  !!(val & BIT((retimer->id << 2) + lattr->index)));
+}
+
+#define link_status_attr(_index)				\
+	static struct link_attr link_attr_status##_index =	\
+		{ .attr = __ATTR(link_status##_index, 0444,	\
+				 link_status_show, NULL),	\
+		  .index = (_index) }
+
+link_status_attr(0);
+link_status_attr(1);
+link_status_attr(2);
+link_status_attr(3);
+
+static struct attribute *m10bmc_retimer_attrs[] = {
+	&dev_attr_tag.attr,
+	&dev_attr_sbus_version.attr,
+	&dev_attr_serdes_version.attr,
+	&link_attr_status0.attr.attr,
+	&link_attr_status1.attr.attr,
+	&link_attr_status2.attr.attr,
+	&link_attr_status3.attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(m10bmc_retimer);
+
+static int intel_m10bmc_retimer_probe(struct platform_device *pdev)
+{
+	struct intel_m10bmc *m10bmc = dev_get_drvdata(pdev->dev.parent);
+	struct m10bmc_retimer *retimer;
+	struct resource *res;
+
+	retimer = devm_kzalloc(&pdev->dev, sizeof(*retimer), GFP_KERNEL);
+	if (!retimer)
+		return -ENOMEM;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_REG, "version");
+	if (!res) {
+		dev_err(&pdev->dev, "No REG resource for version\n");
+		return -EINVAL;
+	}
+
+	/* find the id of the retimer via the addr of the version register */
+	if (res->start == M10BMC_PKVL_A_VER) {
+		retimer->id = 0;
+	} else if (res->start == M10BMC_PKVL_B_VER) {
+		retimer->id = 1;
+	} else {
+		dev_err(&pdev->dev, "version REG resource invalid\n");
+		return -EINVAL;
+	}
+
+	retimer->ver_reg = res->start;
+	retimer->dev = &pdev->dev;
+	retimer->m10bmc = m10bmc;
+
+	dev_set_drvdata(&pdev->dev, retimer);
+
+	return 0;
+}
+
+static struct platform_driver intel_m10bmc_retimer_driver = {
+	.probe = intel_m10bmc_retimer_probe,
+	.driver = {
+		.name = N3000BMC_RETIMER_DEV_NAME,
+		.dev_groups = m10bmc_retimer_groups,
+	},
+};
+module_platform_driver(intel_m10bmc_retimer_driver);
+
+MODULE_ALIAS("platform:" N3000BMC_RETIMER_DEV_NAME);
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("Intel MAX 10 BMC retimer driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Greg KH]

On Thu, Jan 07, 2021 at 02:07:08PM +0800, Xu Yilun wrote:
> This driver supports the ethernet retimers (C827) for the Intel PAC
> (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> 
> C827 is an Intel(R) Ethernet serdes transceiver chip that supports
> up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
> managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
> 10G/25G retimer mode. Host could query their link states and firmware
> version information via retimer interfaces (Shared registers) on Intel
> MAX 10 BMC. The driver creates sysfs interfaces for users to query these
> information.

Networking people, please look at this sysfs file:

> +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> +Date:		Jan 2021
> +KernelVersion:	5.12
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read only. Returns the status of each line side link. "1" for
> +		link up, "0" for link down.
> +		Format: "%u".

as I need your approval to add it because it is not the "normal" way for
link status to be exported to userspace.

One code issue:

> +#define to_link_attr(dev_attr) \
> +	container_of(dev_attr, struct link_attr, attr)
> +
> +static ssize_t
> +link_status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
> +	struct link_attr *lattr = to_link_attr(attr);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = m10bmc_sys_read(retimer->m10bmc, M10BMC_PKVL_LSTATUS, &val);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%u\n",
> +			  !!(val & BIT((retimer->id << 2) + lattr->index)));
> +}
> +
> +#define link_status_attr(_index)				\
> +	static struct link_attr link_attr_status##_index =	\
> +		{ .attr = __ATTR(link_status##_index, 0444,	\
> +				 link_status_show, NULL),	\
> +		  .index = (_index) }

Why is this a "raw" attribute and not a device attribute?

Please just use a normal DEVICE_ATTR_RO() macro to make it simpler and
easier to understand over time, what you are doing here.  I can't
determine what is happening with this code now...

thanks,

greg k-h

Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Andrew Lunn]

On Thu, Jan 07, 2021 at 10:26:12AM +0100, Greg KH wrote:
> On Thu, Jan 07, 2021 at 02:07:08PM +0800, Xu Yilun wrote:
> > This driver supports the ethernet retimers (C827) for the Intel PAC
> > (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> > 
> > C827 is an Intel(R) Ethernet serdes transceiver chip that supports
> > up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
> > managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
> > 10G/25G retimer mode. Host could query their link states and firmware
> > version information via retimer interfaces (Shared registers) on Intel
> > MAX 10 BMC. The driver creates sysfs interfaces for users to query these
> > information.
> 
> Networking people, please look at this sysfs file:
> 
> > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> > +Date:		Jan 2021
> > +KernelVersion:	5.12
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read only. Returns the status of each line side link. "1" for
> > +		link up, "0" for link down.
> > +		Format: "%u".
> 
> as I need your approval to add it because it is not the "normal" way for
> link status to be exported to userspace.

Hi Greg

Correct, this is not going to be acceptable.

The whole architecture needs to cleanly fit into the phylink model for
controlling the SFP and the retimer.

I'm guessing Intel needs to rewrite portions of the BMC firmware to
either transparently pass through access to the SFP socket and the
retimer for phylink and a C827 specific driver, or add a high level
API which a MAC driver can use, and completely hide away these PHY
details from Linux, which is what many of the Intel Ethernet drivers
do.

       Andrew

Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Xu Yilun]

On Thu, Jan 07, 2021 at 03:51:38PM +0100, Andrew Lunn wrote:
> On Thu, Jan 07, 2021 at 10:26:12AM +0100, Greg KH wrote:
> > On Thu, Jan 07, 2021 at 02:07:08PM +0800, Xu Yilun wrote:
> > > This driver supports the ethernet retimers (C827) for the Intel PAC
> > > (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> > > 
> > > C827 is an Intel(R) Ethernet serdes transceiver chip that supports
> > > up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
> > > managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
> > > 10G/25G retimer mode. Host could query their link states and firmware
> > > version information via retimer interfaces (Shared registers) on Intel
> > > MAX 10 BMC. The driver creates sysfs interfaces for users to query these
> > > information.
> > 
> > Networking people, please look at this sysfs file:
> > 
> > > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> > > +Date:		Jan 2021
> > > +KernelVersion:	5.12
> > > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > > +Description:	Read only. Returns the status of each line side link. "1" for
> > > +		link up, "0" for link down.
> > > +		Format: "%u".
> > 
> > as I need your approval to add it because it is not the "normal" way for
> > link status to be exported to userspace.
> 
> Hi Greg
> 
> Correct, this is not going to be acceptable.
> 
> The whole architecture needs to cleanly fit into the phylink model for
> controlling the SFP and the retimer.
> 
> I'm guessing Intel needs to rewrite portions of the BMC firmware to
> either transparently pass through access to the SFP socket and the
> retimer for phylink and a C827 specific driver, or add a high level
> API which a MAC driver can use, and completely hide away these PHY
> details from Linux, which is what many of the Intel Ethernet drivers
> do.

Got it, Thanks for your explanation.

Yilun

Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Xu Yilun]

On Thu, Jan 07, 2021 at 10:26:12AM +0100, Greg KH wrote:
> On Thu, Jan 07, 2021 at 02:07:08PM +0800, Xu Yilun wrote:
> > This driver supports the ethernet retimers (C827) for the Intel PAC
> > (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> > 
> > C827 is an Intel(R) Ethernet serdes transceiver chip that supports
> > up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
> > managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
> > 10G/25G retimer mode. Host could query their link states and firmware
> > version information via retimer interfaces (Shared registers) on Intel
> > MAX 10 BMC. The driver creates sysfs interfaces for users to query these
> > information.
> 
> Networking people, please look at this sysfs file:
> 
> > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> > +Date:		Jan 2021
> > +KernelVersion:	5.12
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read only. Returns the status of each line side link. "1" for
> > +		link up, "0" for link down.
> > +		Format: "%u".
> 
> as I need your approval to add it because it is not the "normal" way for
> link status to be exported to userspace.
> 
> One code issue:
> 
> > +#define to_link_attr(dev_attr) \
> > +	container_of(dev_attr, struct link_attr, attr)
> > +
> > +static ssize_t
> > +link_status_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
> > +	struct link_attr *lattr = to_link_attr(attr);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = m10bmc_sys_read(retimer->m10bmc, M10BMC_PKVL_LSTATUS, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sysfs_emit(buf, "%u\n",
> > +			  !!(val & BIT((retimer->id << 2) + lattr->index)));
> > +}
> > +
> > +#define link_status_attr(_index)				\
> > +	static struct link_attr link_attr_status##_index =	\
> > +		{ .attr = __ATTR(link_status##_index, 0444,	\
> > +				 link_status_show, NULL),	\
> > +		  .index = (_index) }
> 
> Why is this a "raw" attribute and not a device attribute?

It is actually a device_attribute. The device_attribute is embedded in
link_attr, like:

  struct link_attr {
	struct device_attribute attr;
	u32 index;
  };

An index for the link is appended along with the device_attribute, so we
could identify which link is being queried on link_status_show(). There
are 4 links and this is to avoid duplicated code like
link_status_1_show(), link_status_2_show() ...

> 
> Please just use a normal DEVICE_ATTR_RO() macro to make it simpler and

DEVICE_ATTR_RO() is to define a standalone device_attribute variable, but
here we are initializing a field in struct link_attr.

Thanks,
Yilun

> easier to understand over time, what you are doing here.  I can't
> determine what is happening with this code now...
> 
> thanks,
> 
> greg k-h

Re: [RESEND PATCH 2/2] misc: add support for retimers interfaces on Intel MAX 10 BMC

[Greg KH]

On Fri, Jan 08, 2021 at 10:05:26AM +0800, Xu Yilun wrote:
> On Thu, Jan 07, 2021 at 10:26:12AM +0100, Greg KH wrote:
> > On Thu, Jan 07, 2021 at 02:07:08PM +0800, Xu Yilun wrote:
> > > This driver supports the ethernet retimers (C827) for the Intel PAC
> > > (Programmable Acceleration Card) N3000, which is a FPGA based Smart NIC.
> > > 
> > > C827 is an Intel(R) Ethernet serdes transceiver chip that supports
> > > up to 100G transfer. On Intel PAC N3000 there are 2 C827 chips
> > > managed by the Intel MAX 10 BMC firmware. They are configured in 4 ports
> > > 10G/25G retimer mode. Host could query their link states and firmware
> > > version information via retimer interfaces (Shared registers) on Intel
> > > MAX 10 BMC. The driver creates sysfs interfaces for users to query these
> > > information.
> > 
> > Networking people, please look at this sysfs file:
> > 
> > > +What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
> > > +Date:		Jan 2021
> > > +KernelVersion:	5.12
> > > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > > +Description:	Read only. Returns the status of each line side link. "1" for
> > > +		link up, "0" for link down.
> > > +		Format: "%u".
> > 
> > as I need your approval to add it because it is not the "normal" way for
> > link status to be exported to userspace.
> > 
> > One code issue:
> > 
> > > +#define to_link_attr(dev_attr) \
> > > +	container_of(dev_attr, struct link_attr, attr)
> > > +
> > > +static ssize_t
> > > +link_status_show(struct device *dev, struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct m10bmc_retimer *retimer = dev_get_drvdata(dev);
> > > +	struct link_attr *lattr = to_link_attr(attr);
> > > +	unsigned int val;
> > > +	int ret;
> > > +
> > > +	ret = m10bmc_sys_read(retimer->m10bmc, M10BMC_PKVL_LSTATUS, &val);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return sysfs_emit(buf, "%u\n",
> > > +			  !!(val & BIT((retimer->id << 2) + lattr->index)));
> > > +}
> > > +
> > > +#define link_status_attr(_index)				\
> > > +	static struct link_attr link_attr_status##_index =	\
> > > +		{ .attr = __ATTR(link_status##_index, 0444,	\
> > > +				 link_status_show, NULL),	\
> > > +		  .index = (_index) }
> > 
> > Why is this a "raw" attribute and not a device attribute?
> 
> It is actually a device_attribute. The device_attribute is embedded in
> link_attr, like:
> 
>   struct link_attr {
> 	struct device_attribute attr;
> 	u32 index;
>   };
> 
> An index for the link is appended along with the device_attribute, so we
> could identify which link is being queried on link_status_show(). There
> are 4 links and this is to avoid duplicated code like
> link_status_1_show(), link_status_2_show() ...

Duplicated code is better to read than complex code :)

> > Please just use a normal DEVICE_ATTR_RO() macro to make it simpler and
> 
> DEVICE_ATTR_RO() is to define a standalone device_attribute variable, but
> here we are initializing a field in struct link_attr.

Then use the correct initialization macro that is given to you for that,
do not roll your own.

greg k-h