[PATCH net 0/3] Add I2C bus lock for Wangxun

[PATCH net 0/3] Add I2C bus lock for Wangxun

[Jiawen Wu]

Sometimes the driver can not get the SFP information because the I2C bus
is accessed by the firmware at the same time. So we need to add the lock
on the I2C bus access. The hardware semaphores perform this lock.

Jiawen Wu (3):
  net: txgbe: add IO address in I2C platform device data
  i2c: designware: add device private data passing to lock functions
  i2c: designware: support hardware lock for Wangxun 10Gb NIC

 drivers/i2c/busses/Makefile                   |  1 +
 drivers/i2c/busses/i2c-designware-amdpsp.c    |  4 +-
 drivers/i2c/busses/i2c-designware-baytrail.c  | 14 +++-
 drivers/i2c/busses/i2c-designware-common.c    |  4 +-
 drivers/i2c/busses/i2c-designware-core.h      |  6 +-
 drivers/i2c/busses/i2c-designware-platdrv.c   |  3 +
 drivers/i2c/busses/i2c-designware-wx.c        | 65 +++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    |  5 ++
 include/linux/platform_data/i2c-wx.h          | 11 ++++
 9 files changed, 105 insertions(+), 8 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-designware-wx.c
 create mode 100644 include/linux/platform_data/i2c-wx.h

-- 
2.27.0

[PATCH net 1/3] net: txgbe: add IO address in I2C platform device data

[Jiawen Wu]

Consider the necessity of reading/writing the IO address to acquire and
release the lock between software and firmware, add the IO address as
the platform data to register I2C platform device.

Cc: stable@vger.kernel.org
Fixes: c625e72561f6 ("net: txgbe: Register I2C platform device")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c |  5 +++++
 include/linux/platform_data/i2c-wx.h           | 11 +++++++++++
 2 files changed, 16 insertions(+)
 create mode 100644 include/linux/platform_data/i2c-wx.h

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index 5f502265f0a6..781a3a34aa4c 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -9,6 +9,7 @@
 #include <linux/i2c.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/platform_data/i2c-wx.h>
 #include <linux/regmap.h>
 #include <linux/pcs/pcs-xpcs.h>
 #include <linux/phylink.h>
@@ -618,6 +619,7 @@ static const struct regmap_config i2c_regmap_config = {
 
 static int txgbe_i2c_register(struct txgbe *txgbe)
 {
+	struct txgbe_i2c_platform_data pdata = {};
 	struct platform_device_info info = {};
 	struct platform_device *i2c_dev;
 	struct regmap *i2c_regmap;
@@ -636,6 +638,9 @@ static int txgbe_i2c_register(struct txgbe *txgbe)
 	info.fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_I2C]);
 	info.name = "i2c_designware";
 	info.id = pci_dev_id(pdev);
+	pdata.hw_addr = wx->hw_addr;
+	info.data = &pdata;
+	info.size_data = sizeof(pdata);
 
 	info.res = &DEFINE_RES_IRQ(pdev->irq);
 	info.num_res = 1;
diff --git a/include/linux/platform_data/i2c-wx.h b/include/linux/platform_data/i2c-wx.h
new file mode 100644
index 000000000000..b46777fa1d85
--- /dev/null
+++ b/include/linux/platform_data/i2c-wx.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2015 - 2024 Beijing WangXun Technology Co., Ltd. */
+
+#ifndef _I2C_WX_H_
+#define _I2C_WX_H_
+
+struct txgbe_i2c_platform_data {
+	void __iomem *hw_addr;
+};
+
+#endif /* _I2C_WX_H_ */
-- 
2.27.0

Re: [PATCH net 1/3] net: txgbe: add IO address in I2C platform device data

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:02:40AM +0800, Jiawen Wu wrote:
> Consider the necessity of reading/writing the IO address to acquire and
> release the lock between software and firmware, add the IO address as
> the platform data to register I2C platform device.

...

> +#include <linux/platform_data/i2c-wx.h>

I don't like this. Can you provide a property for that or so?

-- 
With Best Regards,
Andy Shevchenko

[PATCH net 2/3] i2c: designware: add device private data passing to lock functions

[Jiawen Wu]

In order to add the hardware lock for Wangxun devices with minimal
modification, pass struct dw_i2c_dev to the acquire and release lock
functions.

Cc: stable@vger.kernel.org
Fixes: 2f8d1ed79345 ("i2c: designware: Add driver support for Wangxun 10Gb NIC")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/i2c/busses/i2c-designware-amdpsp.c   |  4 ++--
 drivers/i2c/busses/i2c-designware-baytrail.c | 14 ++++++++++++--
 drivers/i2c/busses/i2c-designware-common.c   |  4 ++--
 drivers/i2c/busses/i2c-designware-core.h     |  4 ++--
 4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c
index 63454b06e5da..ee7cc4b33f4b 100644
--- a/drivers/i2c/busses/i2c-designware-amdpsp.c
+++ b/drivers/i2c/busses/i2c-designware-amdpsp.c
@@ -167,7 +167,7 @@ static void psp_release_i2c_bus_deferred(struct work_struct *work)
 }
 static DECLARE_DELAYED_WORK(release_queue, psp_release_i2c_bus_deferred);
 
-static int psp_acquire_i2c_bus(void)
+static int psp_acquire_i2c_bus(struct dw_i2c_dev *dev)
 {
 	int status;
 
@@ -206,7 +206,7 @@ static int psp_acquire_i2c_bus(void)
 	return 0;
 }
 
-static void psp_release_i2c_bus(void)
+static void psp_release_i2c_bus(struct dw_i2c_dev *dev)
 {
 	mutex_lock(&psp_i2c_access_mutex);
 
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 45774aa47c28..9dde796e0fcc 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -12,6 +12,16 @@
 
 #include "i2c-designware-core.h"
 
+static int iosf_mbi_block_punit_i2c_access_dev(struct dw_i2c_dev *dev)
+{
+	return iosf_mbi_block_punit_i2c_access();
+}
+
+static void iosf_mbi_unblock_punit_i2c_access_dev(struct dw_i2c_dev *dev)
+{
+	return iosf_mbi_unblock_punit_i2c_access();
+}
+
 int i2c_dw_baytrail_probe_lock_support(struct dw_i2c_dev *dev)
 {
 	acpi_status status;
@@ -36,8 +46,8 @@ int i2c_dw_baytrail_probe_lock_support(struct dw_i2c_dev *dev)
 		return -EPROBE_DEFER;
 
 	dev_info(dev->dev, "I2C bus managed by PUNIT\n");
-	dev->acquire_lock = iosf_mbi_block_punit_i2c_access;
-	dev->release_lock = iosf_mbi_unblock_punit_i2c_access;
+	dev->acquire_lock = iosf_mbi_block_punit_i2c_access_dev;
+	dev->release_lock = iosf_mbi_unblock_punit_i2c_access_dev;
 	dev->shared_with_punit = true;
 
 	return 0;
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e8a688d04aee..743875090356 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -524,7 +524,7 @@ int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
 	if (!dev->acquire_lock)
 		return 0;
 
-	ret = dev->acquire_lock();
+	ret = dev->acquire_lock(dev);
 	if (!ret)
 		return 0;
 
@@ -536,7 +536,7 @@ int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
 void i2c_dw_release_lock(struct dw_i2c_dev *dev)
 {
 	if (dev->release_lock)
-		dev->release_lock();
+		dev->release_lock(dev);
 }
 
 /*
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index e9606c00b8d1..12b77f464fb5 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -291,8 +291,8 @@ struct dw_i2c_dev {
 	u16			fp_lcnt;
 	u16			hs_hcnt;
 	u16			hs_lcnt;
-	int			(*acquire_lock)(void);
-	void			(*release_lock)(void);
+	int			(*acquire_lock)(struct dw_i2c_dev *dev);
+	void			(*release_lock)(struct dw_i2c_dev *dev);
 	int			semaphore_idx;
 	bool			shared_with_punit;
 	void			(*disable)(struct dw_i2c_dev *dev);
-- 
2.27.0

Re: [PATCH net 2/3] i2c: designware: add device private data passing to lock functions

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:02:41AM +0800, Jiawen Wu wrote:
> In order to add the hardware lock for Wangxun devices with minimal
> modification, pass struct dw_i2c_dev to the acquire and release lock
> functions.

...

> +static int iosf_mbi_block_punit_i2c_access_dev(struct dw_i2c_dev *dev)

> +static void iosf_mbi_unblock_punit_i2c_access_dev(struct dw_i2c_dev *dev)

Rather name them in accordance with the namespace of this module.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net 2/3] i2c: designware: add device private data passing to lock functions

[Paolo Abeni]

On 8/23/24 05:02, Jiawen Wu wrote:
> In order to add the hardware lock for Wangxun devices with minimal
> modification, pass struct dw_i2c_dev to the acquire and release lock
> functions.
> 
> Cc: stable@vger.kernel.org
> Fixes: 2f8d1ed79345 ("i2c: designware: Add driver support for Wangxun 10Gb NIC")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>   drivers/i2c/busses/i2c-designware-amdpsp.c   |  4 ++--
>   drivers/i2c/busses/i2c-designware-baytrail.c | 14 ++++++++++++--
>   drivers/i2c/busses/i2c-designware-common.c   |  4 ++--
>   drivers/i2c/busses/i2c-designware-core.h     |  4 ++--
>   4 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c
> index 63454b06e5da..ee7cc4b33f4b 100644
> --- a/drivers/i2c/busses/i2c-designware-amdpsp.c
> +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c
> @@ -167,7 +167,7 @@ static void psp_release_i2c_bus_deferred(struct work_struct *work)
>   }
>   static DECLARE_DELAYED_WORK(release_queue, psp_release_i2c_bus_deferred);
>   
> -static int psp_acquire_i2c_bus(void)
> +static int psp_acquire_i2c_bus(struct dw_i2c_dev *dev)
>   {
>   	int status;
>   

This function is used in a few other places in this compilation unit. 
You need to update all the users accordingly.

> @@ -206,7 +206,7 @@ static int psp_acquire_i2c_bus(void)
>   	return 0;
>   }
>   
> -static void psp_release_i2c_bus(void)
> +static void psp_release_i2c_bus(struct dw_i2c_dev *dev)
>   {
>   	mutex_lock(&psp_i2c_access_mutex);
>   

The same here.

Cheers,

Paolo

Re: [PATCH net 2/3] i2c: designware: add device private data passing to lock functions

[kernel test robot]

Hi Jiawen,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Jiawen-Wu/net-txgbe-add-IO-address-in-I2C-platform-device-data/20240826-122232
base:   net/main
patch link:    https://lore.kernel.org/r/20240823030242.3083528-3-jiawenwu%40trustnetic.com
patch subject: [PATCH net 2/3] i2c: designware: add device private data passing to lock functions
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240829/202408291212.L5DejDpz-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240829/202408291212.L5DejDpz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408291212.L5DejDpz-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-designware-amdpsp.c:246:22: error: too few arguments to function call, single argument 'dev' was not specified
     246 |         psp_acquire_i2c_bus();
         |         ~~~~~~~~~~~~~~~~~~~ ^
   drivers/i2c/busses/i2c-designware-amdpsp.c:170:12: note: 'psp_acquire_i2c_bus' declared here
     170 | static int psp_acquire_i2c_bus(struct dw_i2c_dev *dev)
         |            ^                   ~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-designware-amdpsp.c:259:22: error: too few arguments to function call, single argument 'dev' was not specified
     259 |         psp_acquire_i2c_bus();
         |         ~~~~~~~~~~~~~~~~~~~ ^
   drivers/i2c/busses/i2c-designware-amdpsp.c:170:12: note: 'psp_acquire_i2c_bus' declared here
     170 | static int psp_acquire_i2c_bus(struct dw_i2c_dev *dev)
         |            ^                   ~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-designware-amdpsp.c:267:22: error: too few arguments to function call, single argument 'dev' was not specified
     267 |         psp_release_i2c_bus();
         |         ~~~~~~~~~~~~~~~~~~~ ^
   drivers/i2c/busses/i2c-designware-amdpsp.c:209:13: note: 'psp_release_i2c_bus' declared here
     209 | static void psp_release_i2c_bus(struct dw_i2c_dev *dev)
         |             ^                   ~~~~~~~~~~~~~~~~~~~~~~
   3 errors generated.


vim +/dev +246 drivers/i2c/busses/i2c-designware-amdpsp.c

78d5e9e299e31bc Jan Dabros 2022-02-08  235  
78d5e9e299e31bc Jan Dabros 2022-02-08  236  /*
78d5e9e299e31bc Jan Dabros 2022-02-08  237   * Locking methods are based on the default implementation from
78d5e9e299e31bc Jan Dabros 2022-02-08  238   * drivers/i2c/i2c-core-base.c, but with psp acquire and release operations
78d5e9e299e31bc Jan Dabros 2022-02-08  239   * added. With this in place we can ensure that i2c clients on the bus shared
78d5e9e299e31bc Jan Dabros 2022-02-08  240   * with psp are able to lock HW access to the bus for arbitrary number of
78d5e9e299e31bc Jan Dabros 2022-02-08  241   * operations - that is e.g. write-wait-read.
78d5e9e299e31bc Jan Dabros 2022-02-08  242   */
78d5e9e299e31bc Jan Dabros 2022-02-08  243  static void i2c_adapter_dw_psp_lock_bus(struct i2c_adapter *adapter,
78d5e9e299e31bc Jan Dabros 2022-02-08  244  					unsigned int flags)
78d5e9e299e31bc Jan Dabros 2022-02-08  245  {
78d5e9e299e31bc Jan Dabros 2022-02-08 @246  	psp_acquire_i2c_bus();
78d5e9e299e31bc Jan Dabros 2022-02-08  247  	rt_mutex_lock_nested(&adapter->bus_lock, i2c_adapter_depth(adapter));
78d5e9e299e31bc Jan Dabros 2022-02-08  248  }
78d5e9e299e31bc Jan Dabros 2022-02-08  249  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH net 3/3] i2c: designware: support hardware lock for Wangxun 10Gb NIC

[Jiawen Wu]

Support acquire_lock() and release_lock() for Wangxun 10Gb NIC. Since the
firmware needs to access I2C all the time for some features, the semaphore
is used between software and firmware. The driver should set software
semaphore before accessing I2C bus and release it when it is finished.
Otherwise, there is probability that the correct information on I2C bus
will not be obtained.

Cc: stable@vger.kernel.org
Fixes: 2f8d1ed79345 ("i2c: designware: Add driver support for Wangxun 10Gb NIC")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/i2c/busses/Makefile                 |  1 +
 drivers/i2c/busses/i2c-designware-core.h    |  2 +
 drivers/i2c/busses/i2c-designware-platdrv.c |  3 +
 drivers/i2c/busses/i2c-designware-wx.c      | 65 +++++++++++++++++++++
 4 files changed, 71 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-designware-wx.c

diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 78d0561339e5..f0ad9ebaacaa 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -58,6 +58,7 @@ i2c-designware-core-y					+= i2c-designware-master.o
 i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) 	+= i2c-designware-slave.o
 obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)			+= i2c-designware-platform.o
 i2c-designware-platform-y 				:= i2c-designware-platdrv.o
+i2c-designware-platform-y 				+= i2c-designware-wx.o
 i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_AMDPSP)	+= i2c-designware-amdpsp.o
 i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o
 obj-$(CONFIG_I2C_DESIGNWARE_PCI)			+= i2c-designware-pci.o
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 12b77f464fb5..eae2c4cdc851 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -414,6 +414,8 @@ int i2c_dw_baytrail_probe_lock_support(struct dw_i2c_dev *dev);
 int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev);
 #endif
 
+int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev);
+
 int i2c_dw_validate_speed(struct dw_i2c_dev *dev);
 void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev);
 
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index df3dc1e8093e..49c71ae5b6c1 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -229,6 +229,9 @@ static const struct i2c_dw_semaphore_callbacks i2c_dw_semaphore_cb_table[] = {
 		.probe = i2c_dw_amdpsp_probe_lock_support,
 	},
 #endif
+	{
+		.probe = i2c_dw_txgbe_probe_lock_support,
+	},
 	{}
 };
 
diff --git a/drivers/i2c/busses/i2c-designware-wx.c b/drivers/i2c/busses/i2c-designware-wx.c
new file mode 100644
index 000000000000..0f98160b956d
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-wx.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2015 - 2024 Beijing WangXun Technology Co., Ltd. */
+
+#include <linux/platform_data/i2c-wx.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/pci.h>
+
+#include "i2c-designware-core.h"
+
+#define I2C_DW_TXGBE_REQ_RETRY_CNT	4000
+#define I2C_DW_TXGBE_MNG_SW		0x1E004
+#define I2C_DW_TXGBE_MNG_SW_SM		BIT(0)
+#define I2C_DW_TXGBE_FLUSH		0x10000
+
+static int i2c_dw_txgbe_acquire_lock(struct dw_i2c_dev *dev)
+{
+	void __iomem *req_addr;
+	u32 swsm;
+	int i;
+
+	req_addr = dev->ext + I2C_DW_TXGBE_MNG_SW;
+
+	for (i = 0; i < I2C_DW_TXGBE_REQ_RETRY_CNT; i++) {
+		writel(I2C_DW_TXGBE_MNG_SW_SM, req_addr);
+
+		/* If we set the bit successfully then we got semaphore. */
+		swsm = readl(req_addr);
+		if (swsm & I2C_DW_TXGBE_MNG_SW_SM)
+			break;
+
+		udelay(50);
+	}
+
+	if (i == I2C_DW_TXGBE_REQ_RETRY_CNT)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static void i2c_dw_txgbe_release_lock(struct dw_i2c_dev *dev)
+{
+	writel(0, dev->ext + I2C_DW_TXGBE_MNG_SW);
+	/* flush register status */
+	readl(dev->ext + I2C_DW_TXGBE_FLUSH);
+}
+
+int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev->dev);
+	struct txgbe_i2c_platform_data *pdata;
+
+	pdata = dev_get_platdata(&pdev->dev);
+	if (!pdata)
+		return -ENXIO;
+
+	dev->ext = pdata->hw_addr;
+	if (!dev->ext)
+		return -ENXIO;
+
+	dev->acquire_lock = i2c_dw_txgbe_acquire_lock;
+	dev->release_lock = i2c_dw_txgbe_release_lock;
+
+	return 0;
+}
-- 
2.27.0

Re: [PATCH net 3/3] i2c: designware: support hardware lock for Wangxun 10Gb NIC

[Andy Shevchenko]

On Fri, Aug 23, 2024 at 11:02:42AM +0800, Jiawen Wu wrote:
> Support acquire_lock() and release_lock() for Wangxun 10Gb NIC. Since the
> firmware needs to access I2C all the time for some features, the semaphore
> is used between software and firmware. The driver should set software
> semaphore before accessing I2C bus and release it when it is finished.
> Otherwise, there is probability that the correct information on I2C bus
> will not be obtained.

...

>  i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) 	+= i2c-designware-slave.o

>  i2c-designware-platform-y 				:= i2c-designware-platdrv.o
> +i2c-designware-platform-y 				+= i2c-designware-wx.o

These lines have TABs/spaces mixture. Please fix at least your entry to avoid
this from happening.


...

>  int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev);
>  #endif

^^^

> +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev);

See below.

...

>  		.probe = i2c_dw_amdpsp_probe_lock_support,
>  	},
>  #endif

^^^

> +	{
> +		.probe = i2c_dw_txgbe_probe_lock_support,
> +	},

Do we all need this support? Even if the driver is not compiled? Why?

...

> +#include <linux/platform_data/i2c-wx.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/pci.h>

This is a semi-random list. Please, take your time to understand the core you
wrote. Follow IWYU principle.

...

> +static int i2c_dw_txgbe_acquire_lock(struct dw_i2c_dev *dev)
> +{
> +	void __iomem *req_addr;
> +	u32 swsm;
> +	int i;
> +
> +	req_addr = dev->ext + I2C_DW_TXGBE_MNG_SW;
> +
> +	for (i = 0; i < I2C_DW_TXGBE_REQ_RETRY_CNT; i++) {

Retry loops much better in a form of

	unsigned int retries = ...;
	...
	do {
		...
	} while (--retries);

BUT... see below.

> +		writel(I2C_DW_TXGBE_MNG_SW_SM, req_addr);
> +
> +		/* If we set the bit successfully then we got semaphore. */
> +		swsm = readl(req_addr);
> +		if (swsm & I2C_DW_TXGBE_MNG_SW_SM)
> +			break;
> +
> +		udelay(50);

So, can a macro from iopoll.h be utilised here? Why not?

> +	}
> +
> +	if (i == I2C_DW_TXGBE_REQ_RETRY_CNT)
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}

> +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev->dev);

Why do you need this dance? I.o.w. how pdev is being used here?

> +	struct txgbe_i2c_platform_data *pdata;
> +
> +	pdata = dev_get_platdata(&pdev->dev);
> +	if (!pdata)
> +		return -ENXIO;
> +
> +	dev->ext = pdata->hw_addr;
> +	if (!dev->ext)
> +		return -ENXIO;
> +
> +	dev->acquire_lock = i2c_dw_txgbe_acquire_lock;
> +	dev->release_lock = i2c_dw_txgbe_release_lock;
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH net 3/3] i2c: designware: support hardware lock for Wangxun 10Gb NIC

[Jiawen Wu]

On Fri, Aug 23, 2024 10:13 PM, Andy Shevchenko wrote:
> On Fri, Aug 23, 2024 at 11:02:42AM +0800, Jiawen Wu wrote:
> > Support acquire_lock() and release_lock() for Wangxun 10Gb NIC. Since the
> > firmware needs to access I2C all the time for some features, the semaphore
> > is used between software and firmware. The driver should set software
> > semaphore before accessing I2C bus and release it when it is finished.
> > Otherwise, there is probability that the correct information on I2C bus
> > will not be obtained.
> 
> ...
> 
> >  i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) 	+= i2c-designware-slave.o
> 
> >  i2c-designware-platform-y 				:= i2c-designware-platdrv.o
> > +i2c-designware-platform-y 				+= i2c-designware-wx.o
> 
> These lines have TABs/spaces mixture. Please fix at least your entry to avoid
> this from happening.
> 
> 
> ...
> 
> >  int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev);
> >  #endif
> 
> ^^^
> 
> > +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev);
> 
> See below.
> 
> ...
> 
> >  		.probe = i2c_dw_amdpsp_probe_lock_support,
> >  	},
> >  #endif
> 
> ^^^
> 
> > +	{
> > +		.probe = i2c_dw_txgbe_probe_lock_support,
> > +	},
> 
> Do we all need this support? Even if the driver is not compiled? Why?

I'll add the macro CONFIG_I2C_DESIGNWARE_WX to control it.

> ...
> 
> > +#include <linux/platform_data/i2c-wx.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/pci.h>
> 
> This is a semi-random list. Please, take your time to understand the core you
> wrote. Follow IWYU principle.
> 
> ...
> 
> > +static int i2c_dw_txgbe_acquire_lock(struct dw_i2c_dev *dev)
> > +{
> > +	void __iomem *req_addr;
> > +	u32 swsm;
> > +	int i;
> > +
> > +	req_addr = dev->ext + I2C_DW_TXGBE_MNG_SW;
> > +
> > +	for (i = 0; i < I2C_DW_TXGBE_REQ_RETRY_CNT; i++) {
> 
> Retry loops much better in a form of
> 
> 	unsigned int retries = ...;
> 	...
> 	do {
> 		...
> 	} while (--retries);
> 
> BUT... see below.
> 
> > +		writel(I2C_DW_TXGBE_MNG_SW_SM, req_addr);
> > +
> > +		/* If we set the bit successfully then we got semaphore. */
> > +		swsm = readl(req_addr);
> > +		if (swsm & I2C_DW_TXGBE_MNG_SW_SM)
> > +			break;
> > +
> > +		udelay(50);
> 
> So, can a macro from iopoll.h be utilised here? Why not?

I need to write the register first and then read it in this loop.
It does not seem to apply to the macros in iopoll.h.

> > +	}
> > +
> > +	if (i == I2C_DW_TXGBE_REQ_RETRY_CNT)
> > +		return -ETIMEDOUT;
> > +
> > +	return 0;
> > +}
> 
> > +int i2c_dw_txgbe_probe_lock_support(struct dw_i2c_dev *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev->dev);
> 
> Why do you need this dance? I.o.w. how pdev is being used here?

I'll change to add the data in node property.
 

Re: [PATCH net 3/3] i2c: designware: support hardware lock for Wangxun 10Gb NIC

['Andy Shevchenko']

On Thu, Aug 29, 2024 at 05:15:42PM +0800, Jiawen Wu wrote:
> On Fri, Aug 23, 2024 10:13 PM, Andy Shevchenko wrote:
> > On Fri, Aug 23, 2024 at 11:02:42AM +0800, Jiawen Wu wrote:

...

> > > +static int i2c_dw_txgbe_acquire_lock(struct dw_i2c_dev *dev)
> > > +{
> > > +	void __iomem *req_addr;
> > > +	u32 swsm;
> > > +	int i;
> > > +
> > > +	req_addr = dev->ext + I2C_DW_TXGBE_MNG_SW;
> > > +
> > > +	for (i = 0; i < I2C_DW_TXGBE_REQ_RETRY_CNT; i++) {
> > 
> > Retry loops much better in a form of
> > 
> > 	unsigned int retries = ...;
> > 	...
> > 	do {
> > 		...
> > 	} while (--retries);
> > 
> > BUT... see below.
> > 
> > > +		writel(I2C_DW_TXGBE_MNG_SW_SM, req_addr);
> > > +
> > > +		/* If we set the bit successfully then we got semaphore. */
> > > +		swsm = readl(req_addr);
> > > +		if (swsm & I2C_DW_TXGBE_MNG_SW_SM)
> > > +			break;
> > > +
> > > +		udelay(50);
> > 
> > So, can a macro from iopoll.h be utilised here? Why not?
> 
> I need to write the register first and then read it in this loop.
> It does not seem to apply to the macros in iopoll.h.

I don't see how does it prevent from using read_poll_timeout() macro.
You need to implement a body of the loop as a helper function that you supply
into macro as a parameter.

> > > +	}
> > > +
> > > +	if (i == I2C_DW_TXGBE_REQ_RETRY_CNT)
> > > +		return -ETIMEDOUT;
> > > +
> > > +	return 0;
> > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Jarkko Nikula]

Hi
Hi

On 8/23/24 6:02 AM, Jiawen Wu wrote:
> Sometimes the driver can not get the SFP information because the I2C bus
> is accessed by the firmware at the same time. So we need to add the lock
> on the I2C bus access. The hardware semaphores perform this lock.
> 
> Jiawen Wu (3):
>    net: txgbe: add IO address in I2C platform device data
>    i2c: designware: add device private data passing to lock functions
>    i2c: designware: support hardware lock for Wangxun 10Gb NIC
> 
I was wondering the Fixes tag use in the series. Obviously patchset is 
not fixing a regression so question is what happens when issue occurs?

I don't think e.g. failing I2C transfer with an error code yet qualifies 
backporting into Linux stable?

RE: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Jiawen Wu]

On Fri, Aug 23, 2024 7:05 PM, Jarkko Nikula wrote:
> Hi
> Hi
> 
> On 8/23/24 6:02 AM, Jiawen Wu wrote:
> > Sometimes the driver can not get the SFP information because the I2C bus
> > is accessed by the firmware at the same time. So we need to add the lock
> > on the I2C bus access. The hardware semaphores perform this lock.
> >
> > Jiawen Wu (3):
> >    net: txgbe: add IO address in I2C platform device data
> >    i2c: designware: add device private data passing to lock functions
> >    i2c: designware: support hardware lock for Wangxun 10Gb NIC
> >
> I was wondering the Fixes tag use in the series. Obviously patchset is
> not fixing a regression so question is what happens when issue occurs?
> 
> I don't think e.g. failing I2C transfer with an error code yet qualifies
> backporting into Linux stable?

Ah, I think this was a leftover bug from when I added Wangxun NIC support
in I2C designware, so I added a fix tag.

Re: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Andrew Lunn]

On Fri, Aug 23, 2024 at 11:02:39AM +0800, Jiawen Wu wrote:
> Sometimes the driver can not get the SFP information because the I2C bus
> is accessed by the firmware at the same time.

Please could you explain this some more. What firmware?

There some registers which are clear on read. They don't work when you
have multiple entities reading them.

	Andrew

RE: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Jiawen Wu]

On Mon, Aug 26, 2024 9:33 AM, Andrew Lunn wrote:
> On Fri, Aug 23, 2024 at 11:02:39AM +0800, Jiawen Wu wrote:
> > Sometimes the driver can not get the SFP information because the I2C bus
> > is accessed by the firmware at the same time.
> 
> Please could you explain this some more. What firmware?

It's the firmware of our ethernet devices.

> There some registers which are clear on read. They don't work when you
> have multiple entities reading them.

I'm not trying to multiple access the I2C registers, but these registers cannot
be accessed by other interfaces in the process of reading complete information
each time. So there is a semaphore needed that locks up the entire read process.

Re: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Andrew Lunn]

On Mon, Aug 26, 2024 at 10:04:42AM +0800, Jiawen Wu wrote:
> On Mon, Aug 26, 2024 9:33 AM, Andrew Lunn wrote:
> > On Fri, Aug 23, 2024 at 11:02:39AM +0800, Jiawen Wu wrote:
> > > Sometimes the driver can not get the SFP information because the I2C bus
> > > is accessed by the firmware at the same time.
> > 
> > Please could you explain this some more. What firmware?
> 
> It's the firmware of our ethernet devices.
> 
> > There some registers which are clear on read. They don't work when you
> > have multiple entities reading them.
> 
> I'm not trying to multiple access the I2C registers, but these registers cannot
> be accessed by other interfaces in the process of reading complete information
> each time. So there is a semaphore needed that locks up the entire read process.

More details please.

Linux assume it is driving the hardware. Your firmware cannot be
touching any registers which will clear on read. QSFP states that
registers 3-31 of page 0 are all clear on read, for example. The
firmware should also not be setting any registers, otherwise you can
confuse Linux which assumes registers it set stay set, because it is
controlling the hardware.

Your firmware also needs to handle that Linux can change the page. If
the firmware changes the page, it must restore it back to whatever
page Linux selected, etc.

The fact you are submitting this for net suggests you have seen real
issues. Please describe what those issues are.

	Andrew

RE: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Jiawen Wu]

On Mon, Aug 26, 2024 10:34 AM, Andrew Lunn wrote:
> On Mon, Aug 26, 2024 at 10:04:42AM +0800, Jiawen Wu wrote:
> > On Mon, Aug 26, 2024 9:33 AM, Andrew Lunn wrote:
> > > On Fri, Aug 23, 2024 at 11:02:39AM +0800, Jiawen Wu wrote:
> > > > Sometimes the driver can not get the SFP information because the I2C bus
> > > > is accessed by the firmware at the same time.
> > >
> > > Please could you explain this some more. What firmware?
> >
> > It's the firmware of our ethernet devices.
> >
> > > There some registers which are clear on read. They don't work when you
> > > have multiple entities reading them.
> >
> > I'm not trying to multiple access the I2C registers, but these registers cannot
> > be accessed by other interfaces in the process of reading complete information
> > each time. So there is a semaphore needed that locks up the entire read process.
> 
> More details please.
> 
> Linux assume it is driving the hardware. Your firmware cannot be
> touching any registers which will clear on read. QSFP states that
> registers 3-31 of page 0 are all clear on read, for example. The
> firmware should also not be setting any registers, otherwise you can
> confuse Linux which assumes registers it set stay set, because it is
> controlling the hardware.
> 
> Your firmware also needs to handle that Linux can change the page. If
> the firmware changes the page, it must restore it back to whatever
> page Linux selected, etc.
> 
> The fact you are submitting this for net suggests you have seen real
> issues. Please describe what those issues are.

The error log shows:

[257681.367827] sfp sfp.1025: Host maximum power 1.0W
[257681.370813] txgbe 0000:04:00.1: 31.504 Gb/s available PCIe bandwidth, limited by 8.0 GT/s PCIe x4 link at 0000:02:02.0 (capable
of 63.008 Gb/s with 8.0 GT/s PCIe x8 link)
[257681.373364] txgbe 0000:04:00.1 enp4s0f1: renamed from eth0
[257681.434719] txgbe 0000:04:00.1 enp4s0f1: configuring for inband/10gbase-r link mode
[257681.676747] sfp sfp.1025: EEPROM base structure checksum failure: 0x63 != 0x1f
[257681.676755] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00  ............g...
[257681.676757] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 64  ....FiberStore d
[257681.676759] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53      ...!SFP-10GS
[257681.676760] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f  R-85    A   .R..
[257681.676762] sfp EE: 00000040: 00 81 cd 5b df 25 0a bd 40 f6 c6 ce 47 8e ff ff  ...[.%..@...G...
[257681.676763] sfp EE: 00000050: 10 d8 24 33 44 8e ff ff 10 41 b0 9a ff ff ff ff  ..$3D....A......
 
It looks like some fields are read incorrectly. For comparison, I printed the
 SFP info when it loaded correctly:

[260908.194533] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00  ............g...
[260908.194536] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 20  ....FiberStore
[260908.194538] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53      ...!SFP-10GS
[260908.194540] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f  R-85    A   .R..
[260908.194541] sfp EE: 00000040: 40 63 bd df 40 8e ff ff 10 41 b0 9a ff ff ff ff  @c..@....A......
[260908.194543] sfp EE: 00000050: 10 58 5b 29 41 8e ff ff 10 41 b0 9a ff ff ff ff  .X[)A....A......
[260908.198205] sfp sfp.1025: module FiberStore       SFP-10GSR-85     rev A    sn G1804125607      dc 180605

Since the read mechanism of I2C is to write the offset and read command
first, and then read the target address. I think it's possible that the different
offsets be written at the same time, from Linux and firmware.

Re: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Andrew Lunn]

> > More details please.
> > 
> > Linux assume it is driving the hardware. Your firmware cannot be
> > touching any registers which will clear on read. QSFP states that
> > registers 3-31 of page 0 are all clear on read, for example. The
> > firmware should also not be setting any registers, otherwise you can
> > confuse Linux which assumes registers it set stay set, because it is
> > controlling the hardware.
> > 
> > Your firmware also needs to handle that Linux can change the page. If
> > the firmware changes the page, it must restore it back to whatever
> > page Linux selected, etc.
> > 
> > The fact you are submitting this for net suggests you have seen real
> > issues. Please describe what those issues are.
> 
> The error log shows:
> 
> [257681.367827] sfp sfp.1025: Host maximum power 1.0W
> [257681.370813] txgbe 0000:04:00.1: 31.504 Gb/s available PCIe bandwidth, limited by 8.0 GT/s PCIe x4 link at 0000:02:02.0 (capable
> of 63.008 Gb/s with 8.0 GT/s PCIe x8 link)
> [257681.373364] txgbe 0000:04:00.1 enp4s0f1: renamed from eth0
> [257681.434719] txgbe 0000:04:00.1 enp4s0f1: configuring for inband/10gbase-r link mode
> [257681.676747] sfp sfp.1025: EEPROM base structure checksum failure: 0x63 != 0x1f
> [257681.676755] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00  ............g...
> [257681.676757] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 64  ....FiberStore d
> [257681.676759] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53      ...!SFP-10GS
> [257681.676760] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f  R-85    A   .R..
> [257681.676762] sfp EE: 00000040: 00 81 cd 5b df 25 0a bd 40 f6 c6 ce 47 8e ff ff  ...[.%..@...G...
> [257681.676763] sfp EE: 00000050: 10 d8 24 33 44 8e ff ff 10 41 b0 9a ff ff ff ff  ..$3D....A......
>  
> It looks like some fields are read incorrectly. For comparison, I printed the
>  SFP info when it loaded correctly:
> 
> [260908.194533] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00  ............g...
> [260908.194536] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 20  ....FiberStore
> [260908.194538] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53      ...!SFP-10GS
> [260908.194540] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f  R-85    A   .R..
> [260908.194541] sfp EE: 00000040: 40 63 bd df 40 8e ff ff 10 41 b0 9a ff ff ff ff  @c..@....A......
> [260908.194543] sfp EE: 00000050: 10 58 5b 29 41 8e ff ff 10 41 b0 9a ff ff ff ff  .X[)A....A......
> [260908.198205] sfp sfp.1025: module FiberStore       SFP-10GSR-85     rev A    sn G1804125607      dc 180605
> 
> Since the read mechanism of I2C is to write the offset and read command
> first, and then read the target address. I think it's possible that the different
> offsets be written at the same time, from Linux and firmware.
 
O.K, that is bad. The SFP is totally unreliable...

You however have still not answered my question. What is the firmware
accessing? How does it handle pages?

The hack you have put in place is per i2c transaction. But accessing
pages is likely to be multiple transactions. One to change the page,
followed by a few reads/writes in the new page, then maybe followed by
a transactions to return to page 0.

I think your best solution is to simply take the mutex and never
release it. Block your firmware from accessing the SFP.

	Andrew

RE: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Jiawen Wu]

On Tue, Aug 27, 2024 8:19 PM, Andrew Lunn wrote:
> > > More details please.
> > >
> > > Linux assume it is driving the hardware. Your firmware cannot be
> > > touching any registers which will clear on read. QSFP states that
> > > registers 3-31 of page 0 are all clear on read, for example. The
> > > firmware should also not be setting any registers, otherwise you can
> > > confuse Linux which assumes registers it set stay set, because it is
> > > controlling the hardware.
> > >
> > > Your firmware also needs to handle that Linux can change the page. If
> > > the firmware changes the page, it must restore it back to whatever
> > > page Linux selected, etc.
> > >
> > > The fact you are submitting this for net suggests you have seen real
> > > issues. Please describe what those issues are.
> >
> > The error log shows:
> >
> > [257681.367827] sfp sfp.1025: Host maximum power 1.0W
> > [257681.370813] txgbe 0000:04:00.1: 31.504 Gb/s available PCIe bandwidth, limited by 8.0 GT/s PCIe x4 link at 0000:02:02.0
(capable
> > of 63.008 Gb/s with 8.0 GT/s PCIe x8 link)
> > [257681.373364] txgbe 0000:04:00.1 enp4s0f1: renamed from eth0
> > [257681.434719] txgbe 0000:04:00.1 enp4s0f1: configuring for inband/10gbase-r link mode
> > [257681.676747] sfp sfp.1025: EEPROM base structure checksum failure: 0x63 != 0x1f
> > [257681.676755] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00  ............g...
> > [257681.676757] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 64  ....FiberStore d
> > [257681.676759] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53      ...!SFP-10GS
> > [257681.676760] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f  R-85    A   .R..
> > [257681.676762] sfp EE: 00000040: 00 81 cd 5b df 25 0a bd 40 f6 c6 ce 47 8e ff ff  ...[.%..@...G...
> > [257681.676763] sfp EE: 00000050: 10 d8 24 33 44 8e ff ff 10 41 b0 9a ff ff ff ff  ..$3D....A......
> >
> > It looks like some fields are read incorrectly. For comparison, I printed the
> >  SFP info when it loaded correctly:
> >
> > [260908.194533] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00  ............g...
> > [260908.194536] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 20  ....FiberStore
> > [260908.194538] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53      ...!SFP-10GS
> > [260908.194540] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f  R-85    A   .R..
> > [260908.194541] sfp EE: 00000040: 40 63 bd df 40 8e ff ff 10 41 b0 9a ff ff ff ff  @c..@....A......
> > [260908.194543] sfp EE: 00000050: 10 58 5b 29 41 8e ff ff 10 41 b0 9a ff ff ff ff  .X[)A....A......
> > [260908.198205] sfp sfp.1025: module FiberStore       SFP-10GSR-85     rev A    sn G1804125607      dc 180605
> >
> > Since the read mechanism of I2C is to write the offset and read command
> > first, and then read the target address. I think it's possible that the different
> > offsets be written at the same time, from Linux and firmware.
> 
> O.K, that is bad. The SFP is totally unreliable...
> 
> You however have still not answered my question. What is the firmware
> accessing? How does it handle pages?
>
> The hack you have put in place is per i2c transaction. But accessing
> pages is likely to be multiple transactions. One to change the page,
> followed by a few reads/writes in the new page, then maybe followed by
> a transactions to return to page 0.

Do you mean the bus address A0 or A2? Firmware accesses I2C just like driver,
but it only change the page once per full transaction, during a possession of
the semaphore.  What you fear seems unlikely to happen.

> I think your best solution is to simply take the mutex and never
> release it. Block your firmware from accessing the SFP.

Firmware accesses the SFP in order to provide information to the BMC.
So it cannot simply be blocked.

Re: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Andrew Lunn]

> > O.K, that is bad. The SFP is totally unreliable...
> > 
> > You however have still not answered my question. What is the firmware
> > accessing? How does it handle pages?
> >
> > The hack you have put in place is per i2c transaction. But accessing
> > pages is likely to be multiple transactions. One to change the page,
> > followed by a few reads/writes in the new page, then maybe followed by
> > a transactions to return to page 0.
> 
> Do you mean the bus address A0 or A2? Firmware accesses I2C just like driver,
> but it only change the page once per full transaction, during a possession of
> the semaphore.  What you fear seems unlikely to happen.

What sort of SFP is this? QSFP byte 127 selects the page for addresses
128-255. Paged 0 and 3 are mandatory, pages 1 and 2 are optional.

SFP+ also uses byte 127 in the same way:

10.3 Optional Page Select Byte [Address A2h, Byte 127]

In order to provide memory space for DWDM and CDR control functions
and for other potential extensions, multiple Pages can be defined for
the upper half of the A2h address space. At startup the value of byte
127 defaults to 00h, which points to the User EEPROM. This ensures
backward compatibility for transceivers that do not implement the
optional Page structure. When a Page value is written to byte 127,
subsequent reads and writes to bytes 128-255 are made to the relevant
Page.

This specification defines functions in Pages 00h-02h. Pages 03-7Fh
are reserved for future use. Writing the value of a non-supported Page
shall not be accepted by the transceiver. The Page Select byte shall
revert to 0 and read / write operations shall be to the unpaged A2h
memory map.

ethtool allows you to access more than page 0.

ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off]
        [hex on|off] [offset N] [length N] [page N] [bank N] [i2c N]

> > I think your best solution is to simply take the mutex and never
> > release it. Block your firmware from accessing the SFP.
> 
> Firmware accesses the SFP in order to provide information to the BMC.
> So it cannot simply be blocked.

Then you have a design problem. And i don't think locking the I2C bus
per transaction is sufficient.

	Andrew

RE: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Jiawen Wu]

On Thu, Aug 29, 2024 11:28 PM, Andrew Lunn wrote:
> > > O.K, that is bad. The SFP is totally unreliable...
> > >
> > > You however have still not answered my question. What is the firmware
> > > accessing? How does it handle pages?
> > >
> > > The hack you have put in place is per i2c transaction. But accessing
> > > pages is likely to be multiple transactions. One to change the page,
> > > followed by a few reads/writes in the new page, then maybe followed by
> > > a transactions to return to page 0.
> >
> > Do you mean the bus address A0 or A2? Firmware accesses I2C just like driver,
> > but it only change the page once per full transaction, during a possession of
> > the semaphore.  What you fear seems unlikely to happen.
> 
> What sort of SFP is this? QSFP byte 127 selects the page for addresses
> 128-255. Paged 0 and 3 are mandatory, pages 1 and 2 are optional.
>
> SFP+ also uses byte 127 in the same way:
> 
> 10.3 Optional Page Select Byte [Address A2h, Byte 127]
> 
> In order to provide memory space for DWDM and CDR control functions
> and for other potential extensions, multiple Pages can be defined for
> the upper half of the A2h address space. At startup the value of byte
> 127 defaults to 00h, which points to the User EEPROM. This ensures
> backward compatibility for transceivers that do not implement the
> optional Page structure. When a Page value is written to byte 127,
> subsequent reads and writes to bytes 128-255 are made to the relevant
> Page.
> 
> This specification defines functions in Pages 00h-02h. Pages 03-7Fh
> are reserved for future use. Writing the value of a non-supported Page
> shall not be accepted by the transceiver. The Page Select byte shall
> revert to 0 and read / write operations shall be to the unpaged A2h
> memory map.
> 
> ethtool allows you to access more than page 0.
> 
> ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off]
>         [hex on|off] [offset N] [length N] [page N] [bank N] [i2c N]
> 
> > > I think your best solution is to simply take the mutex and never
> > > release it. Block your firmware from accessing the SFP.
> >
> > Firmware accesses the SFP in order to provide information to the BMC.
> > So it cannot simply be blocked.
> 
> Then you have a design problem. And i don't think locking the I2C bus
> per transaction is sufficient.

SFP+ is used on our device.

But I don't quite understand why this lock is not sufficient. The entire
transaction is locked, include setting the bus address and selecting pages,
and all subsequent reads and writes on this page. Also, firmware uses this
lock (hardware semaphore) in the same way. Neither driver nor firmware
switches pages whiling the other is reading / writing ?

Re: [PATCH net 0/3] Add I2C bus lock for Wangxun

[Andrew Lunn]

> SFP+ is used on our device.
> 
> But I don't quite understand why this lock is not sufficient. The entire
> transaction is locked, include setting the bus address and selecting pages,
> and all subsequent reads and writes on this page. Also, firmware uses this
> lock (hardware semaphore) in the same way. Neither driver nor firmware
> switches pages whiling the other is reading / writing ?
 
The standard say you cannot read past a 1/2 page boundary. So you do
one i2c transaction to write to byte 127. You then do transactions to
write/read in the range 128-255. And then you do a transaction to
write to byte 127 to set the page back, maybe.

You would need to hold the lock over all these transactions. The i2c
bus driver is too low for this, it would need to be done in the SFP
layer. The firmware would also need to do the same, hold the lock over
all the transactions, not just one.

And i said 'maybe'. It could leave the page select byte on something
other than page 0. Linux is driving the hardware... It might know it
is going to use the same page sometime later. So the firmware will
need to take the lock, read byte 127, change it to whatever it needs,
do its read/writes, and then restore the page value at 127, and then
release the lock..

Also, you have to think about the quirks. Cut/paste from sfp.c:

/* SFP_EEPROM_BLOCK_SIZE is the size of data chunk to read the EEPROM
 * at a time. Some SFP modules and also some Linux I2C drivers do not like
 * reads longer than 16 bytes.
 */
#define SFP_EEPROM_BLOCK_SIZE   16

        /* Some SFP modules (e.g. Nokia 3FE46541AA) lock up if read from
         * address 0x51 is just one byte at a time. Also SFF-8472 requires
         * that EEPROM supports atomic 16bit read operation for diagnostic
         * fields, so do not switch to one byte reading at a time unless it
         * is really required and we have no other option.
         */

/* GPON modules based on Realtek RTL8672 and RTL9601C chips (e.g. V-SOL
 * V2801F, CarlitoxxPro CPGOS03-0490, Ubiquiti U-Fiber Instant, ...) do
 * not support multibyte reads from the EEPROM. Each multi-byte read
 * operation returns just one byte of EEPROM followed by zeros. There is
 * no way to identify which modules are using Realtek RTL8672 and RTL9601C
 * chips. Moreover every OEM of V-SOL V2801F module puts its own vendor
 * name and vendor id into EEPROM, so there is even no way to detect if
 * module is V-SOL V2801F. Therefore check for those zeros in the read
 * data and then based on check switch to reading EEPROM to one byte
 * at a time.
 */

How easy it is to update your firmware each time we add a quirk? There
is no point Linux working around all the broken SFPs if your firmware
then breaks it by not doing word reads when it should, reading 128
bytes not 16, not falling back to byte reads and disabling your
equivalent of HWMON for the sensors.

	Andrew