| Message ID | d89180cd7ddf6981310179108b37a8d15c44c02f.1570787323.git.igor.russkikh@aquantia.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | Aquantia/Marvell AQtion atlantic driver fixes 10/2019 | expand |
On Fri, 11 Oct 2019 13:45:20 +0000, Igor Russkikh wrote: > From HW specification to correctly reset HW caches (this is a required > workaround when stopping the device), register bit should actually > be toggled. Does the bit get set by the driver or HW? If it gets set by HW there is still a tiny race from reading to writing.. Perhaps doing two writes -> to 0 and to 1 would be a better option? Just wondering, obviously I don't know your HW :) > It was previosly always just set. Due to the way driver stops HW this > never actually caused any issues, but it still may, so cleaning this up. Hm. So is it a cleanup of fix? Does the way code is written guarantee it will never cause issues? > Fixes: 7a1bb49461b1 ("net: aquantia: fix potential IOMMU fault after driver unbind") > Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com> > --- > .../aquantia/atlantic/hw_atl/hw_atl_b0.c | 16 ++++++++++++++-- > .../aquantia/atlantic/hw_atl/hw_atl_llh.c | 17 +++++++++++++++-- > .../aquantia/atlantic/hw_atl/hw_atl_llh.h | 7 +++++-- > .../atlantic/hw_atl/hw_atl_llh_internal.h | 19 +++++++++++++++++++ > 4 files changed, 53 insertions(+), 6 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 30f7fc4c97ff..3459fadb7ddd 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 > @@ -968,14 +968,26 @@ static int hw_atl_b0_hw_interrupt_moderation_set(struct aq_hw_s *self) > > static int hw_atl_b0_hw_stop(struct aq_hw_s *self) > { > + int err; > + u32 val; > + > hw_atl_b0_hw_irq_disable(self, HW_ATL_B0_INT_MASK); > > /* Invalidate Descriptor Cache to prevent writing to the cached > * descriptors and to the data pointer of those descriptors > */ > - hw_atl_rdm_rx_dma_desc_cache_init_set(self, 1); > + hw_atl_rdm_rx_dma_desc_cache_init_tgl(self); > > - return aq_hw_err_from_flags(self); > + err = aq_hw_err_from_flags(self); > + > + if (err) > + goto err_exit; > + > + readx_poll_timeout_atomic(hw_atl_rdm_rx_dma_desc_cache_init_done_get, > + self, val, val == 1, 1000U, 10000U); It's a little strange to toggle, yet wait for it to be of a specific value.. > + > +err_exit: > + return err; Just return err instead of doing this pointless goto. It make the code harder to follow. > } > > static int hw_atl_b0_hw_ring_tx_stop(struct aq_hw_s *self, > diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c > index 1149812ae463..6f340695e6bd 100644 > --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c > +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c > @@ -606,12 +606,25 @@ void hw_atl_rpb_rx_flow_ctl_mode_set(struct aq_hw_s *aq_hw, u32 rx_flow_ctl_mode > HW_ATL_RPB_RX_FC_MODE_SHIFT, rx_flow_ctl_mode); > } > > -void hw_atl_rdm_rx_dma_desc_cache_init_set(struct aq_hw_s *aq_hw, u32 init) > +void hw_atl_rdm_rx_dma_desc_cache_init_tgl(struct aq_hw_s *aq_hw) > { > + u32 val; > + > + val = aq_hw_read_reg_bit(aq_hw, HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_ADR, > + HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_MSK, > + HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_SHIFT); hw_atl_rdm_rx_dma_desc_cache_init_done_get() ? > aq_hw_write_reg_bit(aq_hw, HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_ADR, > HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_MSK, > HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_SHIFT, > - init); > + val ^ 1); > +} > + > +u32 hw_atl_rdm_rx_dma_desc_cache_init_done_get(struct aq_hw_s *aq_hw) > +{ > + return aq_hw_read_reg_bit(aq_hw, RDM_RX_DMA_DESC_CACHE_INIT_DONE_ADR, > + RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSK, > + RDM_RX_DMA_DESC_CACHE_INIT_DONE_SHIFT); > } > > void hw_atl_rpb_rx_pkt_buff_size_per_tc_set(struct aq_hw_s *aq_hw, > diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h > index 0c37abbabca5..c3ee278c3747 100644 > --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h > +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h > @@ -313,8 +313,11 @@ void hw_atl_rpb_rx_pkt_buff_size_per_tc_set(struct aq_hw_s *aq_hw, > u32 rx_pkt_buff_size_per_tc, > u32 buffer); > > -/* set rdm rx dma descriptor cache init */ > -void hw_atl_rdm_rx_dma_desc_cache_init_set(struct aq_hw_s *aq_hw, u32 init); > +/* toggle rdm rx dma descriptor cache init */ > +void hw_atl_rdm_rx_dma_desc_cache_init_tgl(struct aq_hw_s *aq_hw); > + > +/* get rdm rx dma descriptor cache init done */ > +u32 hw_atl_rdm_rx_dma_desc_cache_init_done_get(struct aq_hw_s *aq_hw); > > /* set rx xoff enable (per tc) */ > void hw_atl_rpb_rx_xoff_en_per_tc_set(struct aq_hw_s *aq_hw, u32 rx_xoff_en_per_tc, > diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h > index c3febcdfa92e..35887ad89025 100644 > --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h > +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h > @@ -318,6 +318,25 @@ > /* default value of bitfield rdm_desc_init_i */ > #define HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_DEFAULT 0x0 > > +/* rdm_desc_init_done_i bitfield definitions > + * preprocessor definitions for the bitfield rdm_desc_init_done_i. > + * port="pif_rdm_desc_init_done_i" > + */ > + > +/* register address for bitfield rdm_desc_init_done_i */ > +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_ADR 0x00005a10 > +/* bitmask for bitfield rdm_desc_init_done_i */ > +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSK 0x00000001U > +/* inverted bitmask for bitfield rdm_desc_init_done_i */ > +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSKN 0xfffffffe > +/* lower bit position of bitfield rdm_desc_init_done_i */ > +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_SHIFT 0U > +/* width of bitfield rdm_desc_init_done_i */ > +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_WIDTH 1 > +/* default value of bitfield rdm_desc_init_done_i */ > +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_DEFAULT 0x0 > + > + two empty lines here? > /* rx int_desc_wrb_en bitfield definitions > * preprocessor definitions for the bitfield "int_desc_wrb_en". > * port="pif_rdm_int_desc_wrb_en_i"
Hello Jakub, >> workaround when stopping the device), register bit should actually >> be toggled. > > Does the bit get set by the driver or HW? > > If it gets set by HW there is still a tiny race from reading to > writing.. Perhaps doing two writes -> to 0 and to 1 would be a better > option? No, set is done by the driver, not HW. HW just tracks for the toggle. >> It was previosly always just set. Due to the way driver stops HW this >> never actually caused any issues, but it still may, so cleaning this up. > > Hm. So is it a cleanup of fix? Does the way code is written guarantee > it will never cause issues? Yes, thats a cleanup. We just had other products where this cache reset had to be done multiple times. Obviously doing that second time was just no-op for hardware ;) On linux this always gets called on deinit only once - thus it was safe. We just aligning here the linux driver with actual HW specification. >> + if (err) >> + goto err_exit; >> + >> + readx_poll_timeout_atomic(hw_atl_rdm_rx_dma_desc_cache_init_done_get, >> + self, val, val == 1, 1000U, 10000U); > > It's a little strange to toggle, yet wait for it to be of a specific > value.. Notice thats a different value - 'cache_init_done' bit. This is used by HW to indicate completion of cache reset operation. >> +err_exit: >> + return err; > > Just return err instead of doing this pointless goto. It make the code > harder to follow. Sure >> +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_DEFAULT 0x0 >> + >> + > > two empty lines here? Will fix. Regards, Igor
On Wed, 16 Oct 2019 13:19:30 +0000, Igor Russkikh wrote: > >> It was previosly always just set. Due to the way driver stops HW this > >> never actually caused any issues, but it still may, so cleaning this up. > > > > Hm. So is it a cleanup of fix? Does the way code is written guarantee > > it will never cause issues? > > Yes, thats a cleanup. We just had other products where this cache reset had to > be done multiple times. Obviously doing that second time was just no-op for > hardware ;) > On linux this always gets called on deinit only once - thus it was safe. > We just aligning here the linux driver with actual HW specification. I see, in light of that explanation I think it'd be appropriate to take the Fixes tag away and move this patch to the net-next series. > >> + if (err) > >> + goto err_exit; > >> + > >> + readx_poll_timeout_atomic(hw_atl_rdm_rx_dma_desc_cache_init_done_get, > >> + self, val, val == 1, 1000U, 10000U); > > > > It's a little strange to toggle, yet wait for it to be of a specific > > value.. > > Notice thats a different value - 'cache_init_done' bit. > This is used by HW to indicate completion of cache reset operation. Ah, ack, it's an "eyeful".. :)
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 30f7fc4c97ff..3459fadb7ddd 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 @@ -968,14 +968,26 @@ static int hw_atl_b0_hw_interrupt_moderation_set(struct aq_hw_s *self) static int hw_atl_b0_hw_stop(struct aq_hw_s *self) { + int err; + u32 val; + hw_atl_b0_hw_irq_disable(self, HW_ATL_B0_INT_MASK); /* Invalidate Descriptor Cache to prevent writing to the cached * descriptors and to the data pointer of those descriptors */ - hw_atl_rdm_rx_dma_desc_cache_init_set(self, 1); + hw_atl_rdm_rx_dma_desc_cache_init_tgl(self); - return aq_hw_err_from_flags(self); + err = aq_hw_err_from_flags(self); + + if (err) + goto err_exit; + + readx_poll_timeout_atomic(hw_atl_rdm_rx_dma_desc_cache_init_done_get, + self, val, val == 1, 1000U, 10000U); + +err_exit: + return err; } static int hw_atl_b0_hw_ring_tx_stop(struct aq_hw_s *self, diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c index 1149812ae463..6f340695e6bd 100644 --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.c @@ -606,12 +606,25 @@ void hw_atl_rpb_rx_flow_ctl_mode_set(struct aq_hw_s *aq_hw, u32 rx_flow_ctl_mode HW_ATL_RPB_RX_FC_MODE_SHIFT, rx_flow_ctl_mode); } -void hw_atl_rdm_rx_dma_desc_cache_init_set(struct aq_hw_s *aq_hw, u32 init) +void hw_atl_rdm_rx_dma_desc_cache_init_tgl(struct aq_hw_s *aq_hw) { + u32 val; + + val = aq_hw_read_reg_bit(aq_hw, HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_ADR, + HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_MSK, + HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_SHIFT); + aq_hw_write_reg_bit(aq_hw, HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_ADR, HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_MSK, HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_SHIFT, - init); + val ^ 1); +} + +u32 hw_atl_rdm_rx_dma_desc_cache_init_done_get(struct aq_hw_s *aq_hw) +{ + return aq_hw_read_reg_bit(aq_hw, RDM_RX_DMA_DESC_CACHE_INIT_DONE_ADR, + RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSK, + RDM_RX_DMA_DESC_CACHE_INIT_DONE_SHIFT); } void hw_atl_rpb_rx_pkt_buff_size_per_tc_set(struct aq_hw_s *aq_hw, diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h index 0c37abbabca5..c3ee278c3747 100644 --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh.h @@ -313,8 +313,11 @@ void hw_atl_rpb_rx_pkt_buff_size_per_tc_set(struct aq_hw_s *aq_hw, u32 rx_pkt_buff_size_per_tc, u32 buffer); -/* set rdm rx dma descriptor cache init */ -void hw_atl_rdm_rx_dma_desc_cache_init_set(struct aq_hw_s *aq_hw, u32 init); +/* toggle rdm rx dma descriptor cache init */ +void hw_atl_rdm_rx_dma_desc_cache_init_tgl(struct aq_hw_s *aq_hw); + +/* get rdm rx dma descriptor cache init done */ +u32 hw_atl_rdm_rx_dma_desc_cache_init_done_get(struct aq_hw_s *aq_hw); /* set rx xoff enable (per tc) */ void hw_atl_rpb_rx_xoff_en_per_tc_set(struct aq_hw_s *aq_hw, u32 rx_xoff_en_per_tc, diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h index c3febcdfa92e..35887ad89025 100644 --- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_llh_internal.h @@ -318,6 +318,25 @@ /* default value of bitfield rdm_desc_init_i */ #define HW_ATL_RDM_RX_DMA_DESC_CACHE_INIT_DEFAULT 0x0 +/* rdm_desc_init_done_i bitfield definitions + * preprocessor definitions for the bitfield rdm_desc_init_done_i. + * port="pif_rdm_desc_init_done_i" + */ + +/* register address for bitfield rdm_desc_init_done_i */ +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_ADR 0x00005a10 +/* bitmask for bitfield rdm_desc_init_done_i */ +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSK 0x00000001U +/* inverted bitmask for bitfield rdm_desc_init_done_i */ +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_MSKN 0xfffffffe +/* lower bit position of bitfield rdm_desc_init_done_i */ +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_SHIFT 0U +/* width of bitfield rdm_desc_init_done_i */ +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_WIDTH 1 +/* default value of bitfield rdm_desc_init_done_i */ +#define RDM_RX_DMA_DESC_CACHE_INIT_DONE_DEFAULT 0x0 + + /* rx int_desc_wrb_en bitfield definitions * preprocessor definitions for the bitfield "int_desc_wrb_en". * port="pif_rdm_int_desc_wrb_en_i"
From HW specification to correctly reset HW caches (this is a required workaround when stopping the device), register bit should actually be toggled. It was previosly always just set. Due to the way driver stops HW this never actually caused any issues, but it still may, so cleaning this up. Fixes: 7a1bb49461b1 ("net: aquantia: fix potential IOMMU fault after driver unbind") Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com> --- .../aquantia/atlantic/hw_atl/hw_atl_b0.c | 16 ++++++++++++++-- .../aquantia/atlantic/hw_atl/hw_atl_llh.c | 17 +++++++++++++++-- .../aquantia/atlantic/hw_atl/hw_atl_llh.h | 7 +++++-- .../atlantic/hw_atl/hw_atl_llh_internal.h | 19 +++++++++++++++++++ 4 files changed, 53 insertions(+), 6 deletions(-)