ixgbe: fix semaphore lock for I2C read/writes on 82598

ixgbe_read/write_i2c_phy_82598() does not hold the SWFW_SYNC
semaphore for the entire function. Instead the lock is held only
during the phy.ops.read/write_reg operations. As result when the
function is being called simultaneously the I2C read/writes can
be corrupted.

The following patch introduces the SWFW_SYNC semaphore for the
entire ixgbe_read/write_i2c_phy_82598() function. To accomplish
this I had to create 2 separate functions:

ixgbe_read_phy_reg_mdi()
ixgbe_write_phy_reg_mdi()

Those functions are identical to ixgbe_read/write_phy_reg_generic()
sans the locking, and can be used in ixgbe_read/write_i2c_phy_82598()
with the SWFW_SYNC semaphore being held.
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

drivers/net/ethernet/intel/ixgbe/ixgbe_82598.c

@@ -1018,8 +1018,17 @@ static s32 ixgbe_read_i2c_phy_82598(struct ixgbe_hw *hw, u8 dev_addr,
 	u16 sfp_addr = 0;
 	u16 sfp_data = 0;
 	u16 sfp_stat = 0;
+	u16 gssr;
 	u32 i;
 
+	if (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1)
+		gssr = IXGBE_GSSR_PHY1_SM;
+	else
+		gssr = IXGBE_GSSR_PHY0_SM;
+
+	if (hw->mac.ops.acquire_swfw_sync(hw, gssr) != 0)
+		return IXGBE_ERR_SWFW_SYNC;
+
 	if (hw->phy.type == ixgbe_phy_nl) {
 		/*
 		 * phy SDA/SCL registers are at addresses 0xC30A to
@@ -1028,14 +1037,14 @@ static s32 ixgbe_read_i2c_phy_82598(struct ixgbe_hw *hw, u8 dev_addr,
 		 */
 		sfp_addr = (dev_addr << 8) + byte_offset;
 		sfp_addr = (sfp_addr | IXGBE_I2C_EEPROM_READ_MASK);
-		hw->phy.ops.write_reg(hw,
+		hw->phy.ops.write_reg_mdi(hw,
 					  IXGBE_MDIO_PMA_PMD_SDA_SCL_ADDR,
 					  MDIO_MMD_PMAPMD,
 					  sfp_addr);
 
 		/* Poll status */
 		for (i = 0; i < 100; i++) {
-			hw->phy.ops.read_reg(hw,
+			hw->phy.ops.read_reg_mdi(hw,
 						IXGBE_MDIO_PMA_PMD_SDA_SCL_STAT,
 						MDIO_MMD_PMAPMD,
 						&sfp_stat);
@@ -1052,7 +1061,7 @@ static s32 ixgbe_read_i2c_phy_82598(struct ixgbe_hw *hw, u8 dev_addr,
 		}
 
 		/* Read data */
-		hw->phy.ops.read_reg(hw, IXGBE_MDIO_PMA_PMD_SDA_SCL_DATA,
+		hw->phy.ops.read_reg_mdi(hw, IXGBE_MDIO_PMA_PMD_SDA_SCL_DATA,
 					MDIO_MMD_PMAPMD, &sfp_data);
 
 		*eeprom_data = (u8)(sfp_data >> 8);
@@ -1061,6 +1070,7 @@ static s32 ixgbe_read_i2c_phy_82598(struct ixgbe_hw *hw, u8 dev_addr,
 	}
 
 out:
+	hw->mac.ops.release_swfw_sync(hw, gssr);
 	return status;
 }
 
@@ -1326,6 +1336,8 @@ static struct ixgbe_phy_operations phy_ops_82598 = {
 	.reset			= &ixgbe_reset_phy_generic,
 	.read_reg		= &ixgbe_read_phy_reg_generic,
 	.write_reg		= &ixgbe_write_phy_reg_generic,
+	.read_reg_mdi		= &ixgbe_read_phy_reg_mdi,
+	.write_reg_mdi		= &ixgbe_write_phy_reg_mdi,
 	.setup_link		= &ixgbe_setup_phy_link_generic,
 	.setup_link_speed	= &ixgbe_setup_phy_link_speed_generic,
 	.read_i2c_sff8472	= &ixgbe_read_i2c_sff8472_82598,

drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c

@@ -204,29 +204,17 @@ s32 ixgbe_reset_phy_generic(struct ixgbe_hw *hw)
 }
 
 /**
- *  ixgbe_read_phy_reg_generic - Reads a value from a specified PHY register
+ *  ixgbe_read_phy_mdi - Reads a value from a specified PHY register without
+ *  the SWFW lock
  *  @hw: pointer to hardware structure
  *  @reg_addr: 32 bit address of PHY register to read
  *  @phy_data: Pointer to read data from PHY register
  **/
-s32 ixgbe_read_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
-                               u32 device_type, u16 *phy_data)
+s32 ixgbe_read_phy_reg_mdi(struct ixgbe_hw *hw, u32 reg_addr, u32 device_type,
+		       u16 *phy_data)
 {
-	u32 command;
-	u32 i;
-	u32 data;
-	s32 status = 0;
-	u16 gssr;
-
-	if (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1)
-		gssr = IXGBE_GSSR_PHY1_SM;
-	else
-		gssr = IXGBE_GSSR_PHY0_SM;
-
-	if (hw->mac.ops.acquire_swfw_sync(hw, gssr) != 0)
-		status = IXGBE_ERR_SWFW_SYNC;
+	u32 i, data, command;
 
-	if (status == 0) {
 	/* Setup and write the address cycle command */
 	command = ((reg_addr << IXGBE_MSCA_NP_ADDR_SHIFT)  |
 		   (device_type << IXGBE_MSCA_DEV_TYPE_SHIFT) |
@@ -235,8 +223,7 @@ s32 ixgbe_read_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
 
 	IXGBE_WRITE_REG(hw, IXGBE_MSCA, command);
 
-		/*
-		 * Check every 10 usec to see if the address cycle completed.
+	/* Check every 10 usec to see if the address cycle completed.
 	 * The MDI Command bit will clear when the operation is
 	 * complete
 	 */
@@ -244,31 +231,27 @@ s32 ixgbe_read_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
 		udelay(10);
 
 		command = IXGBE_READ_REG(hw, IXGBE_MSCA);
-
 		if ((command & IXGBE_MSCA_MDI_COMMAND) == 0)
 				break;
 	}
 
+
 	if ((command & IXGBE_MSCA_MDI_COMMAND) != 0) {
 		hw_dbg(hw, "PHY address command did not complete.\n");
-			status = IXGBE_ERR_PHY;
+		return IXGBE_ERR_PHY;
 	}
 
-		if (status == 0) {
-			/*
-			 * Address cycle complete, setup and write the read
+	/* Address cycle complete, setup and write the read
 	 * command
 	 */
 	command = ((reg_addr << IXGBE_MSCA_NP_ADDR_SHIFT)  |
 		   (device_type << IXGBE_MSCA_DEV_TYPE_SHIFT) |
-			           (hw->phy.mdio.prtad <<
-				    IXGBE_MSCA_PHY_ADDR_SHIFT) |
+		   (hw->phy.mdio.prtad << IXGBE_MSCA_PHY_ADDR_SHIFT) |
 		   (IXGBE_MSCA_READ | IXGBE_MSCA_MDI_COMMAND));
 
 	IXGBE_WRITE_REG(hw, IXGBE_MSCA, command);
 
-			/*
-			 * Check every 10 usec to see if the address cycle
+	/* Check every 10 usec to see if the address cycle
 	 * completed. The MDI Command bit will clear when the
 	 * operation is complete
 	 */
@@ -276,44 +259,36 @@ s32 ixgbe_read_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
 		udelay(10);
 
 		command = IXGBE_READ_REG(hw, IXGBE_MSCA);
-
 		if ((command & IXGBE_MSCA_MDI_COMMAND) == 0)
 			break;
 	}
 
 	if ((command & IXGBE_MSCA_MDI_COMMAND) != 0) {
 		hw_dbg(hw, "PHY read command didn't complete\n");
-				status = IXGBE_ERR_PHY;
-			} else {
-				/*
-				 * Read operation is complete.  Get the data
+		return IXGBE_ERR_PHY;
+	}
+
+	/* Read operation is complete.  Get the data
 	 * from MSRWD
 	 */
 	data = IXGBE_READ_REG(hw, IXGBE_MSRWD);
 	data >>= IXGBE_MSRWD_READ_DATA_SHIFT;
 	*phy_data = (u16)(data);
-			}
-		}
 
-		hw->mac.ops.release_swfw_sync(hw, gssr);
-	}
-
-	return status;
+	return 0;
 }
 
 /**
- *  ixgbe_write_phy_reg_generic - Writes a value to specified PHY register
+ *  ixgbe_read_phy_reg_generic - Reads a value from a specified PHY register
+ *  using the SWFW lock - this function is needed in most cases
  *  @hw: pointer to hardware structure
- *  @reg_addr: 32 bit PHY register to write
- *  @device_type: 5 bit device type
- *  @phy_data: Data to write to the PHY register
+ *  @reg_addr: 32 bit address of PHY register to read
+ *  @phy_data: Pointer to read data from PHY register
  **/
-s32 ixgbe_write_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
-                                u32 device_type, u16 phy_data)
+s32 ixgbe_read_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
+                               u32 device_type, u16 *phy_data)
 {
-	u32 command;
-	u32 i;
-	s32 status = 0;
+	s32 status;
 	u16 gssr;
 
 	if (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1)
@@ -321,10 +296,30 @@ s32 ixgbe_write_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
 	else
 		gssr = IXGBE_GSSR_PHY0_SM;
 
-	if (hw->mac.ops.acquire_swfw_sync(hw, gssr) != 0)
+	if (hw->mac.ops.acquire_swfw_sync(hw, gssr) == 0) {
+		status = ixgbe_read_phy_reg_mdi(hw, reg_addr, device_type,
+						phy_data);
+		hw->mac.ops.release_swfw_sync(hw, gssr);
+	} else {
 		status = IXGBE_ERR_SWFW_SYNC;
+	}
+
+	return status;
+}
+
+/**
+ *  ixgbe_write_phy_reg_mdi - Writes a value to specified PHY register
+ *  without SWFW lock
+ *  @hw: pointer to hardware structure
+ *  @reg_addr: 32 bit PHY register to write
+ *  @device_type: 5 bit device type
+ *  @phy_data: Data to write to the PHY register
+ **/
+s32 ixgbe_write_phy_reg_mdi(struct ixgbe_hw *hw, u32 reg_addr,
+				u32 device_type, u16 phy_data)
+{
+	u32 i, command;
 
-	if (status == 0) {
 	/* Put the data in the MDI single read and write data register*/
 	IXGBE_WRITE_REG(hw, IXGBE_MSRWD, (u32)phy_data);
 
@@ -345,31 +340,27 @@ s32 ixgbe_write_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
 		udelay(10);
 
 		command = IXGBE_READ_REG(hw, IXGBE_MSCA);
-
 		if ((command & IXGBE_MSCA_MDI_COMMAND) == 0)
 			break;
 	}
 
 	if ((command & IXGBE_MSCA_MDI_COMMAND) != 0) {
 		hw_dbg(hw, "PHY address cmd didn't complete\n");
-			status = IXGBE_ERR_PHY;
+		return IXGBE_ERR_PHY;
 	}
 
-		if (status == 0) {
 	/*
 	 * Address cycle complete, setup and write the write
 	 * command
 	 */
 	command = ((reg_addr << IXGBE_MSCA_NP_ADDR_SHIFT)  |
 		   (device_type << IXGBE_MSCA_DEV_TYPE_SHIFT) |
-			           (hw->phy.mdio.prtad <<
-				    IXGBE_MSCA_PHY_ADDR_SHIFT) |
+		   (hw->phy.mdio.prtad << IXGBE_MSCA_PHY_ADDR_SHIFT) |
 		   (IXGBE_MSCA_WRITE | IXGBE_MSCA_MDI_COMMAND));
 
 	IXGBE_WRITE_REG(hw, IXGBE_MSCA, command);
 
-			/*
-			 * Check every 10 usec to see if the address cycle
+	/* Check every 10 usec to see if the address cycle
 	 * completed. The MDI Command bit will clear when the
 	 * operation is complete
 	 */
@@ -377,18 +368,43 @@ s32 ixgbe_write_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
 		udelay(10);
 
 		command = IXGBE_READ_REG(hw, IXGBE_MSCA);
-
 		if ((command & IXGBE_MSCA_MDI_COMMAND) == 0)
 			break;
 	}
 
 	if ((command & IXGBE_MSCA_MDI_COMMAND) != 0) {
-				hw_dbg(hw, "PHY address cmd didn't complete\n");
-				status = IXGBE_ERR_PHY;
-			}
+		hw_dbg(hw, "PHY write cmd didn't complete\n");
+		return IXGBE_ERR_PHY;
 	}
 
+	return 0;
+}
+
+/**
+ *  ixgbe_write_phy_reg_generic - Writes a value to specified PHY register
+ *  using SWFW lock- this function is needed in most cases
+ *  @hw: pointer to hardware structure
+ *  @reg_addr: 32 bit PHY register to write
+ *  @device_type: 5 bit device type
+ *  @phy_data: Data to write to the PHY register
+ **/
+s32 ixgbe_write_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
+                                u32 device_type, u16 phy_data)
+{
+	s32 status;
+	u16 gssr;
+
+	if (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1)
+		gssr = IXGBE_GSSR_PHY1_SM;
+	else
+		gssr = IXGBE_GSSR_PHY0_SM;
+
+	if (hw->mac.ops.acquire_swfw_sync(hw, gssr) == 0) {
+		status = ixgbe_write_phy_reg_mdi(hw, reg_addr, device_type,
+						 phy_data);
 		hw->mac.ops.release_swfw_sync(hw, gssr);
+	} else {
+		status = IXGBE_ERR_SWFW_SYNC;
 	}
 
 	return status;

drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h

@@ -107,6 +107,10 @@ s32 ixgbe_read_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
                                u32 device_type, u16 *phy_data);
 s32 ixgbe_write_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
                                 u32 device_type, u16 phy_data);
+s32 ixgbe_read_phy_reg_mdi(struct ixgbe_hw *hw, u32 reg_addr,
+			   u32 device_type, u16 *phy_data);
+s32 ixgbe_write_phy_reg_mdi(struct ixgbe_hw *hw, u32 reg_addr,
+			    u32 device_type, u16 phy_data);
 s32 ixgbe_setup_phy_link_generic(struct ixgbe_hw *hw);
 s32 ixgbe_setup_phy_link_speed_generic(struct ixgbe_hw *hw,
                                        ixgbe_link_speed speed,

drivers/net/ethernet/intel/ixgbe/ixgbe_type.h

@@ -2886,6 +2886,8 @@ struct ixgbe_phy_operations {
 	s32 (*reset)(struct ixgbe_hw *);
 	s32 (*read_reg)(struct ixgbe_hw *, u32, u32, u16 *);
 	s32 (*write_reg)(struct ixgbe_hw *, u32, u32, u16);
+	s32 (*read_reg_mdi)(struct ixgbe_hw *, u32, u32, u16 *);
+	s32 (*write_reg_mdi)(struct ixgbe_hw *, u32, u32, u16);
 	s32 (*setup_link)(struct ixgbe_hw *);
 	s32 (*setup_link_speed)(struct ixgbe_hw *, ixgbe_link_speed, bool);
 	s32 (*check_link)(struct ixgbe_hw *, ixgbe_link_speed *, bool *);