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
