| Message ID | 8bae843a7f567eb7187cf7b84372d96e4914b3e7.1547878835.git.igor.russkikh@aquantia.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: aquantia: minor bug fixes after static analysis | expand |
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
>> -#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
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
> > 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
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
> 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
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;