* [PATCH net 0/5] net: aquantia: minor bug fixes after static analysis
@ 2019-01-21 14:53 Igor Russkikh
2019-01-21 14:53 ` [PATCH net 1/5] net: aquantia: fixed memcpy size Igor Russkikh
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Igor Russkikh @ 2019-01-21 14:53 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev@vger.kernel.org, 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
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH net 1/5] net: aquantia: fixed memcpy size 2019-01-21 14:53 [PATCH net 0/5] net: aquantia: minor bug fixes after static analysis Igor Russkikh @ 2019-01-21 14:53 ` Igor Russkikh 2019-01-21 14:53 ` [PATCH net 2/5] net: aquantia: added newline at end of file Igor Russkikh ` (3 subsequent siblings) 4 siblings, 0 replies; 13+ messages in thread From: Igor Russkikh @ 2019-01-21 14:53 UTC (permalink / raw) To: David S . Miller; +Cc: netdev@vger.kernel.org, Igor Russkikh, Nikita Danilov 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net 2/5] net: aquantia: added newline at end of file 2019-01-21 14:53 [PATCH net 0/5] net: aquantia: minor bug fixes after static analysis Igor Russkikh 2019-01-21 14:53 ` [PATCH net 1/5] net: aquantia: fixed memcpy size Igor Russkikh @ 2019-01-21 14:53 ` Igor Russkikh 2019-01-21 14:53 ` [PATCH net 3/5] net: aquantia: fixed buffer overflow Igor Russkikh ` (2 subsequent siblings) 4 siblings, 0 replies; 13+ messages in thread From: Igor Russkikh @ 2019-01-21 14:53 UTC (permalink / raw) To: David S . Miller; +Cc: netdev@vger.kernel.org, Igor Russkikh, Nikita Danilov 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net 3/5] net: aquantia: fixed buffer overflow 2019-01-21 14:53 [PATCH net 0/5] net: aquantia: minor bug fixes after static analysis Igor Russkikh 2019-01-21 14:53 ` [PATCH net 1/5] net: aquantia: fixed memcpy size Igor Russkikh 2019-01-21 14:53 ` [PATCH net 2/5] net: aquantia: added newline at end of file Igor Russkikh @ 2019-01-21 14:53 ` Igor Russkikh 2019-01-21 14:53 ` [PATCH net 4/5] net: aquantia: fixed instack structure overflow Igor Russkikh 2019-01-21 14:53 ` [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct Igor Russkikh 4 siblings, 0 replies; 13+ messages in thread From: Igor Russkikh @ 2019-01-21 14:53 UTC (permalink / raw) To: David S . Miller; +Cc: netdev@vger.kernel.org, Igor Russkikh, Nikita Danilov 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net 4/5] net: aquantia: fixed instack structure overflow 2019-01-21 14:53 [PATCH net 0/5] net: aquantia: minor bug fixes after static analysis Igor Russkikh ` (2 preceding siblings ...) 2019-01-21 14:53 ` [PATCH net 3/5] net: aquantia: fixed buffer overflow Igor Russkikh @ 2019-01-21 14:53 ` Igor Russkikh 2019-01-21 17:10 ` Andrew Lunn 2019-01-21 14:53 ` [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct Igor Russkikh 4 siblings, 1 reply; 13+ messages in thread From: Igor Russkikh @ 2019-01-21 14:53 UTC (permalink / raw) To: David S . Miller; +Cc: netdev@vger.kernel.org, Igor Russkikh, Nikita Danilov 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net 4/5] net: aquantia: fixed instack structure overflow 2019-01-21 14:53 ` [PATCH net 4/5] net: aquantia: fixed instack structure overflow Igor Russkikh @ 2019-01-21 17:10 ` Andrew Lunn 0 siblings, 0 replies; 13+ messages in thread From: Andrew Lunn @ 2019-01-21 17:10 UTC (permalink / raw) To: Igor Russkikh; +Cc: David S . Miller, netdev@vger.kernel.org, Nikita Danilov 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct 2019-01-21 14:53 [PATCH net 0/5] net: aquantia: minor bug fixes after static analysis Igor Russkikh ` (3 preceding siblings ...) 2019-01-21 14:53 ` [PATCH net 4/5] net: aquantia: fixed instack structure overflow Igor Russkikh @ 2019-01-21 14:53 ` Igor Russkikh 2019-01-21 17:13 ` Andrew Lunn 4 siblings, 1 reply; 13+ messages in thread From: Igor Russkikh @ 2019-01-21 14:53 UTC (permalink / raw) To: David S . Miller; +Cc: netdev@vger.kernel.org, Igor Russkikh, Nikita Danilov 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct 2019-01-21 14:53 ` [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct Igor Russkikh @ 2019-01-21 17:13 ` Andrew Lunn 2019-01-22 12:44 ` Igor Russkikh 0 siblings, 1 reply; 13+ messages in thread From: Andrew Lunn @ 2019-01-21 17:13 UTC (permalink / raw) To: Igor Russkikh; +Cc: David S . Miller, netdev@vger.kernel.org, Nikita Danilov 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct 2019-01-21 17:13 ` Andrew Lunn @ 2019-01-22 12:44 ` Igor Russkikh 2019-01-22 13:34 ` Andrew Lunn 0 siblings, 1 reply; 13+ messages in thread From: Igor Russkikh @ 2019-01-22 12:44 UTC (permalink / raw) To: Andrew Lunn; +Cc: David S . Miller, netdev@vger.kernel.org, Nikita Danilov >> -#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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct 2019-01-22 12:44 ` Igor Russkikh @ 2019-01-22 13:34 ` Andrew Lunn 2019-01-23 9:49 ` Igor Russkikh 0 siblings, 1 reply; 13+ messages in thread From: Andrew Lunn @ 2019-01-22 13:34 UTC (permalink / raw) To: Igor Russkikh; +Cc: David S . Miller, netdev@vger.kernel.org, Nikita Danilov 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct 2019-01-22 13:34 ` Andrew Lunn @ 2019-01-23 9:49 ` Igor Russkikh 2019-01-23 13:15 ` Andrew Lunn 0 siblings, 1 reply; 13+ messages in thread From: Igor Russkikh @ 2019-01-23 9:49 UTC (permalink / raw) To: Andrew Lunn; +Cc: David S . Miller, netdev@vger.kernel.org, Nikita Danilov > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct 2019-01-23 9:49 ` Igor Russkikh @ 2019-01-23 13:15 ` Andrew Lunn 2019-01-24 14:28 ` Igor Russkikh 0 siblings, 1 reply; 13+ messages in thread From: Andrew Lunn @ 2019-01-23 13:15 UTC (permalink / raw) To: Igor Russkikh; +Cc: David S . Miller, netdev@vger.kernel.org, Nikita Danilov 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct 2019-01-23 13:15 ` Andrew Lunn @ 2019-01-24 14:28 ` Igor Russkikh 0 siblings, 0 replies; 13+ messages in thread From: Igor Russkikh @ 2019-01-24 14:28 UTC (permalink / raw) To: Andrew Lunn; +Cc: David S . Miller, netdev@vger.kernel.org, Nikita Danilov > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-01-24 14:28 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-01-21 14:53 [PATCH net 0/5] net: aquantia: minor bug fixes after static analysis Igor Russkikh 2019-01-21 14:53 ` [PATCH net 1/5] net: aquantia: fixed memcpy size Igor Russkikh 2019-01-21 14:53 ` [PATCH net 2/5] net: aquantia: added newline at end of file Igor Russkikh 2019-01-21 14:53 ` [PATCH net 3/5] net: aquantia: fixed buffer overflow Igor Russkikh 2019-01-21 14:53 ` [PATCH net 4/5] net: aquantia: fixed instack structure overflow Igor Russkikh 2019-01-21 17:10 ` Andrew Lunn 2019-01-21 14:53 ` [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct Igor Russkikh 2019-01-21 17:13 ` Andrew Lunn 2019-01-22 12:44 ` Igor Russkikh 2019-01-22 13:34 ` Andrew Lunn 2019-01-23 9:49 ` Igor Russkikh 2019-01-23 13:15 ` Andrew Lunn 2019-01-24 14:28 ` Igor Russkikh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).