Re: [PATCH net-next] net: ena: Fix Kconfig dependencies X86
Sure. Removing them and resubmit. On 10/17/18, 11:37 AM, "Sergei Shtylyov" wrote: Hello! On 17.10.2018 11:16, neta...@amazon.com wrote: > From: Netanel Belgazal > > The Kconfig limitation of X86 is to too wide. > The ENA driver only requires a little endian dependency. > > Change the dependency to be on little endian CPU. > > Signed-off-by: Netanel Belgazal > --- > drivers/net/ethernet/amazon/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/amazon/Kconfig b/drivers/net/ethernet/amazon/Kconfig > index 99b30353541a..f4d16c7e104f 100644 > --- a/drivers/net/ethernet/amazon/Kconfig > +++ b/drivers/net/ethernet/amazon/Kconfig > @@ -17,7 +17,7 @@ if NET_VENDOR_AMAZON > > config ENA_ETHERNET > tristate "Elastic Network Adapter (ENA) support" > - depends on (PCI_MSI && X86) > + depends on (PCI_MSI && !CPU_BIG_ENDIAN) Parens not needed here. High time to remove them, I think. [...] MBR, Sergei
Re: [PATCH v4 17/17] net: ena: Eliminate duplicate barriers on weakly-ordered archs
I think you should either add a parameter to ena_com_write_sq_doorbell() or add
ena_com_write_sq_doorbell_rel().
Right now, you have unused function.
On 3/20/18, 4:43 AM, "Sinan Kaya" wrote:
Code includes barrier() followed by writel(). writel() already has a
barrier
on some architectures like arm64.
This ends up CPU observing two barriers back to back before executing the
register write.
Create a new wrapper function with relaxed write operator. Use the new
wrapper when a write is following a barrier().
Since code already has an explicit barrier call, changing writel() to
writel_relaxed().
Signed-off-by: Sinan Kaya
---
drivers/net/ethernet/amazon/ena/ena_com.c | 6 --
drivers/net/ethernet/amazon/ena/ena_eth_com.h | 22 --
drivers/net/ethernet/amazon/ena/ena_netdev.c | 4 ++--
3 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c
b/drivers/net/ethernet/amazon/ena/ena_com.c
index bf2de52..b6e628f 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -631,7 +631,8 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev
*ena_dev, u16 offset)
*/
wmb();
- writel(mmio_read_reg, ena_dev->reg_bar + ENA_REGS_MMIO_REG_READ_OFF);
+ writel_relaxed(mmio_read_reg,
+ ena_dev->reg_bar + ENA_REGS_MMIO_REG_READ_OFF);
for (i = 0; i < timeout; i++) {
if (read_resp->req_id == mmio_read->seq_num)
@@ -1826,7 +1827,8 @@ void ena_com_aenq_intr_handler(struct ena_com_dev
*dev, void *data)
/* write the aenq doorbell after all AENQ descriptors were read */
mb();
- writel((u32)aenq->head, dev->reg_bar + ENA_REGS_AENQ_HEAD_DB_OFF);
+ writel_relaxed((u32)aenq->head,
+ dev->reg_bar + ENA_REGS_AENQ_HEAD_DB_OFF);
}
int ena_com_dev_reset(struct ena_com_dev *ena_dev,
diff --git a/drivers/net/ethernet/amazon/ena/ena_eth_com.h
b/drivers/net/ethernet/amazon/ena/ena_eth_com.h
index 2f76572..09ef7cd 100644
--- a/drivers/net/ethernet/amazon/ena/ena_eth_com.h
+++ b/drivers/net/ethernet/amazon/ena/ena_eth_com.h
@@ -107,7 +107,8 @@ static inline int ena_com_sq_empty_space(struct
ena_com_io_sq *io_sq)
return io_sq->q_depth - 1 - cnt;
}
-static inline int ena_com_write_sq_doorbell(struct ena_com_io_sq *io_sq)
+static inline int ena_com_write_sq_doorbell(struct ena_com_io_sq *io_sq,
+ bool relaxed)
{
u16 tail;
@@ -116,7 +117,24 @@ static inline int ena_com_write_sq_doorbell(struct
ena_com_io_sq *io_sq)
pr_debug("write submission queue doorbell for queue: %d tail: %d\n",
io_sq->qid, tail);
- writel(tail, io_sq->db_addr);
+ if (relaxed)
+ writel_relaxed(tail, io_sq->db_addr);
+ else
+ writel(tail, io_sq->db_addr);
+
+ return 0;
+}
+
+static inline int ena_com_write_sq_doorbell_rel(struct ena_com_io_sq
*io_sq)
+{
+ u16 tail;
+
+ tail = io_sq->tail;
+
+ pr_debug("write submission queue doorbell for queue: %d tail: %d\n",
+io_sq->qid, tail);
+
+ writel_relaxed(tail, io_sq->db_addr);
return 0;
}
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 6975150..0530201 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -556,7 +556,7 @@ static int ena_refill_rx_bufs(struct ena_ring *rx_ring,
u32 num)
* issue a doorbell
*/
wmb();
- ena_com_write_sq_doorbell(rx_ring->ena_com_io_sq);
+ ena_com_write_sq_doorbell(rx_ring->ena_com_io_sq, true);
}
rx_ring->next_to_use = next_to_use;
@@ -2151,7 +2151,7 @@ static netdev_tx_t ena_start_xmit(struct sk_buff
*skb, struct net_device *dev)
if (netif_xmit_stopped(txq) || !skb->xmit_more) {
/* trigger the dma engine */
- ena_com_write_sq_doorbell(tx_ring->ena_com_io_sq);
+ ena_com_write_sq_doorbell(tx_ring->ena_com_io_sq, false);
u64_stats_update_begin(&tx_ring->syncp);
tx_ring->tx_stats.doorbells++;
u64_stats_update_end(&tx_ring->syncp);
--
2.7.4
Re: [PATCH net 3/3] eet: ena: invoke netif_carrier_off() only after netdev registered
Right.
I’ll remove this patch.
On 1/2/18, 9:08 PM, "David Miller" wrote:
From:
Date: Thu, 28 Dec 2017 21:30:20 +
> From: Netanel Belgazal
>
> netif_carrier_off() should be called only after register netdev.
> Move the function's call after the registration.
>
> Signed-off-by: Netanel Belgazal
> ---
> drivers/net/ethernet/amazon/ena/ena_netdev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index fbe21a817bd8..ee50c56765a4 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -3276,14 +3276,14 @@ static int ena_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
>
> memcpy(adapter->netdev->perm_addr, adapter->mac_addr, netdev->addr_len);
>
> - netif_carrier_off(netdev);
> -
> rc = register_netdev(netdev);
> if (rc) {
> dev_err(&pdev->dev, "Cannot register net device\n");
> goto err_rss;
> }
>
> + netif_carrier_off(netdev);
> +
You cannot invoke this after register_netdev(), asynchronous activity can
cause this
call to lose information and lose a link up event.
Re: [PATCH net 3/3] eet: ena: invoke netif_carrier_off() only after netdev registered
Yes, I mean in my driver. netif_carrier_off() have no effect when netdev is uninitialized. So I must call it after register_netdev(). On 12/29/17, 9:46 AM, "Jakub Kicinski" wrote: By "should" you mean in your driver, right? I think calling netif_carrier_off() on an unregistered netdev is a pretty standard thing to do for drivers which manage carrier state.
Re: [PATCH net-next 12/13] net: ena: change validate_tx_req_id() to be inline function
The optimization purpose I mention was to inform the compiler to inline this
function as there is only one caller to this function.
After reading the coding style you refer to I'll discard this patch.
As a side note, I checked the disassembly code and I can see that gcc inline
the function even without the explicit hint.
Regards,
Netanel
From: Leon Romanovsky
Sent: Monday, June 19, 2017 7:56 AM
To: Belgazal, Netanel
Cc: da...@davemloft.net; netdev@vger.kernel.org; Woodhouse, David; Machulsky,
Zorik; Matushevsky, Alexander; BSHARA, Said; Wilson, Matt; Liguori, Anthony;
Bshara, Nafea; Schmeilin, Evgeny
Subject: Re: [PATCH net-next 12/13] net: ena: change validate_tx_req_id() to be
inline function
On Sun, Jun 18, 2017 at 02:28:17PM +0300, neta...@amazon.com wrote:
> From: Netanel Belgazal
>
> for optimization purpose, change validate_tx_req_id() to be
> inline function.
>
> Signed-off-by: Netanel Belgazal
> ---
> drivers/net/ethernet/amazon/ena/ena_netdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 4540cd3d9f5f..da14b78cc87c 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -698,7 +698,7 @@ static void ena_destroy_all_io_queues(struct ena_adapter
> *adapter)
> ena_destroy_all_rx_queues(adapter);
> }
>
> -static int validate_tx_req_id(struct ena_ring *tx_ring, u16 req_id)
> +static inline int validate_tx_req_id(struct ena_ring *tx_ring, u16 req_id)
inline in C-file?
Please read Documentation/process/coding-style.rst,
15) The inline disease" section why it is wrong and if you anyway
insists on doing it, please provide support of your claim "optimization
purposes" and show what and how exactly your optimization happened.
Thanks
> {
> struct ena_tx_buffer *tx_info = NULL;
>
> --
> 2.7.4
>
Re: [PATCH net-next 09/13] net: ena: adding missing cast in ena_com_mem_addr_set()
Ack, Will use the proposed functions. From: Leon Romanovsky Sent: Monday, June 19, 2017 8:00 AM To: Belgazal, Netanel Cc: da...@davemloft.net; netdev@vger.kernel.org; Woodhouse, David; Machulsky, Zorik; Matushevsky, Alexander; BSHARA, Said; Wilson, Matt; Liguori, Anthony; Bshara, Nafea; Schmeilin, Evgeny Subject: Re: [PATCH net-next 09/13] net: ena: adding missing cast in ena_com_mem_addr_set() On Sun, Jun 18, 2017 at 02:28:14PM +0300, neta...@amazon.com wrote: > From: Netanel Belgazal > > Signed-off-by: Netanel Belgazal > --- > drivers/net/ethernet/amazon/ena/ena_com.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c > b/drivers/net/ethernet/amazon/ena/ena_com.c > index f6e1d30523a6..8efb85e25a42 100644 > --- a/drivers/net/ethernet/amazon/ena/ena_com.c > +++ b/drivers/net/ethernet/amazon/ena/ena_com.c > @@ -100,7 +100,7 @@ static inline int ena_com_mem_addr_set(struct ena_com_dev > *ena_dev, > } > > ena_addr->mem_addr_low = (u32)addr; > - ena_addr->mem_addr_high = (u64)addr >> 32; > + ena_addr->mem_addr_high = (u16)((u64)addr >> 32); We have macro for getting upper 32 bits - upper_32_bits(addr), the same goes for lower 32 bits. > > return 0; > } > -- > 2.7.4 >
Re: [PATCH net-next 10/13] net: ena: add mtu limitation in ena_change_mtu()
I'll discard this patch. From: David Miller Sent: Sunday, June 18, 2017 7:21 PM To: Belgazal, Netanel Cc: netdev@vger.kernel.org; Woodhouse, David; Machulsky, Zorik; Matushevsky, Alexander; BSHARA, Said; Wilson, Matt; Liguori, Anthony; Bshara, Nafea; Schmeilin, Evgeny Subject: Re: [PATCH net-next 10/13] net: ena: add mtu limitation in ena_change_mtu() From: Date: Sun, 18 Jun 2017 14:28:15 +0300 > From: Netanel Belgazal > > Signed-off-by: Netanel Belgazal I don't understand this at all. This whole reason we have those: > @@ -3008,8 +3015,6 @@ static void ena_set_conf_feat_params(struct ena_adapter > *adapter, > ena_set_dev_offloads(feat, netdev); > > adapter->max_mtu = feat->dev_attr.max_mtu; > - netdev->max_mtu = adapter->max_mtu; > - netdev->min_mtu = ENA_MIN_MTU; > } > assignments you are removing is so that the core networking can validate the request and therefore individual drivers don't have to. If it's just to get that silly driver message out when the MTU asked for is too large, that's not a good reason to bypass the infrastructure for MTU changes that we've provided for drivers in the core. I don't like this change at all. You could have helped things a lot by actually writing a commit log message explaining what you are really doing, and why you are doing it. Empty commit log messages are never a good idea.
Re: [PATCH net 0/9] Bugs fixes in ena ethernet driver
Thank you David, Do you might have an ETA when do you plan to merge net branch into net-next? I plan to base my new patchset to net-next on top of this one. Regards, Netanel From: David Miller Sent: Sunday, June 11, 2017 11:38 PM To: Belgazal, Netanel Cc: netdev@vger.kernel.org; Woodhouse, David; Machulsky, Zorik; Matushevsky, Alexander; BSHARA, Said; Wilson, Matt; Liguori, Anthony; Bshara, Nafea; Schmeilin, Evgeny Subject: Re: [PATCH net 0/9] Bugs fixes in ena ethernet driver From: Date: Sun, 11 Jun 2017 15:42:42 +0300 > This patchset contains fixes for the bugs that were discovered so far. Series applied.
Re: [PATCH net-next 0/8] Bug fixes in ena ethernet driver
Ack.
resubmitting to net branch and adding "Fixes" mark.
From: David Miller
Sent: Saturday, June 10, 2017 11:11 PM
To: f.faine...@gmail.com
Cc: Belgazal, Netanel; netdev@vger.kernel.org; Woodhouse, David; Machulsky,
Zorik; Matushevsky, Alexander; BSHARA, Said; Wilson, Matt; Liguori, Anthony;
Bshara, Nafea; Schmeilin, Evgeny
Subject: Re: [PATCH net-next 0/8] Bug fixes in ena ethernet driver
From: Florian Fainelli
Date: Fri, 9 Jun 2017 15:19:54 -0700
> On 06/09/2017 03:13 PM, neta...@amazon.com wrote:
>> From: Netanel Belgazal
>>
>> This patchset contains fixes for the bugs that were discovered so far.
>
> If these are all fixes you should submit them against the "net" tree.
> net-next is for features [1].
>
> Since these are fixes, you may also want to provide a Fixes: 12-digit
> commit ("commit subject") [2] such that David can queue these patches
> for stable trees and this can be retrofitted into kernel distributions.
>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.txt#n25
>
> [2]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n183
Yeah I agree. If they are genuine bug fixes they should be submitted
against 'net'. And yes, Fixes: tags are quite desirable as well.
Re: [PATCH net-next 0/8] Bug fixes in ena ethernet driver
I the last minute I fixed patchset #6 commit subject from stuck to hang and I forget to remove it. Sorry for that. resubmitted. From: David Miller Sent: Friday, June 9, 2017 10:33 PM To: Belgazal, Netanel Cc: netdev@vger.kernel.org; Woodhouse, David; Machulsky, Zorik; Matushevsky, Alexander; BSHARA, Said; Wilson, Matt; Liguori, Anthony; Bshara, Nafea; Schmeilin, Evgeny Subject: Re: [PATCH net-next 0/8] Bug fixes in ena ethernet driver From: Date: Fri, 9 Jun 2017 09:55:16 +0300 > This patchset contains fixes for the bugs that were discovered so far. You submitted patch #6 twice, once with the word "stuck" in the subject line, once with the word "hang" in the subject line. Please sort this out and resubmit, thanks.
Re: [PATCH net-next 0/8] Bug fixes in ena ethernet driver
My apologies, I was not aware. Will make sure this won't happen again. Regards, Netanel From: David Miller Sent: Friday, June 9, 2017 2:17 AM To: Belgazal, Netanel Cc: netdev@vger.kernel.org; Woodhouse, David; Machulsky, Zorik; Matushevsky, Alexander; BSHARA, Said; Wilson, Matt; Liguori, Anthony; Bshara, Nafea; Schmeilin, Evgeny Subject: Re: [PATCH net-next 0/8] Bug fixes in ena ethernet driver Two parallel patch series to the same driver and targetting the same GIT tree is extremely undesirable, please don't do this. Submit one series, and once applied submit the second series. I'm deleting all of your patches from my queue, please resubmit things properly. Thank you.
