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

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:

quoted

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:

quoted

+What:		/sys/bus/platform/devices/n3000bmc-retimer.*.auto/link_statusX
+Date:		Jan 2021
+KernelVersion:	5.12
+Contact:	Xu Yilun [off-list ref]
+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:

quoted

+#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