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

[Michal Kubecek <mkubecek@suse.cz>]

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