Re: [PATCH net-next v5] eth: fbnic: Add hardware monitoring support via HWMON interface

From: Sanman Pradhan
Date: Fri Oct 11 2024 - 20:02:16 EST

On Thu, 10 Oct 2024 at 21:34, Kalesh Anakkur Purayil
<kalesh-anakkur.purayil@xxxxxxxxxxxx> wrote:
>
> On Thu, Oct 10, 2024 at 12:50 AM Sanman Pradhan
> <sanman.p211993@xxxxxxxxx> wrote:
> >
> > From: Sanman Pradhan <sanmanpradhan@xxxxxxxx>
> >
> > 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@xxxxxxxx>
> >
> > ---
> > v5:
> > - Drop hwmon_unregister label from fbnic_pci.c
> >
> > v4: https://patchwork.kernel.org/project/netdevbpf/patch/20241008143212.2354554-1-sanman.p211993@xxxxxxxxx/
> >
> > v3: https://patchwork.kernel.org/project/netdevbpf/patch/20241004204953.2223536-1-sanman.p211993@xxxxxxxxx/
> >
> > v2: https://patchwork.kernel.org/project/netdevbpf/patch/20241003173618.2479520-1-sanman.p211993@xxxxxxxxx/
> >
> > v1: https://lore.kernel.org/netdev/153c5be4-158e-421a-83a5-5632a9263e87@xxxxxxxxxxxx/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 | 3 +
> > 5 files changed, 96 insertions(+)
> > 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);
> Thank you for addressing the comments. One last question.
> [Kalesh] I do not see the corresponding implementation of this
> function. Am I missing soemthing here?
> > };
> >
> > 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..ef9dc8c67927 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;
> > @@ -345,6 +347,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
> >
>
>
> --
> Regards,
> Kalesh A P

Thank you Kalesh, for pointing out the corresponding implementation
and reviewing the patch. I've submitted v6 of my patch for review.

Thank you.

Regards,
Sanman Pradhan