Am 03.12.2020 um 15:38 schrieb Kiyanovski, Arthur: > > >> -----Original Message----- >> From: Heiner Kallweit <[email protected]> >> Sent: Wednesday, December 2, 2020 11:55 PM >> To: Kiyanovski, Arthur <[email protected]>; [email protected]; >> [email protected] >> Cc: Woodhouse, David <[email protected]>; Machulsky, Zorik >> <[email protected]>; Matushevsky, Alexander <[email protected]>; >> Bshara, Saeed <[email protected]>; Wilson, Matt <[email protected]>; >> Liguori, Anthony <[email protected]>; Bshara, Nafea >> <[email protected]>; Tzalik, Guy <[email protected]>; Belgazal, >> Netanel <[email protected]>; Saidi, Ali <[email protected]>; >> Herrenschmidt, Benjamin <[email protected]>; Dagan, Noam >> <[email protected]>; Agroskin, Shay <[email protected]>; Jubran, >> Samih <[email protected]> >> Subject: RE: [EXTERNAL] [PATCH V3 net-next 1/9] net: ena: use constant >> value for net_device allocation >> >> CAUTION: This email originated from outside of the organization. Do not click >> links or open attachments unless you can confirm the sender and know the >> content is safe. >> >> >> >> Am 02.12.2020 um 21:03 schrieb [email protected]: >>> From: Arthur Kiyanovski <[email protected]> >>> >>> The patch changes the maximum number of RX/TX queues it advertises to >>> the kernel (via alloc_etherdev_mq()) from a value received from the >>> device to a constant value which is the minimum between 128 and the >>> number of CPUs in the system. >>> >>> By allocating the net_device struct with a constant number of queues, >>> the driver is able to allocate it at a much earlier stage, before >>> calling any ena_com functions. This would allow to make all log prints >>> in ena_com to use netdev_* log functions instead or current pr_* ones. >>> >> >> Did you test this? Usually using netdev_* before the net_device is registered >> results in quite ugly messages. Therefore there's a number of patches doing >> the opposite, replacing netdev_* with dev_* before register_netdev(). See >> e.g. >> 22148df0d0bd ("r8169: don't use netif_info et al before net_device has been >> registered") > > Thanks for your comment. > Yes we did test it. > Please see the discussion which led to this patch in a previous thread here: > https://www.mail-archive.com/[email protected]/msg353590.html >
Ah, I see. After reading the mail thread your motivation is clear. You accept ugly messages when ena_com functions are called from probe() for the sake of better messages when the same ena_com functions are called later from other parts of the driver. Maybe an explanation of this tradeoff would have been good in the commit message (or a link to the mail thread). >>> Signed-off-by: Shay Agroskin <[email protected]> >>> Signed-off-by: Arthur Kiyanovski <[email protected]> >>> --- >>> drivers/net/ethernet/amazon/ena/ena_netdev.c | 46 >>> ++++++++++---------- >>> 1 file changed, 23 insertions(+), 23 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c >>> b/drivers/net/ethernet/amazon/ena/ena_netdev.c >>> index df1884d57d1a..985dea1870b5 100644 >>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c >>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c >>> @@ -29,6 +29,8 @@ MODULE_LICENSE("GPL"); >>> /* Time in jiffies before concluding the transmitter is hung. */ >>> #define TX_TIMEOUT (5 * HZ) >>> >>> +#define ENA_MAX_RINGS min_t(unsigned int, >> ENA_MAX_NUM_IO_QUEUES, >>> +num_possible_cpus()) >>> + >>> #define ENA_NAPI_BUDGET 64 >>> >>> #define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV | NETIF_MSG_PROBE >> | >>> NETIF_MSG_IFUP | \ @@ -4176,18 +4178,34 @@ static int >> ena_probe(struct >>> pci_dev *pdev, const struct pci_device_id *ent) >>> >>> ena_dev->dmadev = &pdev->dev; >>> >>> + netdev = alloc_etherdev_mq(sizeof(struct ena_adapter), >> ENA_MAX_RINGS); >>> + if (!netdev) { >>> + dev_err(&pdev->dev, "alloc_etherdev_mq failed\n"); >>> + rc = -ENOMEM; >>> + goto err_free_region; >>> + } >>> + >>> + SET_NETDEV_DEV(netdev, &pdev->dev); >>> + adapter = netdev_priv(netdev); >>> + adapter->ena_dev = ena_dev; >>> + adapter->netdev = netdev; >>> + adapter->pdev = pdev; >>> + adapter->msg_enable = netif_msg_init(debug, >> DEFAULT_MSG_ENABLE); >>> + >>> + pci_set_drvdata(pdev, adapter); >>> + >>> rc = ena_device_init(ena_dev, pdev, &get_feat_ctx, &wd_state); >>> if (rc) { >>> dev_err(&pdev->dev, "ENA device init failed\n"); >>> if (rc == -ETIME) >>> rc = -EPROBE_DEFER; >>> - goto err_free_region; >>> + goto err_netdev_destroy; >>> } >>> >>> rc = ena_map_llq_mem_bar(pdev, ena_dev, bars); >>> if (rc) { >>> dev_err(&pdev->dev, "ENA llq bar mapping failed\n"); >>> - goto err_free_ena_dev; >>> + goto err_device_destroy; >>> } >>> >>> calc_queue_ctx.ena_dev = ena_dev; @@ -4207,26 +4225,8 @@ static >>> int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent) >>> goto err_device_destroy; >>> } >>> >>> - /* dev zeroed in init_etherdev */ >>> - netdev = alloc_etherdev_mq(sizeof(struct ena_adapter), >> max_num_io_queues); >>> - if (!netdev) { >>> - dev_err(&pdev->dev, "alloc_etherdev_mq failed\n"); >>> - rc = -ENOMEM; >>> - goto err_device_destroy; >>> - } >>> - >>> - SET_NETDEV_DEV(netdev, &pdev->dev); >>> - >>> - adapter = netdev_priv(netdev); >>> - pci_set_drvdata(pdev, adapter); >>> - >>> - adapter->ena_dev = ena_dev; >>> - adapter->netdev = netdev; >>> - adapter->pdev = pdev; >>> - >>> ena_set_conf_feat_params(adapter, &get_feat_ctx); >>> >>> - adapter->msg_enable = netif_msg_init(debug, >> DEFAULT_MSG_ENABLE); >>> adapter->reset_reason = ENA_REGS_RESET_NORMAL; >>> >>> adapter->requested_tx_ring_size = calc_queue_ctx.tx_queue_size; >>> @@ -4257,7 +4257,7 @@ static int ena_probe(struct pci_dev *pdev, const >> struct pci_device_id *ent) >>> if (rc) { >>> dev_err(&pdev->dev, >>> "Failed to query interrupt moderation feature\n"); >>> - goto err_netdev_destroy; >>> + goto err_device_destroy; >>> } >>> ena_init_io_rings(adapter, >>> 0, >>> @@ -4335,11 +4335,11 @@ static int ena_probe(struct pci_dev *pdev, >> const struct pci_device_id *ent) >>> ena_disable_msix(adapter); >>> err_worker_destroy: >>> del_timer(&adapter->timer_service); >>> -err_netdev_destroy: >>> - free_netdev(netdev); >>> err_device_destroy: >>> ena_com_delete_host_info(ena_dev); >>> ena_com_admin_destroy(ena_dev); >>> +err_netdev_destroy: >>> + free_netdev(netdev); >>> err_free_region: >>> ena_release_bars(ena_dev, pdev); >>> err_free_ena_dev: >>> >
