Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Netanel Belgazal]

On 06/21/2016 01:43 AM, Francois Romieu wrote:
> Netanel Belgazal  :
> [...]
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h 
>>> b/drivers/net/ethernet/amazon/ena/ena_com.h
>>> new file mode 100644
>>> index 000..e49ba43
>>> --- /dev/null
>>> [...]
>>> +static inline void ena_com_update_intr_reg(struct ena_eth_io_intr_reg 
>>> *intr_reg,
>>> +  u32 rx_delay_interval,
>>> +  u32 tx_delay_interval,
>>> +  bool unmask)
>>> +{
>>> +   intr_reg->intr_control = 0;
>>> +   intr_reg->intr_control |= rx_delay_interval &
>>> +   ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
>>> +
>>> +   intr_reg->intr_control |=
>>> +   (tx_delay_interval << ENA_ETH_IO_INTR_REG_TX_INTR_DELAY_SHIFT)
>>> +   & ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
>>>   ^^ TX ?
>>>
>>> Extra: you should not return NETDEV_TX_BUSY in the xmit handler while
>>> queueing is still enabled. Please drop packet and return NETDEV_TX_OK.
>> Ack.
>>
>> Thanks for your review.
> Ack/nack regarding use of ..._RX_INTR_DELAY_MASK with ..._TX_INTR_DELAY_SHIFT 
> ?
You are right, it should have been TX_INTR_DELAY_SHIFT.
I'll fix that.

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Netanel Belgazal]

On 06/21/2016 01:43 AM, Francois Romieu wrote:
> Netanel Belgazal  :
> [...]
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h 
>>> b/drivers/net/ethernet/amazon/ena/ena_com.h
>>> new file mode 100644
>>> index 000..e49ba43
>>> --- /dev/null
>>> [...]
>>> +static inline void ena_com_update_intr_reg(struct ena_eth_io_intr_reg 
>>> *intr_reg,
>>> +  u32 rx_delay_interval,
>>> +  u32 tx_delay_interval,
>>> +  bool unmask)
>>> +{
>>> +   intr_reg->intr_control = 0;
>>> +   intr_reg->intr_control |= rx_delay_interval &
>>> +   ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
>>> +
>>> +   intr_reg->intr_control |=
>>> +   (tx_delay_interval << ENA_ETH_IO_INTR_REG_TX_INTR_DELAY_SHIFT)
>>> +   & ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
>>>   ^^ TX ?
>>>
>>> Extra: you should not return NETDEV_TX_BUSY in the xmit handler while
>>> queueing is still enabled. Please drop packet and return NETDEV_TX_OK.
>> Ack.
>>
>> Thanks for your review.
> Ack/nack regarding use of ..._RX_INTR_DELAY_MASK with ..._TX_INTR_DELAY_SHIFT 
> ?
You are right, it should have been TX_INTR_DELAY_SHIFT.
I'll fix that.

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Francois Romieu]

Netanel Belgazal  :
[...]
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h 
> > b/drivers/net/ethernet/amazon/ena/ena_com.h
> > new file mode 100644
> > index 000..e49ba43
> > --- /dev/null
> > [...]
> > +static inline void ena_com_update_intr_reg(struct ena_eth_io_intr_reg 
> > *intr_reg,
> > +  u32 rx_delay_interval,
> > +  u32 tx_delay_interval,
> > +  bool unmask)
> > +{
> > +   intr_reg->intr_control = 0;
> > +   intr_reg->intr_control |= rx_delay_interval &
> > +   ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
> > +
> > +   intr_reg->intr_control |=
> > +   (tx_delay_interval << ENA_ETH_IO_INTR_REG_TX_INTR_DELAY_SHIFT)
> > +   & ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
> >   ^^ TX ?
> >
> > Extra: you should not return NETDEV_TX_BUSY in the xmit handler while
> > queueing is still enabled. Please drop packet and return NETDEV_TX_OK.
> Ack.
> 
> Thanks for your review.

Ack/nack regarding use of ..._RX_INTR_DELAY_MASK with ..._TX_INTR_DELAY_SHIFT ?

-- 
Ueimor

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Francois Romieu]

Netanel Belgazal  :
[...]
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h 
> > b/drivers/net/ethernet/amazon/ena/ena_com.h
> > new file mode 100644
> > index 000..e49ba43
> > --- /dev/null
> > [...]
> > +static inline void ena_com_update_intr_reg(struct ena_eth_io_intr_reg 
> > *intr_reg,
> > +  u32 rx_delay_interval,
> > +  u32 tx_delay_interval,
> > +  bool unmask)
> > +{
> > +   intr_reg->intr_control = 0;
> > +   intr_reg->intr_control |= rx_delay_interval &
> > +   ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
> > +
> > +   intr_reg->intr_control |=
> > +   (tx_delay_interval << ENA_ETH_IO_INTR_REG_TX_INTR_DELAY_SHIFT)
> > +   & ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
> >   ^^ TX ?
> >
> > Extra: you should not return NETDEV_TX_BUSY in the xmit handler while
> > queueing is still enabled. Please drop packet and return NETDEV_TX_OK.
> Ack.
> 
> Thanks for your review.

Ack/nack regarding use of ..._RX_INTR_DELAY_MASK with ..._TX_INTR_DELAY_SHIFT ?

-- 
Ueimor

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Netanel Belgazal]

On 06/20/2016 08:20 AM, Rosen, Rami wrote:
> Hi all, 
>
> A very limited review below.
>
> +
> + /* get capabilities of particular feature */
> + ENA_ADMIN_GET_FEATURE = 8,
>
> Instead /* get capabilities  SHOULD BE:  /* set capabilities .
> +
> + /* get capabilities of particular feature */
> + ENA_ADMIN_SET_FEATURE = 9,
> +
> ..
>
>
> +int ena_com_set_hash_ctrl(struct ena_com_dev *ena_dev)
> +{
> + struct ena_com_admin_queue *admin_queue = _dev->admin_queue;
> ...
>
> You set ret=-EINVAL, but you do not use this ret as you immediately return 0 
> in the next line, which is the end of the method. Either return ret or return 
> -EINVAL.
> + if (unlikely(ret)) {
> + ena_trc_err("Failed to set hash input. error: %d\n", ret);
> + ret = -EINVAL;
> + }
> +
> + return 0;
> +}
> +
I will return ret.
>
>
> +
> +/* ena_com_set_mmio_read_mode - Enable/disable the mmio reg read mechanism
> + * @ena_dev: ENA communication layer struct
>
>
> Instead realess_supported: SHOULD BE: readless_supported
ack
>
> + * @realess_supported: readless mode (enable/disable)
> + */
> +void ena_com_set_mmio_read_mode(struct ena_com_dev *ena_dev,
> + bool readless_supported);
> +
>
>
>
> +
> +/* ena_com_create_io_queue - Create io queue.
> + * @ena_dev: ENA communication layer struct
>
> Instead ena_com_create_io_ctx SHOULD BE: @ena_com_create_io_ctx
ack
>
> + * ena_com_create_io_ctx - create context structure
> + *
> + * Create the submission and the completion queues.
> + *
> + * @return - 0 on success, negative value on failure.
> + */
> +int ena_com_create_io_queue(struct ena_com_dev *ena_dev,
> + struct ena_com_create_io_ctx *ctx);
> +
>
>
>
>
> +/* ena_com_admin_destroy - Destroy IO queue with the queue id - qid.
> + * @ena_dev: ENA communication layer struct
>
> Missing: @qid
ack
>
> + */
> +void ena_com_destroy_io_queue(struct ena_com_dev *ena_dev, u16 qid);
> +
>
> Regards,
> Rami Rosen
> Intel Corporation
Thanks for the review

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Netanel Belgazal]

On 06/20/2016 08:20 AM, Rosen, Rami wrote:
> Hi all, 
>
> A very limited review below.
>
> +
> + /* get capabilities of particular feature */
> + ENA_ADMIN_GET_FEATURE = 8,
>
> Instead /* get capabilities  SHOULD BE:  /* set capabilities .
> +
> + /* get capabilities of particular feature */
> + ENA_ADMIN_SET_FEATURE = 9,
> +
> ..
>
>
> +int ena_com_set_hash_ctrl(struct ena_com_dev *ena_dev)
> +{
> + struct ena_com_admin_queue *admin_queue = _dev->admin_queue;
> ...
>
> You set ret=-EINVAL, but you do not use this ret as you immediately return 0 
> in the next line, which is the end of the method. Either return ret or return 
> -EINVAL.
> + if (unlikely(ret)) {
> + ena_trc_err("Failed to set hash input. error: %d\n", ret);
> + ret = -EINVAL;
> + }
> +
> + return 0;
> +}
> +
I will return ret.
>
>
> +
> +/* ena_com_set_mmio_read_mode - Enable/disable the mmio reg read mechanism
> + * @ena_dev: ENA communication layer struct
>
>
> Instead realess_supported: SHOULD BE: readless_supported
ack
>
> + * @realess_supported: readless mode (enable/disable)
> + */
> +void ena_com_set_mmio_read_mode(struct ena_com_dev *ena_dev,
> + bool readless_supported);
> +
>
>
>
> +
> +/* ena_com_create_io_queue - Create io queue.
> + * @ena_dev: ENA communication layer struct
>
> Instead ena_com_create_io_ctx SHOULD BE: @ena_com_create_io_ctx
ack
>
> + * ena_com_create_io_ctx - create context structure
> + *
> + * Create the submission and the completion queues.
> + *
> + * @return - 0 on success, negative value on failure.
> + */
> +int ena_com_create_io_queue(struct ena_com_dev *ena_dev,
> + struct ena_com_create_io_ctx *ctx);
> +
>
>
>
>
> +/* ena_com_admin_destroy - Destroy IO queue with the queue id - qid.
> + * @ena_dev: ENA communication layer struct
>
> Missing: @qid
ack
>
> + */
> +void ena_com_destroy_io_queue(struct ena_com_dev *ena_dev, u16 qid);
> +
>
> Regards,
> Rami Rosen
> Intel Corporation
Thanks for the review

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Netanel Belgazal]

On 06/18/2016 12:11 AM, Francois Romieu wrote:
> Netanel Belgazal  :
> [...]
>
> More stuff below.
>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
>> b/drivers/net/ethernet/amazon/ena/ena_com.c
>> new file mode 100644
>> index 000..357e10f
>> --- /dev/null
>> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> [...]
>> +static int ena_get_dev_stats(struct ena_com_dev *ena_dev,
>> + struct ena_admin_aq_get_stats_cmd *get_cmd,
>> + struct ena_admin_acq_get_stats_resp *get_resp,
> At first sight it should be possible avoid one pointer argument with:
>
> struct ena_something {
>   struct ena_admin_aq_get_stats_cmd cmd;
>   struct ena_admin_acq_get_stats_resp resp;
>   
> };
Ack
>
> [...]
>> +int ena_com_get_dev_extended_stats(struct ena_com_dev *ena_dev, char *buff,
>> +   u32 len)
> Where is it used ?
It is currently unused. I'll remove it from this patch and add it in a separate 
once we'll support it.
>
>> +{
>> +int ret = 0;
>> +struct ena_admin_aq_get_stats_cmd get_cmd;
>> +struct ena_admin_acq_get_stats_resp get_resp;
>> +void *virt_addr;
>   int rc = -ENOMEM;
>   char *str;
>
>> +dma_addr_t phys_addr;
>> +
>> +virt_addr = dma_alloc_coherent(ena_dev->dmadev,
>> +   len,
>> +   _addr,
>> +   GFP_KERNEL | __GFP_ZERO);
>> +if (!virt_addr) {
>> +ret = -ENOMEM;
>> +goto done;
>> +}
>> +memset(_cmd, 0x0, sizeof(get_cmd));
>> +ret = ena_com_mem_addr_set(ena_dev,
>> +   _cmd.u.control_buffer.address,
>> +   phys_addr);
>> +if (unlikely(ret)) {
>> +ena_trc_err("memory address set failed\n");
>> +return ret;
>> +}
>> +get_cmd.u.control_buffer.length = len;
>> +
>> +get_cmd.device_id = ena_dev->stats_func;
>> +get_cmd.queue_idx = ena_dev->stats_queue;
>> +
>> +ret = ena_get_dev_stats(ena_dev, _cmd, _resp,
>> +ENA_ADMIN_GET_STATS_TYPE_EXTENDED);
>> +if (ret < 0)
>> +goto free_ext_stats_mem;
>> +
>> +ret = snprintf(buff, len, "%s", (char *)virt_addr);
> So ENA_ADMIN_GET_STATS_TYPE_EXTENDED fills a buffer with bytes in the
> [0; 127] range, right ?

ENA_ADMIN_GET_STATS_TYPE_EXTENDED fills a buffer with 4K bytes.

>
>> +
>> +free_ext_stats_mem:
>> +dma_free_coherent(ena_dev->dmadev, len, virt_addr, phys_addr);
>> +done:
>> +return ret;
>> +}
>> +
>> +int ena_com_set_dev_mtu(struct ena_com_dev *ena_dev, int mtu)
>> +{
>> +struct ena_com_admin_queue *admin_queue;
>> +struct ena_admin_set_feat_cmd cmd;
>> +struct ena_admin_set_feat_resp resp;
>> +int ret = 0;
> Should be -ENODEV or left uninitialized.
I'll initialize it to -ENODEV
>
>> +
>> +if (unlikely(!ena_dev)) {
>> +ena_trc_err("%s : ena_dev is NULL\n", __func__);
>> +return -ENODEV;
>> +}
> Does it mean that ena_com_dev may go away while the net_device is in use ?
ena_com_dev can't go away while net_device is alive.
I'll remove this check.
>
>> +
>> +if (!ena_com_check_supported_feature_id(ena_dev, ENA_ADMIN_MTU)) {
>> +ena_trc_info("Feature %d isn't supported\n", ENA_ADMIN_MTU);
>> +return -EPERM;
>> +}
>   rc = ena_com_check_supported_feature_id(...
>   if (rc < 0) {
>   ena_trc_info(...
>   goto out;
>   }
>> +
>> +memset(, 0x0, sizeof(cmd));
>> +admin_queue = _dev->admin_queue;
>> +
>> +cmd.aq_common_descriptor.opcode = ENA_ADMIN_SET_FEATURE;
>> +cmd.aq_common_descriptor.flags = 0;
>> +cmd.feat_common.feature_id = ENA_ADMIN_MTU;
>> +cmd.u.mtu.mtu = mtu;
>> +
>> +ret = ena_com_execute_admin_command(admin_queue,
>> +(struct ena_admin_aq_entry *),
>> +sizeof(cmd),
>> +(struct ena_admin_acq_entry *),
>> +sizeof(resp));
>> +
>> +if (unlikely(ret)) {
>> +ena_trc_err("Failed to set mtu %d. error: %d\n", mtu, ret);
>> +return -EINVAL;
> ena_com_execute_admin_command should return a proper status code.

ena_com_execute_admin_command already returns a proper status.
I'll remove the return value override.

>
> [...]
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h 
> b/drivers/net/ethernet/amazon/ena/ena_com.h
> new file mode 100644
> index 000..e49ba43
> --- /dev/null
> [...]
> +static inline void ena_com_update_intr_reg(struct ena_eth_io_intr_reg 
> *intr_reg,
> +u32 rx_delay_interval,
> +u32 tx_delay_interval,
> +bool unmask)
> +{
> + intr_reg->intr_control = 

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Netanel Belgazal]

On 06/18/2016 12:11 AM, Francois Romieu wrote:
> Netanel Belgazal  :
> [...]
>
> More stuff below.
>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
>> b/drivers/net/ethernet/amazon/ena/ena_com.c
>> new file mode 100644
>> index 000..357e10f
>> --- /dev/null
>> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> [...]
>> +static int ena_get_dev_stats(struct ena_com_dev *ena_dev,
>> + struct ena_admin_aq_get_stats_cmd *get_cmd,
>> + struct ena_admin_acq_get_stats_resp *get_resp,
> At first sight it should be possible avoid one pointer argument with:
>
> struct ena_something {
>   struct ena_admin_aq_get_stats_cmd cmd;
>   struct ena_admin_acq_get_stats_resp resp;
>   
> };
Ack
>
> [...]
>> +int ena_com_get_dev_extended_stats(struct ena_com_dev *ena_dev, char *buff,
>> +   u32 len)
> Where is it used ?
It is currently unused. I'll remove it from this patch and add it in a separate 
once we'll support it.
>
>> +{
>> +int ret = 0;
>> +struct ena_admin_aq_get_stats_cmd get_cmd;
>> +struct ena_admin_acq_get_stats_resp get_resp;
>> +void *virt_addr;
>   int rc = -ENOMEM;
>   char *str;
>
>> +dma_addr_t phys_addr;
>> +
>> +virt_addr = dma_alloc_coherent(ena_dev->dmadev,
>> +   len,
>> +   _addr,
>> +   GFP_KERNEL | __GFP_ZERO);
>> +if (!virt_addr) {
>> +ret = -ENOMEM;
>> +goto done;
>> +}
>> +memset(_cmd, 0x0, sizeof(get_cmd));
>> +ret = ena_com_mem_addr_set(ena_dev,
>> +   _cmd.u.control_buffer.address,
>> +   phys_addr);
>> +if (unlikely(ret)) {
>> +ena_trc_err("memory address set failed\n");
>> +return ret;
>> +}
>> +get_cmd.u.control_buffer.length = len;
>> +
>> +get_cmd.device_id = ena_dev->stats_func;
>> +get_cmd.queue_idx = ena_dev->stats_queue;
>> +
>> +ret = ena_get_dev_stats(ena_dev, _cmd, _resp,
>> +ENA_ADMIN_GET_STATS_TYPE_EXTENDED);
>> +if (ret < 0)
>> +goto free_ext_stats_mem;
>> +
>> +ret = snprintf(buff, len, "%s", (char *)virt_addr);
> So ENA_ADMIN_GET_STATS_TYPE_EXTENDED fills a buffer with bytes in the
> [0; 127] range, right ?

ENA_ADMIN_GET_STATS_TYPE_EXTENDED fills a buffer with 4K bytes.

>
>> +
>> +free_ext_stats_mem:
>> +dma_free_coherent(ena_dev->dmadev, len, virt_addr, phys_addr);
>> +done:
>> +return ret;
>> +}
>> +
>> +int ena_com_set_dev_mtu(struct ena_com_dev *ena_dev, int mtu)
>> +{
>> +struct ena_com_admin_queue *admin_queue;
>> +struct ena_admin_set_feat_cmd cmd;
>> +struct ena_admin_set_feat_resp resp;
>> +int ret = 0;
> Should be -ENODEV or left uninitialized.
I'll initialize it to -ENODEV
>
>> +
>> +if (unlikely(!ena_dev)) {
>> +ena_trc_err("%s : ena_dev is NULL\n", __func__);
>> +return -ENODEV;
>> +}
> Does it mean that ena_com_dev may go away while the net_device is in use ?
ena_com_dev can't go away while net_device is alive.
I'll remove this check.
>
>> +
>> +if (!ena_com_check_supported_feature_id(ena_dev, ENA_ADMIN_MTU)) {
>> +ena_trc_info("Feature %d isn't supported\n", ENA_ADMIN_MTU);
>> +return -EPERM;
>> +}
>   rc = ena_com_check_supported_feature_id(...
>   if (rc < 0) {
>   ena_trc_info(...
>   goto out;
>   }
>> +
>> +memset(, 0x0, sizeof(cmd));
>> +admin_queue = _dev->admin_queue;
>> +
>> +cmd.aq_common_descriptor.opcode = ENA_ADMIN_SET_FEATURE;
>> +cmd.aq_common_descriptor.flags = 0;
>> +cmd.feat_common.feature_id = ENA_ADMIN_MTU;
>> +cmd.u.mtu.mtu = mtu;
>> +
>> +ret = ena_com_execute_admin_command(admin_queue,
>> +(struct ena_admin_aq_entry *),
>> +sizeof(cmd),
>> +(struct ena_admin_acq_entry *),
>> +sizeof(resp));
>> +
>> +if (unlikely(ret)) {
>> +ena_trc_err("Failed to set mtu %d. error: %d\n", mtu, ret);
>> +return -EINVAL;
> ena_com_execute_admin_command should return a proper status code.

ena_com_execute_admin_command already returns a proper status.
I'll remove the return value override.

>
> [...]
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h 
> b/drivers/net/ethernet/amazon/ena/ena_com.h
> new file mode 100644
> index 000..e49ba43
> --- /dev/null
> [...]
> +static inline void ena_com_update_intr_reg(struct ena_eth_io_intr_reg 
> *intr_reg,
> +u32 rx_delay_interval,
> +u32 tx_delay_interval,
> +bool unmask)
> +{
> + intr_reg->intr_control = 0;
> + 

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Netanel Belgazal]

On 06/17/2016 04:09 AM, Matt Wilson wrote:
> On Thu, Jun 16, 2016 at 10:46:23PM +0200, Francois Romieu wrote:
>> Netanel Belgazal  :
>> [...]
>>
>> Very limited review below.
> I'll comment on the documentation (since I edited it heavily) but will
> leave some the other parts for Netanel to answer.
>
>>> diff --git a/Documentation/networking/ena.txt 
>>> b/Documentation/networking/ena.txt
>>> new file mode 100644
>>> index 000..528544f
>>> --- /dev/null
>>> +++ b/Documentation/networking/ena.txt
>>> @@ -0,0 +1,330 @@
>>> +Linux kernel driver for Elastic Network Adapter (ENA) family:
>>> +=
>>> +
>>> +Overview:
>>> +=
>>> +The ENA driver provides a modern Ethernet device interface optimized
>>> +for high performance and low CPU overhead.
>> Marketing boilerplate. How much of it will still be true in 6 months ?
>> 6 years ?
>>
>> Hint: stay technical. If in doubt, make it boring. :o)
> Hopefully it will still be true for quite a while. But you make a good
> point, the overview here should stick with the technical details.
>
>> [...]
>>> +ENA devices allow high speed and low overhead Ethernet traffic
>>> +processing by providing a dedicated Tx/Rx queue pair per host CPU, a
>>> +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive
>>> +interrupt moderation, and CPU cacheline optimized data placement.
>> How many queues exactly ?
> It's negotiated with the adapter via the admin queue, and it will vary
> based on the capabilities of a particular adapter. See:
>
> rc = ena_com_get_feature(ena_dev, _resp,
>  ENA_ADMIN_MAX_QUEUES_NUM);
>
> in ena_com.c
>
>> [...]
>>> +Memory Allocations:
>>> +===
>>> +DMA Coherent buffers are allocated for the f`ollowing DMA rings:
>>> +- Tx submission ring (For regular mode; for LLQ mode it is allocated
>>> +  using kzalloc)
>>> +- Tx completion ring
>>> +- Rx submission ring
>>> +- Rx completion ring
>>> +- Admin submission ring
>>> +- Admin completion ring
>>> +- AENQ ring
>> Useless documentation (implementation detail).
>>
>> Nobody will maintain it when the driver changes. Please remove.
> Ack.
>
>> [...]
>>> +MTU:
>>> +
>>> +The driver supports an arbitrarily large MTU with a maximum that is
>>> +negotiated with the device. The driver configures MTU using the
>>> +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU
>>> +via ifconfig(8) and ip(8).
>> Please avoid advertising legacy ifconfig.
> Ack.
>
>> [...]
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h 
>>> b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>>> new file mode 100644
>>> index 000..8516e5e
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> [...]
>>> +/* admin commands opcodes */
>>> +enum ena_admin_aq_opcode {
>>> +   /* create submission queue */
>>> +   ENA_ADMIN_CREATE_SQ = 1,
>>> +
>>> +   /* destroy submission queue */
>>> +   ENA_ADMIN_DESTROY_SQ = 2,
>>> +
>>> +   /* create completion queue */
>>> +   ENA_ADMIN_CREATE_CQ = 3,
>>> +
>>> +   /* destroy completion queue */
>>> +   ENA_ADMIN_DESTROY_CQ = 4,
>>> +
>>> +   /* get capabilities of particular feature */
>>> +   ENA_ADMIN_GET_FEATURE = 8,
>>> +
>>> +   /* get capabilities of particular feature */
>>> +   ENA_ADMIN_SET_FEATURE = 9,
>>> +
>>> +   /* get statistics */
>>> +   ENA_ADMIN_GET_STATS = 11,
>>> +};
>> The documentation above simply duplicates the member names -> useless
>> visual payload.
>>
>> Please replace space with tab before "=" to line things up
> I'll leave this to Netanel. A good amount of this comes from a unified
> documentation / struct / register access generation script, and it
> might need some massaging.
Ack. (Removed comments and replace space with tab)
>
>> [...]
>>> +/* Basic Statistics Command. */
>>> +struct ena_admin_basic_stats {
>>> +   /* word 0 :  */
>>> +   u32 tx_bytes_low;
>>> +
>>> +   /* word 1 :  */
>>> +   u32 tx_bytes_high;
>>> +
>>> +   /* word 2 :  */
>>> +   u32 tx_pkts_low;
>>> +
>>> +   /* word 3 :  */
>>> +   u32 tx_pkts_high;
>>> +
>>> +   /* word 4 :  */
>>> +   u32 rx_bytes_low;
>>> +
>>> +   /* word 5 :  */
>>> +   u32 rx_bytes_high;
>>> +
>>> +   /* word 6 :  */
>>> +   u32 rx_pkts_low;
>>> +
>>> +   /* word 7 :  */
>>> +   u32 rx_pkts_high;
>>> +
>>> +   /* word 8 :  */
>>> +   u32 rx_drops_low;
>>> +
>>> +   /* word 9 :  */
>>> +   u32 rx_drops_high;
>>> +};
>> struct zorg {
>>  u32 ...;
>>  u32 ...;
>>  u32 ...;
>>  u32 ...;
>>
>>  u32 ...;
>>  u32 ...;
>>  u32 ...;
>>  u32 ...;
>>
>>  u32 ...;
>>  u32 ...;
>>  u32 ...;
>> };
>>
>> -> No comment needed to figure the offset.
>>Comment removed => no need to check if offset starts at word 0 or word 1.
>>
>> [...]
Ack
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
>>> b/drivers/net/ethernet/amazon/ena/ena_com.c
>>> new file mode 100644
>>> index 

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Netanel Belgazal]

On 06/17/2016 04:09 AM, Matt Wilson wrote:
> On Thu, Jun 16, 2016 at 10:46:23PM +0200, Francois Romieu wrote:
>> Netanel Belgazal  :
>> [...]
>>
>> Very limited review below.
> I'll comment on the documentation (since I edited it heavily) but will
> leave some the other parts for Netanel to answer.
>
>>> diff --git a/Documentation/networking/ena.txt 
>>> b/Documentation/networking/ena.txt
>>> new file mode 100644
>>> index 000..528544f
>>> --- /dev/null
>>> +++ b/Documentation/networking/ena.txt
>>> @@ -0,0 +1,330 @@
>>> +Linux kernel driver for Elastic Network Adapter (ENA) family:
>>> +=
>>> +
>>> +Overview:
>>> +=
>>> +The ENA driver provides a modern Ethernet device interface optimized
>>> +for high performance and low CPU overhead.
>> Marketing boilerplate. How much of it will still be true in 6 months ?
>> 6 years ?
>>
>> Hint: stay technical. If in doubt, make it boring. :o)
> Hopefully it will still be true for quite a while. But you make a good
> point, the overview here should stick with the technical details.
>
>> [...]
>>> +ENA devices allow high speed and low overhead Ethernet traffic
>>> +processing by providing a dedicated Tx/Rx queue pair per host CPU, a
>>> +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive
>>> +interrupt moderation, and CPU cacheline optimized data placement.
>> How many queues exactly ?
> It's negotiated with the adapter via the admin queue, and it will vary
> based on the capabilities of a particular adapter. See:
>
> rc = ena_com_get_feature(ena_dev, _resp,
>  ENA_ADMIN_MAX_QUEUES_NUM);
>
> in ena_com.c
>
>> [...]
>>> +Memory Allocations:
>>> +===
>>> +DMA Coherent buffers are allocated for the f`ollowing DMA rings:
>>> +- Tx submission ring (For regular mode; for LLQ mode it is allocated
>>> +  using kzalloc)
>>> +- Tx completion ring
>>> +- Rx submission ring
>>> +- Rx completion ring
>>> +- Admin submission ring
>>> +- Admin completion ring
>>> +- AENQ ring
>> Useless documentation (implementation detail).
>>
>> Nobody will maintain it when the driver changes. Please remove.
> Ack.
>
>> [...]
>>> +MTU:
>>> +
>>> +The driver supports an arbitrarily large MTU with a maximum that is
>>> +negotiated with the device. The driver configures MTU using the
>>> +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU
>>> +via ifconfig(8) and ip(8).
>> Please avoid advertising legacy ifconfig.
> Ack.
>
>> [...]
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h 
>>> b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>>> new file mode 100644
>>> index 000..8516e5e
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> [...]
>>> +/* admin commands opcodes */
>>> +enum ena_admin_aq_opcode {
>>> +   /* create submission queue */
>>> +   ENA_ADMIN_CREATE_SQ = 1,
>>> +
>>> +   /* destroy submission queue */
>>> +   ENA_ADMIN_DESTROY_SQ = 2,
>>> +
>>> +   /* create completion queue */
>>> +   ENA_ADMIN_CREATE_CQ = 3,
>>> +
>>> +   /* destroy completion queue */
>>> +   ENA_ADMIN_DESTROY_CQ = 4,
>>> +
>>> +   /* get capabilities of particular feature */
>>> +   ENA_ADMIN_GET_FEATURE = 8,
>>> +
>>> +   /* get capabilities of particular feature */
>>> +   ENA_ADMIN_SET_FEATURE = 9,
>>> +
>>> +   /* get statistics */
>>> +   ENA_ADMIN_GET_STATS = 11,
>>> +};
>> The documentation above simply duplicates the member names -> useless
>> visual payload.
>>
>> Please replace space with tab before "=" to line things up
> I'll leave this to Netanel. A good amount of this comes from a unified
> documentation / struct / register access generation script, and it
> might need some massaging.
Ack. (Removed comments and replace space with tab)
>
>> [...]
>>> +/* Basic Statistics Command. */
>>> +struct ena_admin_basic_stats {
>>> +   /* word 0 :  */
>>> +   u32 tx_bytes_low;
>>> +
>>> +   /* word 1 :  */
>>> +   u32 tx_bytes_high;
>>> +
>>> +   /* word 2 :  */
>>> +   u32 tx_pkts_low;
>>> +
>>> +   /* word 3 :  */
>>> +   u32 tx_pkts_high;
>>> +
>>> +   /* word 4 :  */
>>> +   u32 rx_bytes_low;
>>> +
>>> +   /* word 5 :  */
>>> +   u32 rx_bytes_high;
>>> +
>>> +   /* word 6 :  */
>>> +   u32 rx_pkts_low;
>>> +
>>> +   /* word 7 :  */
>>> +   u32 rx_pkts_high;
>>> +
>>> +   /* word 8 :  */
>>> +   u32 rx_drops_low;
>>> +
>>> +   /* word 9 :  */
>>> +   u32 rx_drops_high;
>>> +};
>> struct zorg {
>>  u32 ...;
>>  u32 ...;
>>  u32 ...;
>>  u32 ...;
>>
>>  u32 ...;
>>  u32 ...;
>>  u32 ...;
>>  u32 ...;
>>
>>  u32 ...;
>>  u32 ...;
>>  u32 ...;
>> };
>>
>> -> No comment needed to figure the offset.
>>Comment removed => no need to check if offset starts at word 0 or word 1.
>>
>> [...]
Ack
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
>>> b/drivers/net/ethernet/amazon/ena/ena_com.c
>>> new file mode 100644
>>> index 000..357e10f
>>> --- 

RE: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Rosen, Rami]

Hi all, 

A very limited review below.

+
+   /* get capabilities of particular feature */
+   ENA_ADMIN_GET_FEATURE = 8,

Instead /* get capabilities  SHOULD BE:  /* set capabilities .
+
+   /* get capabilities of particular feature */
+   ENA_ADMIN_SET_FEATURE = 9,
+
..


+int ena_com_set_hash_ctrl(struct ena_com_dev *ena_dev)
+{
+   struct ena_com_admin_queue *admin_queue = _dev->admin_queue;
...

You set ret=-EINVAL, but you do not use this ret as you immediately return 0 in 
the next line, which is the end of the method. Either return ret or return 
-EINVAL.
+   if (unlikely(ret)) {
+   ena_trc_err("Failed to set hash input. error: %d\n", ret);
+   ret = -EINVAL;
+   }
+
+   return 0;
+}
+



+
+/* ena_com_set_mmio_read_mode - Enable/disable the mmio reg read mechanism
+ * @ena_dev: ENA communication layer struct


Instead realess_supported: SHOULD BE: readless_supported

+ * @realess_supported: readless mode (enable/disable)
+ */
+void ena_com_set_mmio_read_mode(struct ena_com_dev *ena_dev,
+   bool readless_supported);
+



+
+/* ena_com_create_io_queue - Create io queue.
+ * @ena_dev: ENA communication layer struct

Instead ena_com_create_io_ctx SHOULD BE: @ena_com_create_io_ctx

+ * ena_com_create_io_ctx - create context structure
+ *
+ * Create the submission and the completion queues.
+ *
+ * @return - 0 on success, negative value on failure.
+ */
+int ena_com_create_io_queue(struct ena_com_dev *ena_dev,
+   struct ena_com_create_io_ctx *ctx);
+




+/* ena_com_admin_destroy - Destroy IO queue with the queue id - qid.
+ * @ena_dev: ENA communication layer struct

Missing: @qid

+ */
+void ena_com_destroy_io_queue(struct ena_com_dev *ena_dev, u16 qid);
+

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Rosen, Rami]

Hi all, 

A very limited review below.

+
+   /* get capabilities of particular feature */
+   ENA_ADMIN_GET_FEATURE = 8,

Instead /* get capabilities  SHOULD BE:  /* set capabilities .
+
+   /* get capabilities of particular feature */
+   ENA_ADMIN_SET_FEATURE = 9,
+
..


+int ena_com_set_hash_ctrl(struct ena_com_dev *ena_dev)
+{
+   struct ena_com_admin_queue *admin_queue = _dev->admin_queue;
...

You set ret=-EINVAL, but you do not use this ret as you immediately return 0 in 
the next line, which is the end of the method. Either return ret or return 
-EINVAL.
+   if (unlikely(ret)) {
+   ena_trc_err("Failed to set hash input. error: %d\n", ret);
+   ret = -EINVAL;
+   }
+
+   return 0;
+}
+



+
+/* ena_com_set_mmio_read_mode - Enable/disable the mmio reg read mechanism
+ * @ena_dev: ENA communication layer struct


Instead realess_supported: SHOULD BE: readless_supported

+ * @realess_supported: readless mode (enable/disable)
+ */
+void ena_com_set_mmio_read_mode(struct ena_com_dev *ena_dev,
+   bool readless_supported);
+



+
+/* ena_com_create_io_queue - Create io queue.
+ * @ena_dev: ENA communication layer struct

Instead ena_com_create_io_ctx SHOULD BE: @ena_com_create_io_ctx

+ * ena_com_create_io_ctx - create context structure
+ *
+ * Create the submission and the completion queues.
+ *
+ * @return - 0 on success, negative value on failure.
+ */
+int ena_com_create_io_queue(struct ena_com_dev *ena_dev,
+   struct ena_com_create_io_ctx *ctx);
+




+/* ena_com_admin_destroy - Destroy IO queue with the queue id - qid.
+ * @ena_dev: ENA communication layer struct

Missing: @qid

+ */
+void ena_com_destroy_io_queue(struct ena_com_dev *ena_dev, u16 qid);
+

Regards,
Rami Rosen
Intel Corporation

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Thu, Jun 16, 2016 at 07:06:52PM +0100, Ben Hutchings wrote:
> On Thu, 2016-06-16 at 10:55 -0700, Benjamin Poirier wrote:
> > On 2016/06/13 11:46, Netanel Belgazal wrote:
> [...]
> > > +static ssize_t ena_show_small_copy_len(struct device *dev,
> > > +    struct device_attribute *attr, char *buf)
> > > +{
> > > + struct ena_adapter *adapter = dev_get_drvdata(dev);
> > > +
> > > + return sprintf(buf, "%d\n", adapter->small_copy_len);
> > > +}
> > > +
> > > +static DEVICE_ATTR(small_copy_len, S_IRUGO | S_IWUSR, 
> > > ena_show_small_copy_len,
> > > +    ena_store_small_copy_len);
> > 
> > This is what many other drivers call (rx_)copybreak. Perhaps it's time
> > to add it to ethtool as well?
> [...]
> 
> There is the 'tunable' ethtool API for random parameters like
> rx_copybreak.  The ethtool utility doesn't support it yet, though.

So I started implementing proper support for this in ethtool in a
generic way (retrieving the string set, parsing generically, etc), but
it seems that the implementation isn't quite as complete as one would
like. You said [1] that the kernel returns the type of each tunable,
but I don't see this implemented anywhere.

The string set also includes "Unspec", which is of dubious value.

I think that something tunable specific is going to need to be added
apart from ETHTOOL_GSTRINGS to provide ethtool the needed type
information.

--msw

[1] http://thread.gmane.org/gmane.linux.network/367058/focus=376701

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Thu, Jun 16, 2016 at 07:06:52PM +0100, Ben Hutchings wrote:
> On Thu, 2016-06-16 at 10:55 -0700, Benjamin Poirier wrote:
> > On 2016/06/13 11:46, Netanel Belgazal wrote:
> [...]
> > > +static ssize_t ena_show_small_copy_len(struct device *dev,
> > > +    struct device_attribute *attr, char *buf)
> > > +{
> > > + struct ena_adapter *adapter = dev_get_drvdata(dev);
> > > +
> > > + return sprintf(buf, "%d\n", adapter->small_copy_len);
> > > +}
> > > +
> > > +static DEVICE_ATTR(small_copy_len, S_IRUGO | S_IWUSR, 
> > > ena_show_small_copy_len,
> > > +    ena_store_small_copy_len);
> > 
> > This is what many other drivers call (rx_)copybreak. Perhaps it's time
> > to add it to ethtool as well?
> [...]
> 
> There is the 'tunable' ethtool API for random parameters like
> rx_copybreak.  The ethtool utility doesn't support it yet, though.

So I started implementing proper support for this in ethtool in a
generic way (retrieving the string set, parsing generically, etc), but
it seems that the implementation isn't quite as complete as one would
like. You said [1] that the kernel returns the type of each tunable,
but I don't see this implemented anywhere.

The string set also includes "Unspec", which is of dubious value.

I think that something tunable specific is going to need to be added
apart from ETHTOOL_GSTRINGS to provide ethtool the needed type
information.

--msw

[1] http://thread.gmane.org/gmane.linux.network/367058/focus=376701

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Francois Romieu]

Netanel Belgazal  :
[...]

More stuff below.

> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
> b/drivers/net/ethernet/amazon/ena/ena_com.c
> new file mode 100644
> index 000..357e10f
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
[...]
> +static int ena_get_dev_stats(struct ena_com_dev *ena_dev,
> +  struct ena_admin_aq_get_stats_cmd *get_cmd,
> +  struct ena_admin_acq_get_stats_resp *get_resp,

At first sight it should be possible avoid one pointer argument with:

struct ena_something {
struct ena_admin_aq_get_stats_cmd cmd;
struct ena_admin_acq_get_stats_resp resp;

};

[...]
> +int ena_com_get_dev_extended_stats(struct ena_com_dev *ena_dev, char *buff,
> +u32 len)

Where is it used ?

> +{
> + int ret = 0;
> + struct ena_admin_aq_get_stats_cmd get_cmd;
> + struct ena_admin_acq_get_stats_resp get_resp;
> + void *virt_addr;

int rc = -ENOMEM;
char *str;

> + dma_addr_t phys_addr;
> +
> + virt_addr = dma_alloc_coherent(ena_dev->dmadev,
> +len,
> +_addr,
> +GFP_KERNEL | __GFP_ZERO);
> + if (!virt_addr) {
> + ret = -ENOMEM;
> + goto done;
> + }
> + memset(_cmd, 0x0, sizeof(get_cmd));
> + ret = ena_com_mem_addr_set(ena_dev,
> +_cmd.u.control_buffer.address,
> +phys_addr);
> + if (unlikely(ret)) {
> + ena_trc_err("memory address set failed\n");
> + return ret;
> + }
> + get_cmd.u.control_buffer.length = len;
> +
> + get_cmd.device_id = ena_dev->stats_func;
> + get_cmd.queue_idx = ena_dev->stats_queue;
> +
> + ret = ena_get_dev_stats(ena_dev, _cmd, _resp,
> + ENA_ADMIN_GET_STATS_TYPE_EXTENDED);
> + if (ret < 0)
> + goto free_ext_stats_mem;
> +
> + ret = snprintf(buff, len, "%s", (char *)virt_addr);

So ENA_ADMIN_GET_STATS_TYPE_EXTENDED fills a buffer with bytes in the
[0; 127] range, right ?

> +
> +free_ext_stats_mem:
> + dma_free_coherent(ena_dev->dmadev, len, virt_addr, phys_addr);
> +done:
> + return ret;
> +}
> +
> +int ena_com_set_dev_mtu(struct ena_com_dev *ena_dev, int mtu)
> +{
> + struct ena_com_admin_queue *admin_queue;
> + struct ena_admin_set_feat_cmd cmd;
> + struct ena_admin_set_feat_resp resp;
> + int ret = 0;

Should be -ENODEV or left uninitialized.

> +
> + if (unlikely(!ena_dev)) {
> + ena_trc_err("%s : ena_dev is NULL\n", __func__);
> + return -ENODEV;
> + }

Does it mean that ena_com_dev may go away while the net_device is in use ?

> +
> + if (!ena_com_check_supported_feature_id(ena_dev, ENA_ADMIN_MTU)) {
> + ena_trc_info("Feature %d isn't supported\n", ENA_ADMIN_MTU);
> + return -EPERM;
> + }

rc = ena_com_check_supported_feature_id(...
if (rc < 0) {
ena_trc_info(...
goto out;
}
> +
> + memset(, 0x0, sizeof(cmd));
> + admin_queue = _dev->admin_queue;
> +
> + cmd.aq_common_descriptor.opcode = ENA_ADMIN_SET_FEATURE;
> + cmd.aq_common_descriptor.flags = 0;
> + cmd.feat_common.feature_id = ENA_ADMIN_MTU;
> + cmd.u.mtu.mtu = mtu;
> +
> + ret = ena_com_execute_admin_command(admin_queue,
> + (struct ena_admin_aq_entry *),
> + sizeof(cmd),
> + (struct ena_admin_acq_entry *),
> + sizeof(resp));
> +
> + if (unlikely(ret)) {
> + ena_trc_err("Failed to set mtu %d. error: %d\n", mtu, ret);
> + return -EINVAL;

ena_com_execute_admin_command should return a proper status code.

[...]
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h 
b/drivers/net/ethernet/amazon/ena/ena_com.h
new file mode 100644
index 000..e49ba43
--- /dev/null
[...]
+static inline void ena_com_update_intr_reg(struct ena_eth_io_intr_reg 
*intr_reg,
+  u32 rx_delay_interval,
+  u32 tx_delay_interval,
+  bool unmask)
+{
+   intr_reg->intr_control = 0;
+   intr_reg->intr_control |= rx_delay_interval &
+   ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
+
+   intr_reg->intr_control |=
+   (tx_delay_interval << ENA_ETH_IO_INTR_REG_TX_INTR_DELAY_SHIFT)
+   & ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
  ^^ TX ?

Extra: you should not return NETDEV_TX_BUSY in the xmit handler while
queueing is still enabled. Please drop packet and return NETDEV_TX_OK.

-- 
Ueimor

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Francois Romieu]

Netanel Belgazal  :
[...]

More stuff below.

> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
> b/drivers/net/ethernet/amazon/ena/ena_com.c
> new file mode 100644
> index 000..357e10f
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
[...]
> +static int ena_get_dev_stats(struct ena_com_dev *ena_dev,
> +  struct ena_admin_aq_get_stats_cmd *get_cmd,
> +  struct ena_admin_acq_get_stats_resp *get_resp,

At first sight it should be possible avoid one pointer argument with:

struct ena_something {
struct ena_admin_aq_get_stats_cmd cmd;
struct ena_admin_acq_get_stats_resp resp;

};

[...]
> +int ena_com_get_dev_extended_stats(struct ena_com_dev *ena_dev, char *buff,
> +u32 len)

Where is it used ?

> +{
> + int ret = 0;
> + struct ena_admin_aq_get_stats_cmd get_cmd;
> + struct ena_admin_acq_get_stats_resp get_resp;
> + void *virt_addr;

int rc = -ENOMEM;
char *str;

> + dma_addr_t phys_addr;
> +
> + virt_addr = dma_alloc_coherent(ena_dev->dmadev,
> +len,
> +_addr,
> +GFP_KERNEL | __GFP_ZERO);
> + if (!virt_addr) {
> + ret = -ENOMEM;
> + goto done;
> + }
> + memset(_cmd, 0x0, sizeof(get_cmd));
> + ret = ena_com_mem_addr_set(ena_dev,
> +_cmd.u.control_buffer.address,
> +phys_addr);
> + if (unlikely(ret)) {
> + ena_trc_err("memory address set failed\n");
> + return ret;
> + }
> + get_cmd.u.control_buffer.length = len;
> +
> + get_cmd.device_id = ena_dev->stats_func;
> + get_cmd.queue_idx = ena_dev->stats_queue;
> +
> + ret = ena_get_dev_stats(ena_dev, _cmd, _resp,
> + ENA_ADMIN_GET_STATS_TYPE_EXTENDED);
> + if (ret < 0)
> + goto free_ext_stats_mem;
> +
> + ret = snprintf(buff, len, "%s", (char *)virt_addr);

So ENA_ADMIN_GET_STATS_TYPE_EXTENDED fills a buffer with bytes in the
[0; 127] range, right ?

> +
> +free_ext_stats_mem:
> + dma_free_coherent(ena_dev->dmadev, len, virt_addr, phys_addr);
> +done:
> + return ret;
> +}
> +
> +int ena_com_set_dev_mtu(struct ena_com_dev *ena_dev, int mtu)
> +{
> + struct ena_com_admin_queue *admin_queue;
> + struct ena_admin_set_feat_cmd cmd;
> + struct ena_admin_set_feat_resp resp;
> + int ret = 0;

Should be -ENODEV or left uninitialized.

> +
> + if (unlikely(!ena_dev)) {
> + ena_trc_err("%s : ena_dev is NULL\n", __func__);
> + return -ENODEV;
> + }

Does it mean that ena_com_dev may go away while the net_device is in use ?

> +
> + if (!ena_com_check_supported_feature_id(ena_dev, ENA_ADMIN_MTU)) {
> + ena_trc_info("Feature %d isn't supported\n", ENA_ADMIN_MTU);
> + return -EPERM;
> + }

rc = ena_com_check_supported_feature_id(...
if (rc < 0) {
ena_trc_info(...
goto out;
}
> +
> + memset(, 0x0, sizeof(cmd));
> + admin_queue = _dev->admin_queue;
> +
> + cmd.aq_common_descriptor.opcode = ENA_ADMIN_SET_FEATURE;
> + cmd.aq_common_descriptor.flags = 0;
> + cmd.feat_common.feature_id = ENA_ADMIN_MTU;
> + cmd.u.mtu.mtu = mtu;
> +
> + ret = ena_com_execute_admin_command(admin_queue,
> + (struct ena_admin_aq_entry *),
> + sizeof(cmd),
> + (struct ena_admin_acq_entry *),
> + sizeof(resp));
> +
> + if (unlikely(ret)) {
> + ena_trc_err("Failed to set mtu %d. error: %d\n", mtu, ret);
> + return -EINVAL;

ena_com_execute_admin_command should return a proper status code.

[...]
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h 
b/drivers/net/ethernet/amazon/ena/ena_com.h
new file mode 100644
index 000..e49ba43
--- /dev/null
[...]
+static inline void ena_com_update_intr_reg(struct ena_eth_io_intr_reg 
*intr_reg,
+  u32 rx_delay_interval,
+  u32 tx_delay_interval,
+  bool unmask)
+{
+   intr_reg->intr_control = 0;
+   intr_reg->intr_control |= rx_delay_interval &
+   ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
+
+   intr_reg->intr_control |=
+   (tx_delay_interval << ENA_ETH_IO_INTR_REG_TX_INTR_DELAY_SHIFT)
+   & ENA_ETH_IO_INTR_REG_RX_INTR_DELAY_MASK;
  ^^ TX ?

Extra: you should not return NETDEV_TX_BUSY in the xmit handler while
queueing is still enabled. Please drop packet and return NETDEV_TX_OK.

-- 
Ueimor

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Thu, Jun 16, 2016 at 10:46:23PM +0200, Francois Romieu wrote:
> Netanel Belgazal  :
> [...]
> 
> Very limited review below.

I'll comment on the documentation (since I edited it heavily) but will
leave some the other parts for Netanel to answer.

> > diff --git a/Documentation/networking/ena.txt 
> > b/Documentation/networking/ena.txt
> > new file mode 100644
> > index 000..528544f
> > --- /dev/null
> > +++ b/Documentation/networking/ena.txt
> > @@ -0,0 +1,330 @@
> > +Linux kernel driver for Elastic Network Adapter (ENA) family:
> > +=
> > +
> > +Overview:
> > +=
> > +The ENA driver provides a modern Ethernet device interface optimized
> > +for high performance and low CPU overhead.
> 
> Marketing boilerplate. How much of it will still be true in 6 months ?
> 6 years ?
> 
> Hint: stay technical. If in doubt, make it boring. :o)

Hopefully it will still be true for quite a while. But you make a good
point, the overview here should stick with the technical details.

> [...]
> > +ENA devices allow high speed and low overhead Ethernet traffic
> > +processing by providing a dedicated Tx/Rx queue pair per host CPU, a
> > +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive
> > +interrupt moderation, and CPU cacheline optimized data placement.
> 
> How many queues exactly ?

It's negotiated with the adapter via the admin queue, and it will vary
based on the capabilities of a particular adapter. See:

rc = ena_com_get_feature(ena_dev, _resp,
 ENA_ADMIN_MAX_QUEUES_NUM);

in ena_com.c

> [...]
> > +Memory Allocations:
> > +===
> > +DMA Coherent buffers are allocated for the f`ollowing DMA rings:
> > +- Tx submission ring (For regular mode; for LLQ mode it is allocated
> > +  using kzalloc)
> > +- Tx completion ring
> > +- Rx submission ring
> > +- Rx completion ring
> > +- Admin submission ring
> > +- Admin completion ring
> > +- AENQ ring
> 
> Useless documentation (implementation detail).
> 
> Nobody will maintain it when the driver changes. Please remove.

Ack.

> [...]
> > +MTU:
> > +
> > +The driver supports an arbitrarily large MTU with a maximum that is
> > +negotiated with the device. The driver configures MTU using the
> > +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU
> > +via ifconfig(8) and ip(8).
> 
> Please avoid advertising legacy ifconfig.

Ack.

> [...]
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h 
> > b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> > new file mode 100644
> > index 000..8516e5e
> > --- /dev/null
> > +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> [...]
> > +/* admin commands opcodes */
> > +enum ena_admin_aq_opcode {
> > +   /* create submission queue */
> > +   ENA_ADMIN_CREATE_SQ = 1,
> > +
> > +   /* destroy submission queue */
> > +   ENA_ADMIN_DESTROY_SQ = 2,
> > +
> > +   /* create completion queue */
> > +   ENA_ADMIN_CREATE_CQ = 3,
> > +
> > +   /* destroy completion queue */
> > +   ENA_ADMIN_DESTROY_CQ = 4,
> > +
> > +   /* get capabilities of particular feature */
> > +   ENA_ADMIN_GET_FEATURE = 8,
> > +
> > +   /* get capabilities of particular feature */
> > +   ENA_ADMIN_SET_FEATURE = 9,
> > +
> > +   /* get statistics */
> > +   ENA_ADMIN_GET_STATS = 11,
> > +};
> 
> The documentation above simply duplicates the member names -> useless
> visual payload.
> 
> Please replace space with tab before "=" to line things up.

I'll leave this to Netanel. A good amount of this comes from a unified
documentation / struct / register access generation script, and it
might need some massaging.

> [...]
> > +/* Basic Statistics Command. */
> > +struct ena_admin_basic_stats {
> > +   /* word 0 :  */
> > +   u32 tx_bytes_low;
> > +
> > +   /* word 1 :  */
> > +   u32 tx_bytes_high;
> > +
> > +   /* word 2 :  */
> > +   u32 tx_pkts_low;
> > +
> > +   /* word 3 :  */
> > +   u32 tx_pkts_high;
> > +
> > +   /* word 4 :  */
> > +   u32 rx_bytes_low;
> > +
> > +   /* word 5 :  */
> > +   u32 rx_bytes_high;
> > +
> > +   /* word 6 :  */
> > +   u32 rx_pkts_low;
> > +
> > +   /* word 7 :  */
> > +   u32 rx_pkts_high;
> > +
> > +   /* word 8 :  */
> > +   u32 rx_drops_low;
> > +
> > +   /* word 9 :  */
> > +   u32 rx_drops_high;
> > +};
> 
> struct zorg {
>   u32 ...;
>   u32 ...;
>   u32 ...;
>   u32 ...;
> 
>   u32 ...;
>   u32 ...;
>   u32 ...;
>   u32 ...;
> 
>   u32 ...;
>   u32 ...;
>   u32 ...;
> };
> 
> -> No comment needed to figure the offset.
>Comment removed => no need to check if offset starts at word 0 or word 1.
> 
> [...]
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
> > b/drivers/net/ethernet/amazon/ena/ena_com.c
> > new file mode 100644
> > index 000..357e10f
> > --- /dev/null
> > +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> [...]
> > +static inline int 

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Thu, Jun 16, 2016 at 10:46:23PM +0200, Francois Romieu wrote:
> Netanel Belgazal  :
> [...]
> 
> Very limited review below.

I'll comment on the documentation (since I edited it heavily) but will
leave some the other parts for Netanel to answer.

> > diff --git a/Documentation/networking/ena.txt 
> > b/Documentation/networking/ena.txt
> > new file mode 100644
> > index 000..528544f
> > --- /dev/null
> > +++ b/Documentation/networking/ena.txt
> > @@ -0,0 +1,330 @@
> > +Linux kernel driver for Elastic Network Adapter (ENA) family:
> > +=
> > +
> > +Overview:
> > +=
> > +The ENA driver provides a modern Ethernet device interface optimized
> > +for high performance and low CPU overhead.
> 
> Marketing boilerplate. How much of it will still be true in 6 months ?
> 6 years ?
> 
> Hint: stay technical. If in doubt, make it boring. :o)

Hopefully it will still be true for quite a while. But you make a good
point, the overview here should stick with the technical details.

> [...]
> > +ENA devices allow high speed and low overhead Ethernet traffic
> > +processing by providing a dedicated Tx/Rx queue pair per host CPU, a
> > +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive
> > +interrupt moderation, and CPU cacheline optimized data placement.
> 
> How many queues exactly ?

It's negotiated with the adapter via the admin queue, and it will vary
based on the capabilities of a particular adapter. See:

rc = ena_com_get_feature(ena_dev, _resp,
 ENA_ADMIN_MAX_QUEUES_NUM);

in ena_com.c

> [...]
> > +Memory Allocations:
> > +===
> > +DMA Coherent buffers are allocated for the f`ollowing DMA rings:
> > +- Tx submission ring (For regular mode; for LLQ mode it is allocated
> > +  using kzalloc)
> > +- Tx completion ring
> > +- Rx submission ring
> > +- Rx completion ring
> > +- Admin submission ring
> > +- Admin completion ring
> > +- AENQ ring
> 
> Useless documentation (implementation detail).
> 
> Nobody will maintain it when the driver changes. Please remove.

Ack.

> [...]
> > +MTU:
> > +
> > +The driver supports an arbitrarily large MTU with a maximum that is
> > +negotiated with the device. The driver configures MTU using the
> > +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU
> > +via ifconfig(8) and ip(8).
> 
> Please avoid advertising legacy ifconfig.

Ack.

> [...]
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h 
> > b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> > new file mode 100644
> > index 000..8516e5e
> > --- /dev/null
> > +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> [...]
> > +/* admin commands opcodes */
> > +enum ena_admin_aq_opcode {
> > +   /* create submission queue */
> > +   ENA_ADMIN_CREATE_SQ = 1,
> > +
> > +   /* destroy submission queue */
> > +   ENA_ADMIN_DESTROY_SQ = 2,
> > +
> > +   /* create completion queue */
> > +   ENA_ADMIN_CREATE_CQ = 3,
> > +
> > +   /* destroy completion queue */
> > +   ENA_ADMIN_DESTROY_CQ = 4,
> > +
> > +   /* get capabilities of particular feature */
> > +   ENA_ADMIN_GET_FEATURE = 8,
> > +
> > +   /* get capabilities of particular feature */
> > +   ENA_ADMIN_SET_FEATURE = 9,
> > +
> > +   /* get statistics */
> > +   ENA_ADMIN_GET_STATS = 11,
> > +};
> 
> The documentation above simply duplicates the member names -> useless
> visual payload.
> 
> Please replace space with tab before "=" to line things up.

I'll leave this to Netanel. A good amount of this comes from a unified
documentation / struct / register access generation script, and it
might need some massaging.

> [...]
> > +/* Basic Statistics Command. */
> > +struct ena_admin_basic_stats {
> > +   /* word 0 :  */
> > +   u32 tx_bytes_low;
> > +
> > +   /* word 1 :  */
> > +   u32 tx_bytes_high;
> > +
> > +   /* word 2 :  */
> > +   u32 tx_pkts_low;
> > +
> > +   /* word 3 :  */
> > +   u32 tx_pkts_high;
> > +
> > +   /* word 4 :  */
> > +   u32 rx_bytes_low;
> > +
> > +   /* word 5 :  */
> > +   u32 rx_bytes_high;
> > +
> > +   /* word 6 :  */
> > +   u32 rx_pkts_low;
> > +
> > +   /* word 7 :  */
> > +   u32 rx_pkts_high;
> > +
> > +   /* word 8 :  */
> > +   u32 rx_drops_low;
> > +
> > +   /* word 9 :  */
> > +   u32 rx_drops_high;
> > +};
> 
> struct zorg {
>   u32 ...;
>   u32 ...;
>   u32 ...;
>   u32 ...;
> 
>   u32 ...;
>   u32 ...;
>   u32 ...;
>   u32 ...;
> 
>   u32 ...;
>   u32 ...;
>   u32 ...;
> };
> 
> -> No comment needed to figure the offset.
>Comment removed => no need to check if offset starts at word 0 or word 1.
> 
> [...]
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
> > b/drivers/net/ethernet/amazon/ena/ena_com.c
> > new file mode 100644
> > index 000..357e10f
> > --- /dev/null
> > +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> [...]
> > +static inline int ena_com_mem_addr_set(struct ena_com_dev 

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Francois Romieu]

Netanel Belgazal  :
[...]

Very limited review below.

> diff --git a/Documentation/networking/ena.txt 
> b/Documentation/networking/ena.txt
> new file mode 100644
> index 000..528544f
> --- /dev/null
> +++ b/Documentation/networking/ena.txt
> @@ -0,0 +1,330 @@
> +Linux kernel driver for Elastic Network Adapter (ENA) family:
> +=
> +
> +Overview:
> +=
> +The ENA driver provides a modern Ethernet device interface optimized
> +for high performance and low CPU overhead.

Marketing boilerplate. How much of it will still be true in 6 months ?
6 years ?

Hint: stay technical. If in doubt, make it boring. :o)

[...]
> +ENA devices allow high speed and low overhead Ethernet traffic
> +processing by providing a dedicated Tx/Rx queue pair per host CPU, a
> +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive
> +interrupt moderation, and CPU cacheline optimized data placement.

How many queues exactly ?

[...]
> +Memory Allocations:
> +===
> +DMA Coherent buffers are allocated for the following DMA rings:
> +- Tx submission ring (For regular mode; for LLQ mode it is allocated
> +  using kzalloc)
> +- Tx completion ring
> +- Rx submission ring
> +- Rx completion ring
> +- Admin submission ring
> +- Admin completion ring
> +- AENQ ring

Useless documentation (implementation detail).

Nobody will maintain it when the driver changes. Please remove.

[...]
> +MTU:
> +
> +The driver supports an arbitrarily large MTU with a maximum that is
> +negotiated with the device. The driver configures MTU using the
> +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU
> +via ifconfig(8) and ip(8).

Please avoid advertising legacy ifconfig.

[...]
> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h 
> b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> new file mode 100644
> index 000..8516e5e
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
[...]
> +/* admin commands opcodes */
> +enum ena_admin_aq_opcode {
> + /* create submission queue */
> + ENA_ADMIN_CREATE_SQ = 1,
> +
> + /* destroy submission queue */
> + ENA_ADMIN_DESTROY_SQ = 2,
> +
> + /* create completion queue */
> + ENA_ADMIN_CREATE_CQ = 3,
> +
> + /* destroy completion queue */
> + ENA_ADMIN_DESTROY_CQ = 4,
> +
> + /* get capabilities of particular feature */
> + ENA_ADMIN_GET_FEATURE = 8,
> +
> + /* get capabilities of particular feature */
> + ENA_ADMIN_SET_FEATURE = 9,
> +
> + /* get statistics */
> + ENA_ADMIN_GET_STATS = 11,
> +};

The documentation above simply duplicates the member names -> useless
visual payload.

Please replace space with tab before "=" to line things up.

[...]
> +/* Basic Statistics Command. */
> +struct ena_admin_basic_stats {
> + /* word 0 :  */
> + u32 tx_bytes_low;
> +
> + /* word 1 :  */
> + u32 tx_bytes_high;
> +
> + /* word 2 :  */
> + u32 tx_pkts_low;
> +
> + /* word 3 :  */
> + u32 tx_pkts_high;
> +
> + /* word 4 :  */
> + u32 rx_bytes_low;
> +
> + /* word 5 :  */
> + u32 rx_bytes_high;
> +
> + /* word 6 :  */
> + u32 rx_pkts_low;
> +
> + /* word 7 :  */
> + u32 rx_pkts_high;
> +
> + /* word 8 :  */
> + u32 rx_drops_low;
> +
> + /* word 9 :  */
> + u32 rx_drops_high;
> +};

struct zorg {
u32 ...;
u32 ...;
u32 ...;
u32 ...;

u32 ...;
u32 ...;
u32 ...;
u32 ...;

u32 ...;
u32 ...;
u32 ...;
};

-> No comment needed to figure the offset.
   Comment removed => no need to check if offset starts at word 0 or word 1.

[...]
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
> b/drivers/net/ethernet/amazon/ena/ena_com.c
> new file mode 100644
> index 000..357e10f
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
[...]
> +static inline int ena_com_mem_addr_set(struct ena_com_dev *ena_dev,
> +struct ena_common_mem_addr *ena_addr,
> +dma_addr_t addr)
> +{
> + if ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 0)) != addr) {
> + ena_trc_err("dma address has more bits that the device 
> supports\n");
> + return -EINVAL;
> + }
> +
> + ena_addr->mem_addr_low = (u32)addr;
> + ena_addr->mem_addr_high =
> + ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 32)) >> 32);

No need to "... & GENMASK" given the test above.

> +
> + return 0;
> +}
> +
> +static int ena_com_admin_init_sq(struct ena_com_admin_queue *queue)
> +{
> + queue->sq.entries =
> + dma_alloc_coherent(queue->q_dmadev,
> +ADMIN_SQ_SIZE(queue->q_depth),
> +>sq.dma_addr,
> +GFP_KERNEL | __GFP_ZERO);

1. Use 

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Francois Romieu]

Netanel Belgazal  :
[...]

Very limited review below.

> diff --git a/Documentation/networking/ena.txt 
> b/Documentation/networking/ena.txt
> new file mode 100644
> index 000..528544f
> --- /dev/null
> +++ b/Documentation/networking/ena.txt
> @@ -0,0 +1,330 @@
> +Linux kernel driver for Elastic Network Adapter (ENA) family:
> +=
> +
> +Overview:
> +=
> +The ENA driver provides a modern Ethernet device interface optimized
> +for high performance and low CPU overhead.

Marketing boilerplate. How much of it will still be true in 6 months ?
6 years ?

Hint: stay technical. If in doubt, make it boring. :o)

[...]
> +ENA devices allow high speed and low overhead Ethernet traffic
> +processing by providing a dedicated Tx/Rx queue pair per host CPU, a
> +dedicated MSI-X interrupt vector per Tx/Rx queue pair, adaptive
> +interrupt moderation, and CPU cacheline optimized data placement.

How many queues exactly ?

[...]
> +Memory Allocations:
> +===
> +DMA Coherent buffers are allocated for the following DMA rings:
> +- Tx submission ring (For regular mode; for LLQ mode it is allocated
> +  using kzalloc)
> +- Tx completion ring
> +- Rx submission ring
> +- Rx completion ring
> +- Admin submission ring
> +- Admin completion ring
> +- AENQ ring

Useless documentation (implementation detail).

Nobody will maintain it when the driver changes. Please remove.

[...]
> +MTU:
> +
> +The driver supports an arbitrarily large MTU with a maximum that is
> +negotiated with the device. The driver configures MTU using the
> +SetFeature command (ENA_ADMIN_MTU property). The user can change MTU
> +via ifconfig(8) and ip(8).

Please avoid advertising legacy ifconfig.

[...]
> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h 
> b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> new file mode 100644
> index 000..8516e5e
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
[...]
> +/* admin commands opcodes */
> +enum ena_admin_aq_opcode {
> + /* create submission queue */
> + ENA_ADMIN_CREATE_SQ = 1,
> +
> + /* destroy submission queue */
> + ENA_ADMIN_DESTROY_SQ = 2,
> +
> + /* create completion queue */
> + ENA_ADMIN_CREATE_CQ = 3,
> +
> + /* destroy completion queue */
> + ENA_ADMIN_DESTROY_CQ = 4,
> +
> + /* get capabilities of particular feature */
> + ENA_ADMIN_GET_FEATURE = 8,
> +
> + /* get capabilities of particular feature */
> + ENA_ADMIN_SET_FEATURE = 9,
> +
> + /* get statistics */
> + ENA_ADMIN_GET_STATS = 11,
> +};

The documentation above simply duplicates the member names -> useless
visual payload.

Please replace space with tab before "=" to line things up.

[...]
> +/* Basic Statistics Command. */
> +struct ena_admin_basic_stats {
> + /* word 0 :  */
> + u32 tx_bytes_low;
> +
> + /* word 1 :  */
> + u32 tx_bytes_high;
> +
> + /* word 2 :  */
> + u32 tx_pkts_low;
> +
> + /* word 3 :  */
> + u32 tx_pkts_high;
> +
> + /* word 4 :  */
> + u32 rx_bytes_low;
> +
> + /* word 5 :  */
> + u32 rx_bytes_high;
> +
> + /* word 6 :  */
> + u32 rx_pkts_low;
> +
> + /* word 7 :  */
> + u32 rx_pkts_high;
> +
> + /* word 8 :  */
> + u32 rx_drops_low;
> +
> + /* word 9 :  */
> + u32 rx_drops_high;
> +};

struct zorg {
u32 ...;
u32 ...;
u32 ...;
u32 ...;

u32 ...;
u32 ...;
u32 ...;
u32 ...;

u32 ...;
u32 ...;
u32 ...;
};

-> No comment needed to figure the offset.
   Comment removed => no need to check if offset starts at word 0 or word 1.

[...]
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
> b/drivers/net/ethernet/amazon/ena/ena_com.c
> new file mode 100644
> index 000..357e10f
> --- /dev/null
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
[...]
> +static inline int ena_com_mem_addr_set(struct ena_com_dev *ena_dev,
> +struct ena_common_mem_addr *ena_addr,
> +dma_addr_t addr)
> +{
> + if ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 0)) != addr) {
> + ena_trc_err("dma address has more bits that the device 
> supports\n");
> + return -EINVAL;
> + }
> +
> + ena_addr->mem_addr_low = (u32)addr;
> + ena_addr->mem_addr_high =
> + ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 32)) >> 32);

No need to "... & GENMASK" given the test above.

> +
> + return 0;
> +}
> +
> +static int ena_com_admin_init_sq(struct ena_com_admin_queue *queue)
> +{
> + queue->sq.entries =
> + dma_alloc_coherent(queue->q_dmadev,
> +ADMIN_SQ_SIZE(queue->q_depth),
> +>sq.dma_addr,
> +GFP_KERNEL | __GFP_ZERO);

1. Use dma_zalloc_coherent().

2. Style

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Ben Hutchings]

On Thu, 2016-06-16 at 10:55 -0700, Benjamin Poirier wrote:
> On 2016/06/13 11:46, Netanel Belgazal wrote:
[...]
> > +static ssize_t ena_show_small_copy_len(struct device *dev,
> > +      struct device_attribute *attr, char *buf)
> > +{
> > +   struct ena_adapter *adapter = dev_get_drvdata(dev);
> > +
> > +   return sprintf(buf, "%d\n", adapter->small_copy_len);
> > +}
> > +
> > +static DEVICE_ATTR(small_copy_len, S_IRUGO | S_IWUSR, 
> > ena_show_small_copy_len,
> > +      ena_store_small_copy_len);
> 
> This is what many other drivers call (rx_)copybreak. Perhaps it's time
> to add it to ethtool as well?
[...]

There is the 'tunable' ethtool API for random parameters like
rx_copybreak.  The ethtool utility doesn't support it yet, though.

Ben.

-- 
Ben Hutchings
Life is what happens to you while you're busy making other plans.
   - John
Lennon


signature.asc
Description: This is a digitally signed message part

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Ben Hutchings]

On Thu, 2016-06-16 at 10:55 -0700, Benjamin Poirier wrote:
> On 2016/06/13 11:46, Netanel Belgazal wrote:
[...]
> > +static ssize_t ena_show_small_copy_len(struct device *dev,
> > +      struct device_attribute *attr, char *buf)
> > +{
> > +   struct ena_adapter *adapter = dev_get_drvdata(dev);
> > +
> > +   return sprintf(buf, "%d\n", adapter->small_copy_len);
> > +}
> > +
> > +static DEVICE_ATTR(small_copy_len, S_IRUGO | S_IWUSR, 
> > ena_show_small_copy_len,
> > +      ena_store_small_copy_len);
> 
> This is what many other drivers call (rx_)copybreak. Perhaps it's time
> to add it to ethtool as well?
[...]

There is the 'tunable' ethtool API for random parameters like
rx_copybreak.  The ethtool utility doesn't support it yet, though.

Ben.

-- 
Ben Hutchings
Life is what happens to you while you're busy making other plans.
   - John
Lennon


signature.asc
Description: This is a digitally signed message part

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Benjamin Poirier]

On 2016/06/13 11:46, Netanel Belgazal wrote:
[...]
> +
> +static int ena_set_coalesce(struct net_device *net_dev,
> + struct ethtool_coalesce *coalesce)
> +{
> + struct ena_adapter *adapter = netdev_priv(net_dev);
> + struct ena_com_dev *ena_dev = adapter->ena_dev;
> + int rc;
> +
> + if (!ena_com_interrupt_moderation_supported(ena_dev)) {
> + /* the devie doesn't support interrupt moderation */
> + return -EOPNOTSUPP;
> + }
> +
> + /* Note, adaptive coalescing settings are updated through sysfs */

I believe the usual approach is to use ethtool for these kinds of
settings, extending the interface if necessary.

> + if (coalesce->rx_coalesce_usecs_irq ||
> + coalesce->rx_max_coalesced_frames ||
> + coalesce->rx_max_coalesced_frames_irq ||
> + coalesce->tx_coalesce_usecs_irq ||
> + coalesce->tx_max_coalesced_frames ||
> + coalesce->tx_max_coalesced_frames_irq ||
> + coalesce->stats_block_coalesce_usecs ||
> + coalesce->use_adaptive_tx_coalesce ||
> + coalesce->pkt_rate_low ||
> + coalesce->rx_coalesce_usecs_low ||
> + coalesce->rx_max_coalesced_frames_low ||
> + coalesce->tx_coalesce_usecs_low ||
> + coalesce->tx_max_coalesced_frames_low ||
> + coalesce->pkt_rate_high ||
> + coalesce->rx_coalesce_usecs_high ||
> + coalesce->rx_max_coalesced_frames_high ||
> + coalesce->tx_coalesce_usecs_high ||
> + coalesce->tx_max_coalesced_frames_high ||
> + coalesce->rate_sample_interval)
> + return -EINVAL;
> +

[...]

> +
> +static ssize_t ena_store_small_copy_len(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct ena_adapter *adapter = dev_get_drvdata(dev);
> + unsigned long small_copy_len;
> + struct ena_ring *rx_ring;
> + int err, i;
> +
> + err = kstrtoul(buf, 10, _copy_len);
> + if (err < 0)
> + return err;
> +
> + err = ena_validate_small_copy_len(adapter, small_copy_len);
> + if (err)
> + return err;
> +
> + rtnl_lock();
> + adapter->small_copy_len = small_copy_len;
> +
> + for (i = 0; i < adapter->num_queues; i++) {
> + rx_ring = >rx_ring[i];
> + rx_ring->rx_small_copy_len = small_copy_len;
> + }
> + rtnl_unlock();
> +
> + return len;
> +}
> +
> +static ssize_t ena_show_small_copy_len(struct device *dev,
> +struct device_attribute *attr, char *buf)
> +{
> + struct ena_adapter *adapter = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d\n", adapter->small_copy_len);
> +}
> +
> +static DEVICE_ATTR(small_copy_len, S_IRUGO | S_IWUSR, 
> ena_show_small_copy_len,
> +ena_store_small_copy_len);

This is what many other drivers call (rx_)copybreak. Perhaps it's time
to add it to ethtool as well?

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Benjamin Poirier]

On 2016/06/13 11:46, Netanel Belgazal wrote:
[...]
> +
> +static int ena_set_coalesce(struct net_device *net_dev,
> + struct ethtool_coalesce *coalesce)
> +{
> + struct ena_adapter *adapter = netdev_priv(net_dev);
> + struct ena_com_dev *ena_dev = adapter->ena_dev;
> + int rc;
> +
> + if (!ena_com_interrupt_moderation_supported(ena_dev)) {
> + /* the devie doesn't support interrupt moderation */
> + return -EOPNOTSUPP;
> + }
> +
> + /* Note, adaptive coalescing settings are updated through sysfs */

I believe the usual approach is to use ethtool for these kinds of
settings, extending the interface if necessary.

> + if (coalesce->rx_coalesce_usecs_irq ||
> + coalesce->rx_max_coalesced_frames ||
> + coalesce->rx_max_coalesced_frames_irq ||
> + coalesce->tx_coalesce_usecs_irq ||
> + coalesce->tx_max_coalesced_frames ||
> + coalesce->tx_max_coalesced_frames_irq ||
> + coalesce->stats_block_coalesce_usecs ||
> + coalesce->use_adaptive_tx_coalesce ||
> + coalesce->pkt_rate_low ||
> + coalesce->rx_coalesce_usecs_low ||
> + coalesce->rx_max_coalesced_frames_low ||
> + coalesce->tx_coalesce_usecs_low ||
> + coalesce->tx_max_coalesced_frames_low ||
> + coalesce->pkt_rate_high ||
> + coalesce->rx_coalesce_usecs_high ||
> + coalesce->rx_max_coalesced_frames_high ||
> + coalesce->tx_coalesce_usecs_high ||
> + coalesce->tx_max_coalesced_frames_high ||
> + coalesce->rate_sample_interval)
> + return -EINVAL;
> +

[...]

> +
> +static ssize_t ena_store_small_copy_len(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct ena_adapter *adapter = dev_get_drvdata(dev);
> + unsigned long small_copy_len;
> + struct ena_ring *rx_ring;
> + int err, i;
> +
> + err = kstrtoul(buf, 10, _copy_len);
> + if (err < 0)
> + return err;
> +
> + err = ena_validate_small_copy_len(adapter, small_copy_len);
> + if (err)
> + return err;
> +
> + rtnl_lock();
> + adapter->small_copy_len = small_copy_len;
> +
> + for (i = 0; i < adapter->num_queues; i++) {
> + rx_ring = >rx_ring[i];
> + rx_ring->rx_small_copy_len = small_copy_len;
> + }
> + rtnl_unlock();
> +
> + return len;
> +}
> +
> +static ssize_t ena_show_small_copy_len(struct device *dev,
> +struct device_attribute *attr, char *buf)
> +{
> + struct ena_adapter *adapter = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d\n", adapter->small_copy_len);
> +}
> +
> +static DEVICE_ATTR(small_copy_len, S_IRUGO | S_IWUSR, 
> ena_show_small_copy_len,
> +ena_store_small_copy_len);

This is what many other drivers call (rx_)copybreak. Perhaps it's time
to add it to ethtool as well?

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Wed, Jun 15, 2016 at 09:32:35PM +0300, Netanel Belgazal wrote:
> I removed HAVE_NETDEV_NAPI_LIST and fixed the ena print macro.
> I'll wait a bit to see if there are additional comment before I'll send the
> v2 patch.

Also, please don't top post (reply inline) and send emails as plain
text only. Have a look at [1] for how to set up a mailer like mutt
with Gmail.

--msw

[1] https://www.kernel.org/doc/Documentation/email-clients.txt

> On 15 June 2016 at 21:22, Matt Wilson  wrote:
> 
> > On Wed, Jun 15, 2016 at 11:07:17AM -0700, Matt Wilson wrote:
> > >
> > > You also had a few typos in the email addresses on the Cc: line that
> > > I've corrected (d...@avemeloft.net -> da...@davemeloft.net,
> >
> > Argh, and of course I typo'ed the correction. -> da...@davemloft.net.
> >
> > --msw
> >

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Wed, Jun 15, 2016 at 09:32:35PM +0300, Netanel Belgazal wrote:
> I removed HAVE_NETDEV_NAPI_LIST and fixed the ena print macro.
> I'll wait a bit to see if there are additional comment before I'll send the
> v2 patch.

Also, please don't top post (reply inline) and send emails as plain
text only. Have a look at [1] for how to set up a mailer like mutt
with Gmail.

--msw

[1] https://www.kernel.org/doc/Documentation/email-clients.txt

> On 15 June 2016 at 21:22, Matt Wilson  wrote:
> 
> > On Wed, Jun 15, 2016 at 11:07:17AM -0700, Matt Wilson wrote:
> > >
> > > You also had a few typos in the email addresses on the Cc: line that
> > > I've corrected (d...@avemeloft.net -> da...@davemeloft.net,
> >
> > Argh, and of course I typo'ed the correction. -> da...@davemloft.net.
> >
> > --msw
> >

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Wed, Jun 15, 2016 at 11:07:17AM -0700, Matt Wilson wrote:
> 
> You also had a few typos in the email addresses on the Cc: line that
> I've corrected (d...@avemeloft.net -> da...@davemeloft.net,

Argh, and of course I typo'ed the correction. -> da...@davemloft.net.

--msw

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Wed, Jun 15, 2016 at 11:07:17AM -0700, Matt Wilson wrote:
> 
> You also had a few typos in the email addresses on the Cc: line that
> I've corrected (d...@avemeloft.net -> da...@davemeloft.net,

Argh, and of course I typo'ed the correction. -> da...@davemloft.net.

--msw

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Mon, Jun 13, 2016 at 11:46:13AM +0300, Netanel Belgazal wrote:
> This is a driver for the forthcoming ENA family of networking devices.
> 
> Signed-off-by: Netanel Belgazal 

[...]

> +
> +struct ena_napi {
> + struct napi_struct napi cacheline_aligned;
> + struct ena_ring *tx_ring;
> + struct ena_ring *rx_ring;
> +#ifndef HAVE_NETDEV_NAPI_LIST
> + struct net_device poll_dev;
> +#endif /* HAVE_NETDEV_NAPI_LIST */
> + u32 qid;
> +};

Netanel,

Sorry I missed this when I read through the patch earlier this week
(though I pointed it out before...). This "#ifndef
HAVE_NETDEV_NAPI_LIST" bit needs to be removed for the in-tree
version.

You also had a few typos in the email addresses on the Cc: line that
I've corrected (d...@avemeloft.net -> da...@davemeloft.net,
aligo...@amazon.com -> aligu...@amazon.com). Please make sure to
correct in your v2 patch.

--msw

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Mon, Jun 13, 2016 at 11:46:13AM +0300, Netanel Belgazal wrote:
> This is a driver for the forthcoming ENA family of networking devices.
> 
> Signed-off-by: Netanel Belgazal 

[...]

> +
> +struct ena_napi {
> + struct napi_struct napi cacheline_aligned;
> + struct ena_ring *tx_ring;
> + struct ena_ring *rx_ring;
> +#ifndef HAVE_NETDEV_NAPI_LIST
> + struct net_device poll_dev;
> +#endif /* HAVE_NETDEV_NAPI_LIST */
> + u32 qid;
> +};

Netanel,

Sorry I missed this when I read through the patch earlier this week
(though I pointed it out before...). This "#ifndef
HAVE_NETDEV_NAPI_LIST" bit needs to be removed for the in-tree
version.

You also had a few typos in the email addresses on the Cc: line that
I've corrected (d...@avemeloft.net -> da...@davemeloft.net,
aligo...@amazon.com -> aligu...@amazon.com). Please make sure to
correct in your v2 patch.

--msw

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Tue, Jun 14, 2016 at 11:27:09PM -0700, David Miller wrote:
> From: Matt Wilson 
> Date: Tue, 14 Jun 2016 23:23:36 -0700
> 
> > Point taken, though existing drivers (even fairly popular ones) also
> > aren't as clean as you might like. A quick look around...
> 
> Existing drivers do undesirable things, film at 11...
> 
> Yet are never a reason to accept such things in new drivers.

I generally agree with this philosophy.

> > Like many other network drivers, some of this is common code used for
> > non-Linux systems, and that's why there is some overlap with Linux
> > facilities.
> 
> Again, never an excuse for such things.

I suppose I was just happy to have the *majorly* objectionable parts
cleaned up...

> > Are there other things that jump out at you?
> 
> I review hundreds of patches a day, I invested what I was able to
> before moving on to other people's work.

I wasn't asking you to do more, only if you had anything else you
wanted to say before Netanel sends a v2. 

> Other developers must help review such a large driver submission, it
> can't all be on me.

And I'm certainly not saying it's all on you. I've been reviewing this
with the team for quite a while to get it in pretty reasonable (IMHO)
shape.

--msw

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Tue, Jun 14, 2016 at 11:27:09PM -0700, David Miller wrote:
> From: Matt Wilson 
> Date: Tue, 14 Jun 2016 23:23:36 -0700
> 
> > Point taken, though existing drivers (even fairly popular ones) also
> > aren't as clean as you might like. A quick look around...
> 
> Existing drivers do undesirable things, film at 11...
> 
> Yet are never a reason to accept such things in new drivers.

I generally agree with this philosophy.

> > Like many other network drivers, some of this is common code used for
> > non-Linux systems, and that's why there is some overlap with Linux
> > facilities.
> 
> Again, never an excuse for such things.

I suppose I was just happy to have the *majorly* objectionable parts
cleaned up...

> > Are there other things that jump out at you?
> 
> I review hundreds of patches a day, I invested what I was able to
> before moving on to other people's work.

I wasn't asking you to do more, only if you had anything else you
wanted to say before Netanel sends a v2. 

> Other developers must help review such a large driver submission, it
> can't all be on me.

And I'm certainly not saying it's all on you. I've been reviewing this
with the team for quite a while to get it in pretty reasonable (IMHO)
shape.

--msw

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[David Miller]

From: Matt Wilson 
Date: Tue, 14 Jun 2016 23:23:36 -0700

> Point taken, though existing drivers (even fairly popular ones) also
> aren't as clean as you might like. A quick look around...

Existing drivers do undesirable things, film at 11...

Yet are never a reason to accept such things in new drivers.

> Like many other network drivers, some of this is common code used for
> non-Linux systems, and that's why there is some overlap with Linux
> facilities.

Again, never an excuse for such things.

> Are there other things that jump out at you?

I review hundreds of patches a day, I invested what I was able to
before moving on to other people's work.

Other developers must help review such a large driver submission, it
can't all be on me.

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[David Miller]

From: Matt Wilson 
Date: Tue, 14 Jun 2016 23:23:36 -0700

> Point taken, though existing drivers (even fairly popular ones) also
> aren't as clean as you might like. A quick look around...

Existing drivers do undesirable things, film at 11...

Yet are never a reason to accept such things in new drivers.

> Like many other network drivers, some of this is common code used for
> non-Linux systems, and that's why there is some overlap with Linux
> facilities.

Again, never an excuse for such things.

> Are there other things that jump out at you?

I review hundreds of patches a day, I invested what I was able to
before moving on to other people's work.

Other developers must help review such a large driver submission, it
can't all be on me.

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Tue, Jun 14, 2016 at 10:25:16PM -0700, David Miller wrote:
> From: Netanel Belgazal 
> Date: Mon, 13 Jun 2016 11:46:13 +0300
> 
> > +#define ena_trc_dbg(format, arg...) \
> > +   pr_debug("[ENA_COM: %s] " format, __func__, ##arg)
> > +#define ena_trc_info(format, arg...) \
> > +   pr_info("[ENA_COM: %s] " format, __func__, ##arg)
> > +#define ena_trc_warn(format, arg...) \
> > +   pr_warn("[ENA_COM: %s] " format, __func__, ##arg)
> > +#define ena_trc_err(format, arg...) \
> > +   pr_err("[ENA_COM: %s] " format, __func__, ##arg)
> 
> These custom tracing macros are quite inappropriate.
> 
> We have the function tracer in the kernel when that is needed.  So spitting
> out __func__ all over the place is not something that should be found in
> drivers these days.

Point taken, though existing drivers (even fairly popular ones) also
aren't as clean as you might like. A quick look around...

msw@carbon:~/git/upstream/linux$ git grep -B1 '__func__' drivers/net/ethernet/ 
| grep -A1 '#define' 
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h-#define BNX2X_ERROR(fmt, ...)   
 \
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h:pr_err("[%s:%d]" fmt, 
__func__, __LINE__, ##__VA_ARGS__)
[...]
drivers/net/ethernet/intel/ixgb/ixgb_osdep.h:#define ENTER() pr_debug("%s\n", 
__func__);

Like many other network drivers, some of this is common code used for
non-Linux systems, and that's why there is some overlap with Linux
facilities. For example, here's the common ENA parts as it's situated
in DPDK as a PMD:

   http://dpdk.org/browse/dpdk/tree/drivers/net/ena/base/ena_com.c

When you compare to the DPDK version you can see that the common code
has already been contextualized for Linux in this patch in
anticipation of this type of feedback. (e.g., ENA_SPINLOCK_LOCK() ->
spin_lock_irqsave(), etc., as that would obviously never fly).

The Linux-specific bits (ena_netdev.c, ena_ethtool.c, etc.) don't make
use of any of the overlapping functionality needed for the common
code.

> And one can modify pr_fmt do make pr_debug et al. have whatever prefix
> one wants.

Yup, that's an easy improvement.

> I suspect there will be several rounds of review to weed out things
> like this.  You can preempt a lot of that by removing as much in your
> driver that the kernel has existing facilities for.

Are there other things that jump out at you? I felt like this was
pretty good for an initial submission in terms of striking a balance
between using a portable core while avoiding a lot of compatibility
shims.

--msw

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Tue, Jun 14, 2016 at 10:25:16PM -0700, David Miller wrote:
> From: Netanel Belgazal 
> Date: Mon, 13 Jun 2016 11:46:13 +0300
> 
> > +#define ena_trc_dbg(format, arg...) \
> > +   pr_debug("[ENA_COM: %s] " format, __func__, ##arg)
> > +#define ena_trc_info(format, arg...) \
> > +   pr_info("[ENA_COM: %s] " format, __func__, ##arg)
> > +#define ena_trc_warn(format, arg...) \
> > +   pr_warn("[ENA_COM: %s] " format, __func__, ##arg)
> > +#define ena_trc_err(format, arg...) \
> > +   pr_err("[ENA_COM: %s] " format, __func__, ##arg)
> 
> These custom tracing macros are quite inappropriate.
> 
> We have the function tracer in the kernel when that is needed.  So spitting
> out __func__ all over the place is not something that should be found in
> drivers these days.

Point taken, though existing drivers (even fairly popular ones) also
aren't as clean as you might like. A quick look around...

msw@carbon:~/git/upstream/linux$ git grep -B1 '__func__' drivers/net/ethernet/ 
| grep -A1 '#define' 
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h-#define BNX2X_ERROR(fmt, ...)   
 \
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h:pr_err("[%s:%d]" fmt, 
__func__, __LINE__, ##__VA_ARGS__)
[...]
drivers/net/ethernet/intel/ixgb/ixgb_osdep.h:#define ENTER() pr_debug("%s\n", 
__func__);

Like many other network drivers, some of this is common code used for
non-Linux systems, and that's why there is some overlap with Linux
facilities. For example, here's the common ENA parts as it's situated
in DPDK as a PMD:

   http://dpdk.org/browse/dpdk/tree/drivers/net/ena/base/ena_com.c

When you compare to the DPDK version you can see that the common code
has already been contextualized for Linux in this patch in
anticipation of this type of feedback. (e.g., ENA_SPINLOCK_LOCK() ->
spin_lock_irqsave(), etc., as that would obviously never fly).

The Linux-specific bits (ena_netdev.c, ena_ethtool.c, etc.) don't make
use of any of the overlapping functionality needed for the common
code.

> And one can modify pr_fmt do make pr_debug et al. have whatever prefix
> one wants.

Yup, that's an easy improvement.

> I suspect there will be several rounds of review to weed out things
> like this.  You can preempt a lot of that by removing as much in your
> driver that the kernel has existing facilities for.

Are there other things that jump out at you? I felt like this was
pretty good for an initial submission in terms of striking a balance
between using a portable core while avoiding a lot of compatibility
shims.

--msw

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[David Miller]

From: Netanel Belgazal 
Date: Mon, 13 Jun 2016 11:46:13 +0300

> +#define ena_trc_dbg(format, arg...) \
> + pr_debug("[ENA_COM: %s] " format, __func__, ##arg)
> +#define ena_trc_info(format, arg...) \
> + pr_info("[ENA_COM: %s] " format, __func__, ##arg)
> +#define ena_trc_warn(format, arg...) \
> + pr_warn("[ENA_COM: %s] " format, __func__, ##arg)
> +#define ena_trc_err(format, arg...) \
> + pr_err("[ENA_COM: %s] " format, __func__, ##arg)

These custom tracing macros are quite inappropriate.

We have the function tracer in the kernel when that is needed.  So spitting
out __func__ all over the place is not something that should be found in
drivers these days.

And one can modify pr_fmt do make pr_debug et al. have whatever prefix
one wants.

I suspect there will be several rounds of review to weed out things
like this.  You can preempt a lot of that by removing as much in your
driver that the kernel has existing facilities for.

Thanks.

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[David Miller]

From: Netanel Belgazal 
Date: Mon, 13 Jun 2016 11:46:13 +0300

> +#define ena_trc_dbg(format, arg...) \
> + pr_debug("[ENA_COM: %s] " format, __func__, ##arg)
> +#define ena_trc_info(format, arg...) \
> + pr_info("[ENA_COM: %s] " format, __func__, ##arg)
> +#define ena_trc_warn(format, arg...) \
> + pr_warn("[ENA_COM: %s] " format, __func__, ##arg)
> +#define ena_trc_err(format, arg...) \
> + pr_err("[ENA_COM: %s] " format, __func__, ##arg)

These custom tracing macros are quite inappropriate.

We have the function tracer in the kernel when that is needed.  So spitting
out __func__ all over the place is not something that should be found in
drivers these days.

And one can modify pr_fmt do make pr_debug et al. have whatever prefix
one wants.

I suspect there will be several rounds of review to weed out things
like this.  You can preempt a lot of that by removing as much in your
driver that the kernel has existing facilities for.

Thanks.

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Mon, Jun 13, 2016 at 11:46:13AM +0300, Netanel Belgazal wrote:
> This is a driver for the forthcoming ENA family of networking devices.

Reviewed-by: Matt Wilson 

> Signed-off-by: Netanel Belgazal 
> ---
>  Documentation/networking/00-INDEX |2 +
>  Documentation/networking/ena.txt  |  330 ++
>  MAINTAINERS   |9 +
>  drivers/net/ethernet/Kconfig  |1 +
>  drivers/net/ethernet/Makefile |1 +
>  drivers/net/ethernet/amazon/Kconfig   |   27 +
>  drivers/net/ethernet/amazon/Makefile  |5 +
>  drivers/net/ethernet/amazon/ena/Makefile  |9 +
>  drivers/net/ethernet/amazon/ena/ena_admin_defs.h  | 1246 
>  drivers/net/ethernet/amazon/ena/ena_com.c | 2768 +
>  drivers/net/ethernet/amazon/ena/ena_com.h | 1051 +++
>  drivers/net/ethernet/amazon/ena/ena_common_defs.h |   52 +
>  drivers/net/ethernet/amazon/ena/ena_eth_com.c |  508 
>  drivers/net/ethernet/amazon/ena/ena_eth_com.h |  160 +
>  drivers/net/ethernet/amazon/ena/ena_eth_io_defs.h |  460 +++
>  drivers/net/ethernet/amazon/ena/ena_ethtool.c |  836 +
>  drivers/net/ethernet/amazon/ena/ena_netdev.c  | 3362 
> +
>  drivers/net/ethernet/amazon/ena/ena_netdev.h  |  327 ++
>  drivers/net/ethernet/amazon/ena/ena_pci_id_tbl.h  |   67 +
>  drivers/net/ethernet/amazon/ena/ena_regs_defs.h   |  133 +
>  drivers/net/ethernet/amazon/ena/ena_sysfs.c   |  264 ++
>  drivers/net/ethernet/amazon/ena/ena_sysfs.h   |   55 +
>  22 files changed, 11673 insertions(+)
>  create mode 100644 Documentation/networking/ena.txt
>  create mode 100644 drivers/net/ethernet/amazon/Kconfig
>  create mode 100644 drivers/net/ethernet/amazon/Makefile
>  create mode 100644 drivers/net/ethernet/amazon/ena/Makefile
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_com.c
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_com.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_common_defs.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_eth_com.c
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_eth_com.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_eth_io_defs.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_ethtool.c
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_netdev.c
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_netdev.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_pci_id_tbl.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_regs_defs.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_sysfs.c
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_sysfs.h

[...]

--msw

Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Matt Wilson]

On Mon, Jun 13, 2016 at 11:46:13AM +0300, Netanel Belgazal wrote:
> This is a driver for the forthcoming ENA family of networking devices.

Reviewed-by: Matt Wilson 

> Signed-off-by: Netanel Belgazal 
> ---
>  Documentation/networking/00-INDEX |2 +
>  Documentation/networking/ena.txt  |  330 ++
>  MAINTAINERS   |9 +
>  drivers/net/ethernet/Kconfig  |1 +
>  drivers/net/ethernet/Makefile |1 +
>  drivers/net/ethernet/amazon/Kconfig   |   27 +
>  drivers/net/ethernet/amazon/Makefile  |5 +
>  drivers/net/ethernet/amazon/ena/Makefile  |9 +
>  drivers/net/ethernet/amazon/ena/ena_admin_defs.h  | 1246 
>  drivers/net/ethernet/amazon/ena/ena_com.c | 2768 +
>  drivers/net/ethernet/amazon/ena/ena_com.h | 1051 +++
>  drivers/net/ethernet/amazon/ena/ena_common_defs.h |   52 +
>  drivers/net/ethernet/amazon/ena/ena_eth_com.c |  508 
>  drivers/net/ethernet/amazon/ena/ena_eth_com.h |  160 +
>  drivers/net/ethernet/amazon/ena/ena_eth_io_defs.h |  460 +++
>  drivers/net/ethernet/amazon/ena/ena_ethtool.c |  836 +
>  drivers/net/ethernet/amazon/ena/ena_netdev.c  | 3362 
> +
>  drivers/net/ethernet/amazon/ena/ena_netdev.h  |  327 ++
>  drivers/net/ethernet/amazon/ena/ena_pci_id_tbl.h  |   67 +
>  drivers/net/ethernet/amazon/ena/ena_regs_defs.h   |  133 +
>  drivers/net/ethernet/amazon/ena/ena_sysfs.c   |  264 ++
>  drivers/net/ethernet/amazon/ena/ena_sysfs.h   |   55 +
>  22 files changed, 11673 insertions(+)
>  create mode 100644 Documentation/networking/ena.txt
>  create mode 100644 drivers/net/ethernet/amazon/Kconfig
>  create mode 100644 drivers/net/ethernet/amazon/Makefile
>  create mode 100644 drivers/net/ethernet/amazon/ena/Makefile
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_com.c
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_com.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_common_defs.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_eth_com.c
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_eth_com.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_eth_io_defs.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_ethtool.c
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_netdev.c
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_netdev.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_pci_id_tbl.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_regs_defs.h
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_sysfs.c
>  create mode 100644 drivers/net/ethernet/amazon/ena/ena_sysfs.h

[...]

--msw