[PATCH net 0/5] net: aquantia: minor bug fixes after static analysis

[PATCH net 0/5] net: aquantia: minor bug fixes after static analysis

[Igor Russkikh]

This patchset fixes minor errors and warnings found by smatch and kasan.

Extra patch is to fix AQ_HW_WAIT_FOR macro to make it visible
it changes err variable.

Igor Russkikh (1):
  net: aquantia: fixed instack structure overflow

Nikita Danilov (4):
  net: aquantia: fixed memcpy size
  net: aquantia: added newline at end of file
  net: aquantia: fixed buffer overflow
  net: aquantia: added err var into AQ_HW_WAIT_FOR construct

 .../ethernet/aquantia/atlantic/aq_ethtool.c    |  2 +-
 .../ethernet/aquantia/atlantic/aq_hw_utils.h   |  4 ++--
 .../net/ethernet/aquantia/atlantic/aq_nic.c    |  2 +-
 .../ethernet/aquantia/atlantic/aq_pci_func.c   |  2 ++
 .../aquantia/atlantic/hw_atl/hw_atl_a0.c       |  8 ++++----
 .../aquantia/atlantic/hw_atl/hw_atl_b0.c       |  8 ++++----
 .../aquantia/atlantic/hw_atl/hw_atl_utils.c    | 18 +++++++++---------
 .../atlantic/hw_atl/hw_atl_utils_fw2x.c        | 10 +++++-----
 8 files changed, 28 insertions(+), 26 deletions(-)

-- 
2.17.1

[PATCH net 1/5] net: aquantia: fixed memcpy size

[Igor Russkikh]

From: Nikita Danilov <nikita.danilov@aquantia.com>

Not careful array dereference caused analysis tools
to think there could be memory overflow.

There was actually no corruption because the array is
two dimensional.

drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c: 140
  aq_ethtool_get_strings() error:
    memcpy() '*aq_ethtool_stat_names' too small (32 vs 704)

Signed-off-by: Nikita Danilov <nikita.danilov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
index 38e87eed76b9..a718d7a1f76c 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ethtool.c
@@ -138,7 +138,7 @@ static void aq_ethtool_get_strings(struct net_device *ndev,
 	u8 *p = data;
 
 	if (stringset == ETH_SS_STATS) {
-		memcpy(p, *aq_ethtool_stat_names,
+		memcpy(p, aq_ethtool_stat_names,
 		       sizeof(aq_ethtool_stat_names));
 		p = p + sizeof(aq_ethtool_stat_names);
 		for (i = 0; i < cfg->vecs; i++) {
-- 
2.17.1

[PATCH net 2/5] net: aquantia: added newline at end of file

[Igor Russkikh]

From: Nikita Danilov <nikita.danilov@aquantia.com>

drivers/net/ethernet/aquantia/atlantic/aq_nic.c: 991:1:
  warning: no newline at end of file

Signed-off-by: Nikita Danilov <nikita.danilov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index 0147c037ca96..ff83667410bd 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -986,4 +986,4 @@ void aq_nic_shutdown(struct aq_nic_s *self)
 
 err_exit:
 	rtnl_unlock();
-}
\ No newline at end of file
+}
-- 
2.17.1

[PATCH net 3/5] net: aquantia: fixed buffer overflow

[Igor Russkikh]

From: Nikita Danilov <nikita.danilov@aquantia.com>

The overflow is detected by smatch:

drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c: 175
  aq_pci_func_free_irqs() error: buffer overflow 'self->aq_vec' 8 <= 31

In reality msix_entry_mask always restricts number of iterations,
adding extra condition to make logic clear and smatch happy.

Signed-off-by: Nikita Danilov <nikita.danilov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c b/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
index c8b44cdb91c1..0217ff4669a4 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c
@@ -170,6 +170,8 @@ void aq_pci_func_free_irqs(struct aq_nic_s *self)
 	for (i = 32U; i--;) {
 		if (!((1U << i) & self->msix_entry_mask))
 			continue;
+		if (i >= AQ_CFG_VECS_MAX)
+			continue;
 
 		if (pdev->msix_enabled)
 			irq_set_affinity_hint(pci_irq_vector(pdev, i), NULL);
-- 
2.17.1

[PATCH net 4/5] net: aquantia: fixed instack structure overflow

[Igor Russkikh]

This is a real stack undercorruption found by kasan build.

The issue did no harm normally because it only overflowed
2 bytes after `bitary` array which on most architectures
were mapped into `err` local.

Signed-off-by: Nikita Danilov <nikita.danilov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index b58ca7cb8e9d..c4cdc51350b2 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -199,8 +199,8 @@ static int hw_atl_b0_hw_rss_set(struct aq_hw_s *self,
 	u32 i = 0U;
 	u32 num_rss_queues = max(1U, self->aq_nic_cfg->num_rss_queues);
 	int err = 0;
-	u16 bitary[(HW_ATL_B0_RSS_REDIRECTION_MAX *
-					HW_ATL_B0_RSS_REDIRECTION_BITS / 16U)];
+	u16 bitary[1 + (HW_ATL_B0_RSS_REDIRECTION_MAX *
+		   HW_ATL_B0_RSS_REDIRECTION_BITS / 16U)];
 
 	memset(bitary, 0, sizeof(bitary));
 
-- 
2.17.1

[PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct

[Igor Russkikh]

From: Nikita Danilov <nikita.danilov@aquantia.com>

David noticed this define was hiding 'err' variable
reference. Thats confusing and counterintuitive.

Adding err argument explicitly for better visibility
that err is changed inside macro.

Signed-off-by: Nikita Danilov <nikita.danilov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 .../ethernet/aquantia/atlantic/aq_hw_utils.h   |  4 ++--
 .../aquantia/atlantic/hw_atl/hw_atl_a0.c       |  8 ++++----
 .../aquantia/atlantic/hw_atl/hw_atl_b0.c       |  4 ++--
 .../aquantia/atlantic/hw_atl/hw_atl_utils.c    | 18 +++++++++---------
 .../atlantic/hw_atl/hw_atl_utils_fw2x.c        | 10 +++++-----
 5 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.h b/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.h
index dc88a1221f1d..ca1d20d64a39 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.h
@@ -23,7 +23,7 @@
 
 #define AQ_HW_SLEEP(_US_) mdelay(_US_)
 
-#define AQ_HW_WAIT_FOR(_B_, _US_, _N_) \
+#define AQ_HW_WAIT_FOR(_B_, _US_, _N_, _err_) \
 do { \
 	unsigned int AQ_HW_WAIT_FOR_i; \
 	for (AQ_HW_WAIT_FOR_i = _N_; (!(_B_)) && (AQ_HW_WAIT_FOR_i);\
@@ -31,7 +31,7 @@ do { \
 		udelay(_US_); \
 	} \
 	if (!AQ_HW_WAIT_FOR_i) {\
-		err = -ETIME; \
+		*(_err_) = -ETIME; \
 	} \
 } while (0)
 
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
index 2469ed4d86b9..1ccfc16b320e 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_a0.c
@@ -95,7 +95,7 @@ static int hw_atl_a0_hw_reset(struct aq_hw_s *self)
 	hw_atl_glb_soft_res_set(self, 1);
 
 	/* check 10 times by 1ms */
-	AQ_HW_WAIT_FOR(hw_atl_glb_soft_res_get(self) == 0, 1000U, 10U);
+	AQ_HW_WAIT_FOR(hw_atl_glb_soft_res_get(self) == 0, 1000U, 10U, &err);
 	if (err < 0)
 		goto err_exit;
 
@@ -103,7 +103,7 @@ static int hw_atl_a0_hw_reset(struct aq_hw_s *self)
 	hw_atl_itr_res_irq_set(self, 1U);
 
 	/* check 10 times by 1ms */
-	AQ_HW_WAIT_FOR(hw_atl_itr_res_irq_get(self) == 0, 1000U, 10U);
+	AQ_HW_WAIT_FOR(hw_atl_itr_res_irq_get(self) == 0, 1000U, 10U, &err);
 	if (err < 0)
 		goto err_exit;
 
@@ -189,7 +189,7 @@ static int hw_atl_a0_hw_rss_hash_set(struct aq_hw_s *self,
 		hw_atl_rpf_rss_key_addr_set(self, addr);
 		hw_atl_rpf_rss_key_wr_en_set(self, 1U);
 		AQ_HW_WAIT_FOR(hw_atl_rpf_rss_key_wr_en_get(self) == 0,
-			       1000U, 10U);
+			       1000U, 10U, &err);
 		if (err < 0)
 			goto err_exit;
 	}
@@ -223,7 +223,7 @@ static int hw_atl_a0_hw_rss_set(struct aq_hw_s *self,
 		hw_atl_rpf_rss_redir_tbl_addr_set(self, i);
 		hw_atl_rpf_rss_redir_wr_en_set(self, 1U);
 		AQ_HW_WAIT_FOR(hw_atl_rpf_rss_redir_wr_en_get(self) == 0,
-			       1000U, 10U);
+			       1000U, 10U, &err);
 		if (err < 0)
 			goto err_exit;
 	}
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index c4cdc51350b2..0b06e543cbda 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -181,7 +181,7 @@ static int hw_atl_b0_hw_rss_hash_set(struct aq_hw_s *self,
 		hw_atl_rpf_rss_key_addr_set(self, addr);
 		hw_atl_rpf_rss_key_wr_en_set(self, 1U);
 		AQ_HW_WAIT_FOR(hw_atl_rpf_rss_key_wr_en_get(self) == 0,
-			       1000U, 10U);
+			       1000U, 10U, &err);
 		if (err < 0)
 			goto err_exit;
 	}
@@ -215,7 +215,7 @@ static int hw_atl_b0_hw_rss_set(struct aq_hw_s *self,
 		hw_atl_rpf_rss_redir_tbl_addr_set(self, i);
 		hw_atl_rpf_rss_redir_wr_en_set(self, 1U);
 		AQ_HW_WAIT_FOR(hw_atl_rpf_rss_redir_wr_en_get(self) == 0,
-			       1000U, 10U);
+			       1000U, 10U, &err);
 		if (err < 0)
 			goto err_exit;
 	}
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
index 9b74a3197d7f..892ded523f7c 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.c
@@ -262,7 +262,7 @@ int hw_atl_utils_soft_reset(struct aq_hw_s *self)
 		hw_atl_utils_mpi_set_state(self, MPI_DEINIT);
 		AQ_HW_WAIT_FOR((aq_hw_read_reg(self, HW_ATL_MPI_STATE_ADR) &
 				HW_ATL_MPI_STATE_MSK) == MPI_DEINIT,
-			       10, 1000U);
+			       10, 1000U, &err);
 		if (err)
 			return err;
 	}
@@ -280,7 +280,7 @@ int hw_atl_utils_fw_downld_dwords(struct aq_hw_s *self, u32 a,
 
 	AQ_HW_WAIT_FOR(hw_atl_reg_glb_cpu_sem_get(self,
 						  HW_ATL_FW_SM_RAM) == 1U,
-		       1U, 10000U);
+		       1U, 10000U, &err);
 
 	if (err < 0) {
 		bool is_locked;
@@ -301,11 +301,11 @@ int hw_atl_utils_fw_downld_dwords(struct aq_hw_s *self, u32 a,
 		if (IS_CHIP_FEATURE(REVISION_B1))
 			AQ_HW_WAIT_FOR(a != aq_hw_read_reg(self,
 							   HW_ATL_MIF_ADDR),
-				       1, 1000U);
+				       1, 1000U, &err);
 		else
 			AQ_HW_WAIT_FOR(!(0x100 & aq_hw_read_reg(self,
 							   HW_ATL_MIF_CMD)),
-				       1, 1000U);
+				       1, 1000U, &err);
 
 		*(p++) = aq_hw_read_reg(self, HW_ATL_MIF_VAL);
 		a += 4;
@@ -340,7 +340,7 @@ static int hw_atl_utils_fw_upload_dwords(struct aq_hw_s *self, u32 a, u32 *p,
 			AQ_HW_WAIT_FOR((aq_hw_read_reg(self,
 						       0x32C) & 0xF0000000) !=
 				       0x80000000,
-				       10, 1000);
+				       10, 1000, &err);
 		}
 	} else {
 		u32 offset = 0;
@@ -352,7 +352,7 @@ static int hw_atl_utils_fw_upload_dwords(struct aq_hw_s *self, u32 a, u32 *p,
 			aq_hw_write_reg(self, 0x200, 0xC000);
 
 			AQ_HW_WAIT_FOR((aq_hw_read_reg(self, 0x200U) &
-					0x100) == 0, 10, 1000);
+					0x100) == 0, 10, 1000, &err);
 		}
 	}
 
@@ -396,7 +396,7 @@ static int hw_atl_utils_init_ucp(struct aq_hw_s *self,
 
 	/* check 10 times by 1ms */
 	AQ_HW_WAIT_FOR(0U != (self->mbox_addr =
-			      aq_hw_read_reg(self, 0x360U)), 1000U, 10U);
+			      aq_hw_read_reg(self, 0x360U)), 1000U, 10U, &err);
 
 	return err;
 }
@@ -455,7 +455,7 @@ int hw_atl_utils_fw_rpc_wait(struct aq_hw_s *self,
 		AQ_HW_WAIT_FOR(sw.tid ==
 			       (fw.val =
 				aq_hw_read_reg(self, HW_ATL_RPC_STATE_ADR),
-				fw.tid), 1000U, 100U);
+				fw.tid), 1000U, 100U, &err);
 
 		if (fw.len == 0xFFFFU) {
 			err = hw_atl_utils_fw_rpc_call(self, sw.len);
@@ -562,7 +562,7 @@ static int hw_atl_utils_mpi_set_state(struct aq_hw_s *self,
 		AQ_HW_WAIT_FOR(transaction_id !=
 			       (hw_atl_utils_mpi_read_mbox(self, &mbox),
 				mbox.transaction_id),
-			       1000U, 100U);
+			       1000U, 100U, &err);
 		if (err < 0)
 			goto err_exit;
 	}
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
index 7de3220d9cab..c6500bf549db 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils_fw2x.c
@@ -79,10 +79,10 @@ static int aq_fw2x_init(struct aq_hw_s *self)
 	/* check 10 times by 1ms */
 	AQ_HW_WAIT_FOR(0U != (self->mbox_addr =
 		       aq_hw_read_reg(self, HW_ATL_FW2X_MPI_MBOX_ADDR)),
-		       1000U, 10U);
+		       1000U, 10U, &err);
 	AQ_HW_WAIT_FOR(0U != (self->rpc_addr =
 		       aq_hw_read_reg(self, HW_ATL_FW2X_MPI_RPC_ADDR)),
-		       1000U, 100U);
+		       1000U, 100U, &err);
 
 	return err;
 }
@@ -295,7 +295,7 @@ static int aq_fw2x_update_stats(struct aq_hw_s *self)
 	AQ_HW_WAIT_FOR(orig_stats_val !=
 		       (aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR) &
 			BIT(CAPS_HI_STATISTICS)),
-		       1U, 10000U);
+		       1U, 10000U, &err);
 	if (err)
 		return err;
 
@@ -338,7 +338,7 @@ static int aq_fw2x_set_sleep_proxy(struct aq_hw_s *self, u8 *mac)
 	aq_hw_write_reg(self, HW_ATL_FW2X_MPI_CONTROL2_ADDR, mpi_opts);
 
 	AQ_HW_WAIT_FOR((aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR) &
-			HW_ATL_FW2X_CTRL_SLEEP_PROXY), 1U, 10000U);
+			HW_ATL_FW2X_CTRL_SLEEP_PROXY), 1U, 10000U, &err);
 
 err_exit:
 	return err;
@@ -375,7 +375,7 @@ static int aq_fw2x_set_wol_params(struct aq_hw_s *self, u8 *mac)
 	aq_hw_write_reg(self, HW_ATL_FW2X_MPI_CONTROL2_ADDR, mpi_opts);
 
 	AQ_HW_WAIT_FOR((aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR) &
-			HW_ATL_FW2X_CTRL_WOL), 1U, 10000U);
+			HW_ATL_FW2X_CTRL_WOL), 1U, 10000U, &err);
 
 err_exit:
 	return err;
-- 
2.17.1

Re: [PATCH net 4/5] net: aquantia: fixed instack structure overflow

[Andrew Lunn]

On Mon, Jan 21, 2019 at 02:53:51PM +0000, Igor Russkikh wrote:
> This is a real stack undercorruption found by kasan build.
> 
> The issue did no harm normally because it only overflowed
> 2 bytes after `bitary` array which on most architectures
> were mapped into `err` local.

Hi Igor

Since this is a real bug, please add a fixes: tag, and post it to net.

      Thanks
	Andrew

Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct

[Andrew Lunn]

On Mon, Jan 21, 2019 at 02:53:53PM +0000, Igor Russkikh wrote:
> From: Nikita Danilov <nikita.danilov@aquantia.com>
> 
> David noticed this define was hiding 'err' variable
> reference. Thats confusing and counterintuitive.
> 
> Adding err argument explicitly for better visibility
> that err is changed inside macro.
> 
> Signed-off-by: Nikita Danilov <nikita.danilov@aquantia.com>
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> ---
>  .../ethernet/aquantia/atlantic/aq_hw_utils.h   |  4 ++--
>  .../aquantia/atlantic/hw_atl/hw_atl_a0.c       |  8 ++++----
>  .../aquantia/atlantic/hw_atl/hw_atl_b0.c       |  4 ++--
>  .../aquantia/atlantic/hw_atl/hw_atl_utils.c    | 18 +++++++++---------
>  .../atlantic/hw_atl/hw_atl_utils_fw2x.c        | 10 +++++-----
>  5 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.h b/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.h
> index dc88a1221f1d..ca1d20d64a39 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.h
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_hw_utils.h
> @@ -23,7 +23,7 @@
>  
>  #define AQ_HW_SLEEP(_US_) mdelay(_US_)
>  
> -#define AQ_HW_WAIT_FOR(_B_, _US_, _N_) \
> +#define AQ_HW_WAIT_FOR(_B_, _US_, _N_, _err_) \
>  do { \
>  	unsigned int AQ_HW_WAIT_FOR_i; \
>  	for (AQ_HW_WAIT_FOR_i = _N_; (!(_B_)) && (AQ_HW_WAIT_FOR_i);\
> @@ -31,7 +31,7 @@ do { \
>  		udelay(_US_); \
>  	} \
>  	if (!AQ_HW_WAIT_FOR_i) {\
> -		err = -ETIME; \
> +		*(_err_) = -ETIME; \
>  	} \
>  } while (0)

Hi Igor

How about throwing this horrible macro away and using one of the
readx_poll_timeout() variants.

	     Andrew

Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct

[Igor Russkikh]

>> -#define AQ_HW_WAIT_FOR(_B_, _US_, _N_) \
>> +#define AQ_HW_WAIT_FOR(_B_, _US_, _N_, _err_) \
>>  do { \
>>  	unsigned int AQ_HW_WAIT_FOR_i; \
>>  	for (AQ_HW_WAIT_FOR_i = _N_; (!(_B_)) && (AQ_HW_WAIT_FOR_i);\
>> @@ -31,7 +31,7 @@ do { \
>>  		udelay(_US_); \
>>  	} \
>>  	if (!AQ_HW_WAIT_FOR_i) {\
>> -		err = -ETIME; \
>> +		*(_err_) = -ETIME; \
>>  	} \
>>  } while (0)
> 
> Hi Igor
> 
> How about throwing this horrible macro away and using one of the
> readx_poll_timeout() variants.

Hi Andrew,

Thanks I was not aware of that macro.

The original macro actually differs a bit in a sense that it does not require
a 'value' local variable to be used. Also aq driver used a number of wrapper
functions to hide the actual readl and mmio access.

Thus its hard to adopt readx_poll_timeout() in its current form.

WAIT_FOR is normally used like

	AQ_HW_WAIT_FOR(hw_atl_itr_res_irq_get(self) == 0, 1000U, 10U, &err);

So there is no direct access to register offset and value. Thats an expression waiting for
logical condition to happen, not just register value to change.

I was under impression statement expressions (macro returning values) should not be used because
of compatibility, but since its in use may be its the better to rewrite this as

#define AQ_HW_WAIT_FOR(_B_, _US_, _N_) \
({ \
	unsigned int AQ_HW_WAIT_FOR_i; \
	for (AQ_HW_WAIT_FOR_i = _N_; (!(_B_)) && (AQ_HW_WAIT_FOR_i);\
	--AQ_HW_WAIT_FOR_i) {\
		udelay(_US_); \
	} \
        AQ_HW_WAIT_FOR_i ? EOK : -ETIMEDOUT;\
})

So it could be used as

	err = AQ_HW_WAIT_FOR(hw_atl_itr_res_irq_get(self) == 0, 1000U, 10U);

and may be worth renaming it to something with _poll_timeout suffix as well?

Regards, 
  Igor

Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct

[Andrew Lunn]

On Tue, Jan 22, 2019 at 12:44:07PM +0000, Igor Russkikh wrote:
> 
> >> -#define AQ_HW_WAIT_FOR(_B_, _US_, _N_) \
> >> +#define AQ_HW_WAIT_FOR(_B_, _US_, _N_, _err_) \
> >>  do { \
> >>  	unsigned int AQ_HW_WAIT_FOR_i; \
> >>  	for (AQ_HW_WAIT_FOR_i = _N_; (!(_B_)) && (AQ_HW_WAIT_FOR_i);\
> >> @@ -31,7 +31,7 @@ do { \
> >>  		udelay(_US_); \
> >>  	} \
> >>  	if (!AQ_HW_WAIT_FOR_i) {\
> >> -		err = -ETIME; \
> >> +		*(_err_) = -ETIME; \
> >>  	} \
> >>  } while (0)
> > 
> > Hi Igor
> > 
> > How about throwing this horrible macro away and using one of the
> > readx_poll_timeout() variants.
> 
> Hi Andrew,
> 
> Thanks I was not aware of that macro.
> 
> The original macro actually differs a bit in a sense that it does not require
> a 'value' local variable to be used. Also aq driver used a number of wrapper
> functions to hide the actual readl and mmio access.
> 
> Thus its hard to adopt readx_poll_timeout() in its current form.
> 
> WAIT_FOR is normally used like
> 
> 	AQ_HW_WAIT_FOR(hw_atl_itr_res_irq_get(self) == 0, 1000U, 10U, &err);

Hi Igor

	 err = readx_poll_timeout(hw_atl_itr_res_irq_get, self, alt_itr_res,
	       		          alt_itr_res == 0, 10, 1000);

The advantage of using readx_poll_timeout is that it is used by lots
of other drivers and works. It is much better to use core
infrastructure, then build your own.

   Andrew

Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct

[Igor Russkikh]

> 
> Hi Igor
> 
> 	 err = readx_poll_timeout(hw_atl_itr_res_irq_get, self, alt_itr_res,
> 	       		          alt_itr_res == 0, 10, 1000);
> 
> The advantage of using readx_poll_timeout is that it is used by lots
> of other drivers and works. It is much better to use core
> infrastructure, then build your own.

Hi Andrew, agreed, but driver have more incompatible places with constructs like

	AQ_HW_WAIT_FOR(orig_stats_val !=
		       (aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR) &
				       BIT(CAPS_HI_STATISTICS)),
		       1U, 10000U);

For this only the following hack comes to my mind:

	err = readx_poll_timeout_atomic(, val, val, orig_stats_val !=
		             		(aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR) &
			        	BIT(CAPS_HI_STATISTICS)), 1, 10000);

That way it may be better to do the following declaration then:

	#define do_poll_timeout_atomic(cond, sleep_us, timeout_us) \
	({ \
		int _val; /* to make macro happy */
		readx_poll_timeout_atomic(, _val, _val, cond, sleep_us, timeout_us);
	})

Another way would be just to remove AQ_HW_WAIT_FOR macro for all the hard cases
and rewrite it with explicit `for` loop. But that'll reduce readability I guess.

PS

Found duplicating readl_poll_timeout declaration here:
https://elixir.bootlin.com/linux/latest/source/drivers/phy/qualcomm/phy-qcom-ufs-i.h#L27
Not sure what's the reason, but may worth cleaning it up.

Regards,
  Igor

Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct

[Andrew Lunn]

On Wed, Jan 23, 2019 at 09:49:25AM +0000, Igor Russkikh wrote:
> 
> > 
> > Hi Igor
> > 
> > 	 err = readx_poll_timeout(hw_atl_itr_res_irq_get, self, alt_itr_res,
> > 	       		          alt_itr_res == 0, 10, 1000);
> > 
> > The advantage of using readx_poll_timeout is that it is used by lots
> > of other drivers and works. It is much better to use core
> > infrastructure, then build your own.
> 
> Hi Andrew, agreed, but driver have more incompatible places with constructs like
> 
> 	AQ_HW_WAIT_FOR(orig_stats_val !=
> 		       (aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR) &
> 				       BIT(CAPS_HI_STATISTICS)),
> 		       1U, 10000U);

You can define a little helper:

static u32 aq_hw_read_mpi_state2_addr(struct aq_hw_s *self)
{
	return aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR);
}

Then

	readx_poll_timeout(u32 aq_hw_read_mpi_state2_addr, self,
			   stats_val, orig_stats_val != stats_val & BIT(CAPS_HI_STATISTICS));

Given you have the comment:

        /* Wait FW to report back */

You think the current code is not very readable. So you could actually
have:

static int sq_fw2_update_stats_fw_wait(aq_hw_s *self, u32 orig_stats_val)
{
	u32 stats_val;

	return readx_poll_timeout(u32 aq_hw_read_mpi_state2_addr, self,
			           stats_val,
				   orig_stats_val != stats_val & BIT(CAPS_HI_STATISTICS),
				   1, 1000);
}

You see this sort of construct in quite a lot of drivers.

> Found duplicating readl_poll_timeout declaration here:
> https://elixir.bootlin.com/linux/latest/source/drivers/phy/qualcomm/phy-qcom-ufs-i.h#L27
> Not sure what's the reason, but may worth cleaning it up.

Thanks.

	Andrew

Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct

[Igor Russkikh]

> You can define a little helper:
> 
> static u32 aq_hw_read_mpi_state2_addr(struct aq_hw_s *self)
> {
> 	return aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR);
> }
> 
> Then
> 
> 	readx_poll_timeout(u32 aq_hw_read_mpi_state2_addr, self,
> 			   stats_val, orig_stats_val != stats_val & BIT(CAPS_HI_STATISTICS));
> 

[cut]

> You see this sort of construct in quite a lot of drivers.


Thanks, Andrew,

Think that way this'll be the optimal case with best readability.

We'll update the patch.

Regards, Igor