Re: [PATCH V1 net-next 02/11] net: ena: ethtool: add extra properties retrieval via get_priv_flags

On Fri, May 31, 2019 at 09:57:51PM +0000, Machulsky, Zorik wrote:

    >  
    > +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.

That's what I misunderstood. Sorry for the noise then.

    > +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  

Yes, I didn't question the check, only the error message.

Michal