[PATCH net] eth: fbnic: fix race between concurrent hwmon sensor reads

[PATCH net] eth: fbnic: fix race between concurrent hwmon sensor reads

[Zinc Lim]

From: Zinc Lim <limzhineng2@gmail.com>

Reading an hwmon sensor issues a TSENE firmware mailbox transaction that
uses a shared completion slot. Concurrent reads (e.g. parallel
"cat .../temp1_input" or a monitoring agent polling all attributes) race
over that slot, and the second transmit fails because a completion is
already pending:

  fbnic 0000:41:00.0: Failed to transmit TSENE read msg, err -17

Serialize the hwmon read path with a per-device mutex so only one TSENE
transaction is in flight at a time.

Fixes: 880630734102 ("eth: fbnic: Add hardware monitoring support via HWMON interface")
Signed-off-by: Zinc Lim <limzhineng2@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic.h       |  2 ++
 drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c | 15 +++++++++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index d0715695c43e..e31d6f88b746 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -6,6 +6,7 @@
 
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/mutex.h>
 #include <linux/ptp_clock_kernel.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
@@ -27,6 +28,7 @@ struct fbnic_dev {
 	struct net_device *netdev;
 	struct dentry *dbg_fbd;
 	struct device *hwmon;
+	struct mutex hwmon_mutex; /* Serializes hwmon sensor reads */
 	struct devlink_health_reporter *fw_reporter;
 	struct devlink_health_reporter *otp_reporter;
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
index def8598aceec..ac1b4a422677 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_hwmon.c
@@ -33,10 +33,17 @@ static int fbnic_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 {
 	struct fbnic_dev *fbd = dev_get_drvdata(dev);
 	const struct fbnic_mac *mac = fbd->mac;
-	int id;
+	int id, err;
 
 	id = fbnic_hwmon_sensor_id(type);
-	return id < 0 ? id : mac->get_sensor(fbd, id, val);
+	if (id < 0)
+		return id;
+
+	mutex_lock(&fbd->hwmon_mutex);
+	err = mac->get_sensor(fbd, id, val);
+	mutex_unlock(&fbd->hwmon_mutex);
+
+	return err;
 }
 
 static const struct hwmon_ops fbnic_hwmon_ops = {
@@ -60,6 +67,8 @@ void fbnic_hwmon_register(struct fbnic_dev *fbd)
 	if (!IS_REACHABLE(CONFIG_HWMON))
 		return;
 
+	mutex_init(&fbd->hwmon_mutex);
+
 	fbd->hwmon = hwmon_device_register_with_info(fbd->dev, "fbnic",
 						     fbd, &fbnic_chip_info,
 						     NULL);
@@ -68,6 +77,7 @@ void fbnic_hwmon_register(struct fbnic_dev *fbd)
 			   "Failed to register hwmon device %pe\n",
 			   fbd->hwmon);
 		fbd->hwmon = NULL;
+		mutex_destroy(&fbd->hwmon_mutex);
 	}
 }
 
@@ -78,4 +88,5 @@ void fbnic_hwmon_unregister(struct fbnic_dev *fbd)
 
 	hwmon_device_unregister(fbd->hwmon);
 	fbd->hwmon = NULL;
+	mutex_destroy(&fbd->hwmon_mutex);
 }
-- 
2.53.0-Meta

Re: [PATCH net] eth: fbnic: fix race between concurrent hwmon sensor reads

[Andrew Lunn]

On Wed, Jun 24, 2026 at 01:05:14PM -0700, Zinc Lim wrote:
> From: Zinc Lim <limzhineng2@gmail.com>
> 
> Reading an hwmon sensor

Since this is a hwmon related patch, it is a good idea to Cc: the
hwmon Maintainer.

I don't know hwmon too well, i've never had to look at locking, but...

static int hwmon_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
{
	struct hwmon_thermal_data *tdata = thermal_zone_device_priv(tz);
	struct hwmon_device *hwdev = to_hwmon_device(tdata->dev);
	int ret;
	long t;

	guard(mutex)(&hwdev->lock);

	ret = hwdev->chip->ops->read(tdata->dev, hwmon_temp, hwmon_temp_input,
				     tdata->index, &t);

Could you explain why this lock in the hwmon core is not sufficient?
It could be i'm reading it wrong, but it looks like the lock is shared
by all hwmon instanced registered by a
hwmon_device_register_with_info() call.

	Andrew

Re: [PATCH net] eth: fbnic: fix race between concurrent hwmon sensor reads

[Andrew Lunn]

On Wed, Jun 24, 2026 at 11:51:29PM +0200, Andrew Lunn wrote:
> On Wed, Jun 24, 2026 at 01:05:14PM -0700, Zinc Lim wrote:
> > From: Zinc Lim <limzhineng2@gmail.com>
> > 
> > Reading an hwmon sensor
> 
> Since this is a hwmon related patch, it is a good idea to Cc: the
> hwmon Maintainer.
> 
> I don't know hwmon too well, i've never had to look at locking, but...
> 
> static int hwmon_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> {
> 	struct hwmon_thermal_data *tdata = thermal_zone_device_priv(tz);
> 	struct hwmon_device *hwdev = to_hwmon_device(tdata->dev);
> 	int ret;
> 	long t;
> 
> 	guard(mutex)(&hwdev->lock);
> 
> 	ret = hwdev->chip->ops->read(tdata->dev, hwmon_temp, hwmon_temp_input,
> 				     tdata->index, &t);
> 
> Could you explain why this lock in the hwmon core is not sufficient?
> It could be i'm reading it wrong, but it looks like the lock is shared
> by all hwmon instanced registered by a
> hwmon_device_register_with_info() call.

Documentation/hwmon/hwmon-kernel-api.rst 

When using ``[devm_]hwmon_device_register_with_info()`` to register the
hardware monitoring device, accesses using the associated access functions
are serialised by the hardware monitoring core. If a driver needs locking
for other functions such as interrupt handlers or for attributes which are
fully implemented in the driver, hwmon_lock() and hwmon_unlock() can be used
to ensure that calls to those functions are serialized.

	Andrew

Re: [PATCH net] eth: fbnic: fix race between concurrent hwmon sensor reads

[Alexander Duyck]

On Wed, Jun 24, 2026 at 2:56 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Jun 24, 2026 at 11:51:29PM +0200, Andrew Lunn wrote:
> > On Wed, Jun 24, 2026 at 01:05:14PM -0700, Zinc Lim wrote:
> > > From: Zinc Lim <limzhineng2@gmail.com>
> > >
> > > Reading an hwmon sensor
> >
> > Since this is a hwmon related patch, it is a good idea to Cc: the
> > hwmon Maintainer.
> >
> > I don't know hwmon too well, i've never had to look at locking, but...
> >
> > static int hwmon_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> > {
> >       struct hwmon_thermal_data *tdata = thermal_zone_device_priv(tz);
> >       struct hwmon_device *hwdev = to_hwmon_device(tdata->dev);
> >       int ret;
> >       long t;
> >
> >       guard(mutex)(&hwdev->lock);
> >
> >       ret = hwdev->chip->ops->read(tdata->dev, hwmon_temp, hwmon_temp_input,
> >                                    tdata->index, &t);
> >
> > Could you explain why this lock in the hwmon core is not sufficient?
> > It could be i'm reading it wrong, but it looks like the lock is shared
> > by all hwmon instanced registered by a
> > hwmon_device_register_with_info() call.
>
> Documentation/hwmon/hwmon-kernel-api.rst
>
> When using ``[devm_]hwmon_device_register_with_info()`` to register the
> hardware monitoring device, accesses using the associated access functions
> are serialised by the hardware monitoring core. If a driver needs locking
> for other functions such as interrupt handlers or for attributes which are
> fully implemented in the driver, hwmon_lock() and hwmon_unlock() can be used
> to ensure that calls to those functions are serialized.

I did some digging and it looks like 3ad2a7b9b15d ("hwmon: Serialize
accesses in hwmon core") landed into 6.18. This was a patch put
together based on issues reported in an earlier kernel version and
explains how this got missed. So what we have is a redundant fix. We
can drop this for upstream and probably look at pulling the upstream
fix into our kernels that were having the issues.

Thanks for the review feedback.

- Alex