Hi Michal,
Thanks for your comments.
On 5/31/19, 1:20 AM, "Michal Kubecek" <mkube...@suse.cz> wrote:
On Wed, May 29, 2019 at 12:49:55PM +0300, same...@amazon.com wrote:
> From: Arthur Kiyanovski <akiy...@amazon.com>
>
> This commit adds a mechanism for exposing different driver
> properties via ethtool's priv_flags.
>
> In this commit we:
>
> Add commands, structs and defines necessary for handling
> extra properties
>
> Add functions for:
> Allocation/destruction of a buffer for extra properties strings.
> Retreival of extra properties strings and flags from the network device.
>
> Handle the allocation of a buffer for extra properties strings.
>
> * Initialize buffer with extra properties strings from the
> network device at driver startup.
>
> Use ethtool's get_priv_flags to expose extra properties of
> the ENA device
>
> Signed-off-by: Arthur Kiyanovski <akiy...@amazon.com>
> Signed-off-by: Sameeh Jubran <same...@amazon.com>
> ---
...
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c
b/drivers/net/ethernet/amazon/ena/ena_com.c
> index bd0d785b2..935e8fa8d 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> @@ -1877,6 +1877,62 @@ int ena_com_get_link_params(struct ena_com_dev
*ena_dev,
> return ena_com_get_feature(ena_dev, resp, ENA_ADMIN_LINK_CONFIG);
> }
>
> +int ena_com_extra_properties_strings_init(struct ena_com_dev *ena_dev)
> +{
> + struct ena_admin_get_feat_resp resp;
> + struct ena_extra_properties_strings *extra_properties_strings =
> + &ena_dev->extra_properties_strings;
> + u32 rc;
> +
> + extra_properties_strings->size = ENA_ADMIN_EXTRA_PROPERTIES_COUNT *
> + ENA_ADMIN_EXTRA_PROPERTIES_STRING_LEN;
> +
> + extra_properties_strings->virt_addr =
> + dma_alloc_coherent(ena_dev->dmadev,
> + extra_properties_strings->size,
> + &extra_properties_strings->dma_addr,
> + GFP_KERNEL);
Do you need to fetch the private flag names on each ETHTOOL_GSTRING
request? I suppose they could change e.g. on a firmware update but then
even the count could change which you do not seem to handle. Is there
a reason not to fetch the names once on init rather then accessing the
device memory each time?
My point is that ethtool_ops::get_strings() does not return a value
which indicates that it's supposed to be a trivial operation which
cannot fail, usually a simple copy within kernel memory.
ena_com_extra_properties_strings_init() is called in probe() only, and not for
every ETHTOOL_GSTRING
request. For the latter we use ena_get_strings(). And just to make sure we are
on the same page, extra_properties_strings->virt_addr
points to the host memory and not to the device memory. I believe this should
answer your concern.
> + if (unlikely(!extra_properties_strings->virt_addr)) {
> + pr_err("Failed to allocate extra properties strings\n");
> + return 0;
> + }
> +
> + rc = ena_com_get_feature_ex(ena_dev, &resp,
> + ENA_ADMIN_EXTRA_PROPERTIES_STRINGS,
> + extra_properties_strings->dma_addr,
> + extra_properties_strings->size);
> + if (rc) {
> + pr_debug("Failed to get extra properties strings\n");
> + goto err;
> + }
> +
> + return resp.u.extra_properties_strings.count;
> +err:
> + ena_com_delete_extra_properties_strings(ena_dev);
> + return 0;
> +}
...
> diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> index fe596bc30..65bc5a2b2 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
...
> +static void get_private_flags_strings(struct ena_adapter *adapter, u8
*data)
> +{
> + struct ena_com_dev *ena_dev = adapter->ena_dev;
> + u8 *strings = ena_dev->extra_properties_strings.virt_addr;
> + int i;
> +
> + if (unlikely(!strings)) {
> + adapter->ena_extra_properties_count = 0;
> + netif_err(adapter, drv, adapter->netdev,
> + "Failed to allocate extra properties strings\n");
> + return;
> + }
This is a bit confusing, IMHO. I'm aware we shouldn't really get here as
with strings null, count would be zero and ethtool_get_strings()
wouldn't call the ->get_strings() callback. But if we ever do, it makes
little sense to complain about failed allocation (which happened once on
init) each time userspace makes ETHTOOL_GSTRINGS request for private
flags.
I believe we still want to check validity of the strings pointer to keep the
driver resilient, however I agree that
the logged message is confusing. Let us rework this commit
> +
> + for (i = 0; i < adapter->ena_extra_properties_count; i++) {
> + snprintf(data, ETH_GSTRING_LEN, "%s",
> + strings + ENA_ADMIN_EXTRA_PROPERTIES_STRING_LEN * i);
snprintf() is a bit overkill here, what you are doing is rather
strlcpy() or strscpy(). Or maybe strncpy() as strings returned by
->get_strings() do not have to be null terminated.
> + data += ETH_GSTRING_LEN;
> + }
> +}
Michal Kubecek
