diff mbox series

[net-next,v3] eth: fbnic: Add hardware monitoring support via HWMON interface

Message ID 20241004204953.2223536-1-sanman.p211993@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v3] eth: fbnic: Add hardware monitoring support via HWMON interface | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 6 this patch: 6
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 7 this patch: 7
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 9 this patch: 9
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Sanman Pradhan Oct. 4, 2024, 8:49 p.m. UTC 
From: Sanman Pradhan <sanmanpradhan@meta.com>

This patch adds support for hardware monitoring to the fbnic driver,
allowing for temperature and voltage sensor data to be exposed to
userspace via the HWMON interface. The driver registers a HWMON device
and provides callbacks for reading sensor data, enabling system
admins to monitor the health and operating conditions of fbnic.

Signed-off-by: Sanman Pradhan <sanmanpradhan@meta.com>

---
v3:
  - Add missing "id" initialization in fbnic_hwmon_read
  - Change ifm_hwmon_unregister to hwmon_unregister

v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241003173618.2479520-1-sanman.p211993@gmail.com/

v1: https://lore.kernel.org/netdev/153c5be4-158e-421a-83a5-5632a9263e87@roeck-us.net/T/

---
 drivers/net/ethernet/meta/fbnic/Makefile      |  1 +
 drivers/net/ethernet/meta/fbnic/fbnic.h       |  4 +
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 81 +++++++++++++++++++
 drivers/net/ethernet/meta/fbnic/fbnic_mac.h   |  7 ++
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c   |  7 +-
 5 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c

--
2.43.5

Comments

Kalesh Anakkur Purayil Oct. 5, 2024, 4:23 a.m. UTC | #1 
Hi Sanman,

It looks like you missed one comment. See in line.

On Sat, Oct 5, 2024 at 2:20 AM Sanman Pradhan <sanman.p211993@gmail.com> wrote:
>
> From: Sanman Pradhan <sanmanpradhan@meta.com>
>
> This patch adds support for hardware monitoring to the fbnic driver,
> allowing for temperature and voltage sensor data to be exposed to
> userspace via the HWMON interface. The driver registers a HWMON device
> and provides callbacks for reading sensor data, enabling system
> admins to monitor the health and operating conditions of fbnic.
>
> Signed-off-by: Sanman Pradhan <sanmanpradhan@meta.com>
>
> ---
> v3:
>   - Add missing "id" initialization in fbnic_hwmon_read
>   - Change ifm_hwmon_unregister to hwmon_unregister
>
> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241003173618.2479520-1-sanman.p211993@gmail.com/
>
> v1: https://lore.kernel.org/netdev/153c5be4-158e-421a-83a5-5632a9263e87@roeck-us.net/T/
>
> ---
>  drivers/net/ethernet/meta/fbnic/Makefile      |  1 +
>  drivers/net/ethernet/meta/fbnic/fbnic.h       |  4 +
>  drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 81 +++++++++++++++++++
>  drivers/net/ethernet/meta/fbnic/fbnic_mac.h   |  7 ++
>  drivers/net/ethernet/meta/fbnic/fbnic_pci.c   |  7 +-
>  5 files changed, 99 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
>
> diff --git a/drivers/net/ethernet/meta/fbnic/Makefile b/drivers/net/ethernet/meta/fbnic/Makefile
> index ed4533a73c57..41494022792a 100644
> --- a/drivers/net/ethernet/meta/fbnic/Makefile
> +++ b/drivers/net/ethernet/meta/fbnic/Makefile
> @@ -11,6 +11,7 @@ fbnic-y := fbnic_devlink.o \
>            fbnic_ethtool.o \
>            fbnic_fw.o \
>            fbnic_hw_stats.o \
> +          fbnic_hwmon.o \
>            fbnic_irq.o \
>            fbnic_mac.o \
>            fbnic_netdev.o \
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
> index 0f9e8d79461c..2d3aa20bc876 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
> @@ -18,6 +18,7 @@
>  struct fbnic_dev {
>         struct device *dev;
>         struct net_device *netdev;
> +       struct device *hwmon;
>
>         u32 __iomem *uc_addr0;
>         u32 __iomem *uc_addr4;
> @@ -127,6 +128,9 @@ void fbnic_devlink_unregister(struct fbnic_dev *fbd);
>  int fbnic_fw_enable_mbx(struct fbnic_dev *fbd);
>  void fbnic_fw_disable_mbx(struct fbnic_dev *fbd);
>
> +void fbnic_hwmon_register(struct fbnic_dev *fbd);
> +void fbnic_hwmon_unregister(struct fbnic_dev *fbd);
> +
>  int fbnic_pcs_irq_enable(struct fbnic_dev *fbd);
>  void fbnic_pcs_irq_disable(struct fbnic_dev *fbd);
>
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> new file mode 100644
> index 000000000000..bcd1086e3768
> --- /dev/null
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) Meta Platforms, Inc. and affiliates. */
> +
> +#include <linux/hwmon.h>
> +
> +#include "fbnic.h"
> +#include "fbnic_mac.h"
> +
> +static int fbnic_hwmon_sensor_id(enum hwmon_sensor_types type)
> +{
> +       if (type == hwmon_temp)
> +               return FBNIC_SENSOR_TEMP;
> +       if (type == hwmon_in)
> +               return FBNIC_SENSOR_VOLTAGE;
> +
> +       return -EOPNOTSUPP;
> +}
> +
> +static umode_t fbnic_hwmon_is_visible(const void *drvdata,
> +                                     enum hwmon_sensor_types type,
> +                                     u32 attr, int channel)
> +{
> +       if (type == hwmon_temp && attr == hwmon_temp_input)
> +               return 0444;
> +       if (type == hwmon_in && attr == hwmon_in_input)
> +               return 0444;
> +
> +       return 0;
> +}
> +
> +static int fbnic_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +                           u32 attr, int channel, long *val)
> +{
> +       struct fbnic_dev *fbd = dev_get_drvdata(dev);
> +       const struct fbnic_mac *mac = fbd->mac;
> +       int id;
> +
> +       id = fbnic_hwmon_sensor_id(type);
> +       return id < 0 ? id : mac->get_sensor(fbd, id, val);
> +}
> +
> +static const struct hwmon_ops fbnic_hwmon_ops = {
> +       .is_visible = fbnic_hwmon_is_visible,
> +       .read = fbnic_hwmon_read,
> +};
> +
> +static const struct hwmon_channel_info *fbnic_hwmon_info[] = {
> +       HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> +       HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
> +       NULL
> +};
> +
> +static const struct hwmon_chip_info fbnic_chip_info = {
> +       .ops = &fbnic_hwmon_ops,
> +       .info = fbnic_hwmon_info,
> +};
> +
> +void fbnic_hwmon_register(struct fbnic_dev *fbd)
> +{
> +       if (!IS_REACHABLE(CONFIG_HWMON))
> +               return;
> +
> +       fbd->hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic",
> +                                                    fbd, &fbnic_chip_info,
> +                                                    NULL);
> +       if (IS_ERR(fbd->hwmon)) {
> +               dev_notice(fbd->dev,
> +                          "Failed to register hwmon device %pe\n",
> +                       fbd->hwmon);
> +               fbd->hwmon = NULL;
> +       }
> +}
> +
> +void fbnic_hwmon_unregister(struct fbnic_dev *fbd)
> +{
> +       if (!IS_REACHABLE(CONFIG_HWMON) || !fbd->hwmon)
> +               return;
> +
> +       hwmon_device_unregister(fbd->hwmon);
> +       fbd->hwmon = NULL;
> +}
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mac.h b/drivers/net/ethernet/meta/fbnic/fbnic_mac.h
> index 476239a9d381..05a591653e09 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_mac.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_mac.h
> @@ -47,6 +47,11 @@ enum {
>  #define FBNIC_LINK_MODE_PAM4   (FBNIC_LINK_50R1)
>  #define FBNIC_LINK_MODE_MASK   (FBNIC_LINK_AUTO - 1)
>
> +enum fbnic_sensor_id {
> +       FBNIC_SENSOR_TEMP,              /* Temp in millidegrees Centigrade */
> +       FBNIC_SENSOR_VOLTAGE,           /* Voltage in millivolts */
> +};
> +
>  /* This structure defines the interface hooks for the MAC. The MAC hooks
>   * will be configured as a const struct provided with a set of function
>   * pointers.
> @@ -83,6 +88,8 @@ struct fbnic_mac {
>
>         void (*link_down)(struct fbnic_dev *fbd);
>         void (*link_up)(struct fbnic_dev *fbd, bool tx_pause, bool rx_pause);
> +
> +       int (*get_sensor)(struct fbnic_dev *fbd, int id, long *val);
>  };
>
>  int fbnic_mac_init(struct fbnic_dev *fbd);
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index a4809fe0fc24..debd98ea55e2 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -289,6 +289,8 @@ static int fbnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>
>         fbnic_devlink_register(fbd);
>
> +       fbnic_hwmon_register(fbd);
> +
>         if (!fbd->dsn) {
>                 dev_warn(&pdev->dev, "Reading serial number failed\n");
>                 goto init_failure_mode;
[Kalesh] You should change this label to "hwmon_unregister" as you
should unregister hwmon in case of failure here.

> @@ -297,7 +299,7 @@ static int fbnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>         netdev = fbnic_netdev_alloc(fbd);
>         if (!netdev) {
>                 dev_err(&pdev->dev, "Netdev allocation failed\n");
> -               goto init_failure_mode;
> +               goto hwmon_unregister;
>         }
>
>         err = fbnic_netdev_register(netdev);
> @@ -310,6 +312,8 @@ static int fbnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>
>  ifm_free_netdev:
>         fbnic_netdev_free(fbd);
> +hwmon_unregister:
> +       fbnic_hwmon_unregister(fbd);
>  init_failure_mode:
>         dev_warn(&pdev->dev, "Probe error encountered, entering init failure mode. Normal networking functionality will not be available.\n");
>          /* Always return 0 even on error so devlink is registered to allow
> @@ -345,6 +349,7 @@ static void fbnic_remove(struct pci_dev *pdev)
>                 fbnic_netdev_free(fbd);
>         }
>
> +       fbnic_hwmon_unregister(fbd);
>         fbnic_devlink_unregister(fbd);
>         fbnic_fw_disable_mbx(fbd);
>         fbnic_free_irqs(fbd);
> --
> 2.43.5
>
Jakub Kicinski Oct. 7, 2024, 11:56 p.m. UTC | #2 
On Sat, 5 Oct 2024 09:53:30 +0530 Kalesh Anakkur Purayil wrote:
> [Kalesh] You should change this label to "hwmon_unregister" as you
> should unregister hwmon in case of failure here.

Not really, but you're right that there's a problem with the error path
here..

> > @@ -297,7 +299,7 @@ static int fbnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >         netdev = fbnic_netdev_alloc(fbd);
> >         if (!netdev) {
> >                 dev_err(&pdev->dev, "Netdev allocation failed\n");
> > -               goto init_failure_mode;
> > +               goto hwmon_unregister;
> >         }


.. I don't think we should unregister HWMON if netdev alloc fails.
We will enter "init failure mode" (IFM), and leave the driver bound.
HWMON interface can remain registered, just like devlink and other
auxiliary interfaces.
Kalesh Anakkur Purayil Oct. 8, 2024, 3:20 a.m. UTC | #3 
On Tue, Oct 8, 2024 at 5:26 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 5 Oct 2024 09:53:30 +0530 Kalesh Anakkur Purayil wrote:
> > [Kalesh] You should change this label to "hwmon_unregister" as you
> > should unregister hwmon in case of failure here.
>
> Not really, but you're right that there's a problem with the error path
> here..
>
> > > @@ -297,7 +299,7 @@ static int fbnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >         netdev = fbnic_netdev_alloc(fbd);
> > >         if (!netdev) {
> > >                 dev_err(&pdev->dev, "Netdev allocation failed\n");
> > > -               goto init_failure_mode;
> > > +               goto hwmon_unregister;
> > >         }
>
>
> .. I don't think we should unregister HWMON if netdev alloc fails.
> We will enter "init failure mode" (IFM), and leave the driver bound.
> HWMON interface can remain registered, just like devlink and other
> auxiliary interfaces.
[Kalesh] I see. Thank you Kuba for the explanation.
> --
> pw-bot: cr
diff mbox series

Patch

diff --git a/drivers/net/ethernet/meta/fbnic/Makefile b/drivers/net/ethernet/meta/fbnic/Makefile
index ed4533a73c57..41494022792a 100644
--- a/drivers/net/ethernet/meta/fbnic/Makefile
+++ b/drivers/net/ethernet/meta/fbnic/Makefile
@@ -11,6 +11,7 @@  fbnic-y := fbnic_devlink.o \
 	   fbnic_ethtool.o \
 	   fbnic_fw.o \
 	   fbnic_hw_stats.o \
+	   fbnic_hwmon.o \
 	   fbnic_irq.o \
 	   fbnic_mac.o \
 	   fbnic_netdev.o \
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index 0f9e8d79461c..2d3aa20bc876 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -18,6 +18,7 @@ 
 struct fbnic_dev {
 	struct device *dev;
 	struct net_device *netdev;
+	struct device *hwmon;

 	u32 __iomem *uc_addr0;
 	u32 __iomem *uc_addr4;
@@ -127,6 +128,9 @@  void fbnic_devlink_unregister(struct fbnic_dev *fbd);
 int fbnic_fw_enable_mbx(struct fbnic_dev *fbd);
 void fbnic_fw_disable_mbx(struct fbnic_dev *fbd);

+void fbnic_hwmon_register(struct fbnic_dev *fbd);
+void fbnic_hwmon_unregister(struct fbnic_dev *fbd);
+
 int fbnic_pcs_irq_enable(struct fbnic_dev *fbd);
 void fbnic_pcs_irq_disable(struct fbnic_dev *fbd);

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
new file mode 100644
index 000000000000..bcd1086e3768
--- /dev/null
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -0,0 +1,81 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <linux/hwmon.h>
+
+#include "fbnic.h"
+#include "fbnic_mac.h"
+
+static int fbnic_hwmon_sensor_id(enum hwmon_sensor_types type)
+{
+	if (type == hwmon_temp)
+		return FBNIC_SENSOR_TEMP;
+	if (type == hwmon_in)
+		return FBNIC_SENSOR_VOLTAGE;
+
+	return -EOPNOTSUPP;
+}
+
+static umode_t fbnic_hwmon_is_visible(const void *drvdata,
+				      enum hwmon_sensor_types type,
+				      u32 attr, int channel)
+{
+	if (type == hwmon_temp && attr == hwmon_temp_input)
+		return 0444;
+	if (type == hwmon_in && attr == hwmon_in_input)
+		return 0444;
+
+	return 0;
+}
+
+static int fbnic_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			    u32 attr, int channel, long *val)
+{
+	struct fbnic_dev *fbd = dev_get_drvdata(dev);
+	const struct fbnic_mac *mac = fbd->mac;
+	int id;
+
+	id = fbnic_hwmon_sensor_id(type);
+	return id < 0 ? id : mac->get_sensor(fbd, id, val);
+}
+
+static const struct hwmon_ops fbnic_hwmon_ops = {
+	.is_visible = fbnic_hwmon_is_visible,
+	.read = fbnic_hwmon_read,
+};
+
+static const struct hwmon_channel_info *fbnic_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
+	NULL
+};
+
+static const struct hwmon_chip_info fbnic_chip_info = {
+	.ops = &fbnic_hwmon_ops,
+	.info = fbnic_hwmon_info,
+};
+
+void fbnic_hwmon_register(struct fbnic_dev *fbd)
+{
+	if (!IS_REACHABLE(CONFIG_HWMON))
+		return;
+
+	fbd->hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic",
+						     fbd, &fbnic_chip_info,
+						     NULL);
+	if (IS_ERR(fbd->hwmon)) {
+		dev_notice(fbd->dev,
+			   "Failed to register hwmon device %pe\n",
+			fbd->hwmon);
+		fbd->hwmon = NULL;
+	}
+}
+
+void fbnic_hwmon_unregister(struct fbnic_dev *fbd)
+{
+	if (!IS_REACHABLE(CONFIG_HWMON) || !fbd->hwmon)
+		return;
+
+	hwmon_device_unregister(fbd->hwmon);
+	fbd->hwmon = NULL;
+}
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mac.h b/drivers/net/ethernet/meta/fbnic/fbnic_mac.h
index 476239a9d381..05a591653e09 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_mac.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_mac.h
@@ -47,6 +47,11 @@  enum {
 #define FBNIC_LINK_MODE_PAM4	(FBNIC_LINK_50R1)
 #define FBNIC_LINK_MODE_MASK	(FBNIC_LINK_AUTO - 1)

+enum fbnic_sensor_id {
+	FBNIC_SENSOR_TEMP,		/* Temp in millidegrees Centigrade */
+	FBNIC_SENSOR_VOLTAGE,		/* Voltage in millivolts */
+};
+
 /* This structure defines the interface hooks for the MAC. The MAC hooks
  * will be configured as a const struct provided with a set of function
  * pointers.
@@ -83,6 +88,8 @@  struct fbnic_mac {

 	void (*link_down)(struct fbnic_dev *fbd);
 	void (*link_up)(struct fbnic_dev *fbd, bool tx_pause, bool rx_pause);
+
+	int (*get_sensor)(struct fbnic_dev *fbd, int id, long *val);
 };

 int fbnic_mac_init(struct fbnic_dev *fbd);
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
index a4809fe0fc24..debd98ea55e2 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -289,6 +289,8 @@  static int fbnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

 	fbnic_devlink_register(fbd);

+	fbnic_hwmon_register(fbd);
+
 	if (!fbd->dsn) {
 		dev_warn(&pdev->dev, "Reading serial number failed\n");
 		goto init_failure_mode;
@@ -297,7 +299,7 @@  static int fbnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	netdev = fbnic_netdev_alloc(fbd);
 	if (!netdev) {
 		dev_err(&pdev->dev, "Netdev allocation failed\n");
-		goto init_failure_mode;
+		goto hwmon_unregister;
 	}

 	err = fbnic_netdev_register(netdev);
@@ -310,6 +312,8 @@  static int fbnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

 ifm_free_netdev:
 	fbnic_netdev_free(fbd);
+hwmon_unregister:
+	fbnic_hwmon_unregister(fbd);
 init_failure_mode:
 	dev_warn(&pdev->dev, "Probe error encountered, entering init failure mode. Normal networking functionality will not be available.\n");
 	 /* Always return 0 even on error so devlink is registered to allow
@@ -345,6 +349,7 @@  static void fbnic_remove(struct pci_dev *pdev)
 		fbnic_netdev_free(fbd);
 	}

+	fbnic_hwmon_unregister(fbd);
 	fbnic_devlink_unregister(fbd);
 	fbnic_fw_disable_mbx(fbd);
 	fbnic_free_irqs(fbd);