| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20191015113317.6413f912@cakuba.netronome.com>
Date: Tue, 15 Oct 2019 11:33:17 -0700
From: Jakub Kicinski <jakub.kicinski@...ronome.com>
To: Igor Russkikh <Igor.Russkikh@...antia.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2 net 2/4] net: aquantia: when cleaning hw cache it
should be toggled
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@...antia.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"
Powered by blists - more mailing lists