11/10/2018 17:41, Andrew Rybchenko:
> On 10/11/18 6:29 PM, Thomas Monjalon wrote:
> > 11/10/2018 15:15, Andrew Rybchenko:
> >> On 10/11/18 3:59 PM, Thomas Monjalon wrote:
> >>> 11/10/2018 13:54, Andrew Rybchenko:
> >>>> On 10/11/18 2:45 PM, Thomas Monjalon wrote:
> >>>>> 11/10/2018 12:53, Andrew Rybchenko:
> >>>>>> On 10/8/18 1:09 AM, Thomas Monjalon wrote:
> >>>>>>> The PCI mapping requires to know the PCI driver to use,
> >>>>>>> even before the probing is done. That's why the PCI driver is
> >>>>>>> referenced early inside the PCI device structure. See
> >>>>>>> 1d20a073fa5e ("bus/pci: reference driver structure before mapping")
> >>>>>>>
> >>>>>>> However the rte_driver does not need to be referenced in rte_device
> >>>>>>> before the device probing is done.
> >>>>>>> By moving back this assignment at the end of the device probing,
> >>>>>>> it becomes possible to make clear the status of a rte_device.
> >>>>>>>
> >>>>>>> Signed-off-by: Thomas Monjalon <tho...@monjalon.net>
> >>>>>>> ---
> >>>>>>> diff --git a/drivers/bus/pci/pci_common.c
> >>>>>>> b/drivers/bus/pci/pci_common.c
> >>>>>>> index c7695d108..d63e68045 100644
> >>>>>>> --- a/drivers/bus/pci/pci_common.c
> >>>>>>> +++ b/drivers/bus/pci/pci_common.c
> >>>>>>> @@ -160,14 +160,12 @@ rte_pci_probe_one_driver(struct rte_pci_driver
> >>>>>>> *dr,
> >>>>>>> * driver flags for adjusting configuration.
> >>>>>>> */
> >>>>>>> dev->driver = dr;
> >>>>>>> - dev->device.driver = &dr->driver;
> >>>>>> It breaks net/sfc and I guess other drivers which use
> >>>>>> rte_eth_dma_zone_reserve()
> >>>>>> from probe. The function makes zone name using
> >>>>>> dev->device->driver->name.
> >>>>> Please, can you show code line where we does such access?
> >>>>>
> >>>>> I checked such access before and did not find some.
> >>>>> Anyway, it can be fixed by accessing rte_pci_driver->driver->name.
> >>>>> Note that rte_pci_driver is referenced in rte_pci_device.
> >>>> Below in snprintf(), in theory it can be called for vdev as well.
> >>>>
> >>>> const struct rte_memzone *
> >>>> rte_eth_dma_zone_reserve(const struct rte_eth_dev *dev, const char
> >>>> *ring_name,
> >>>> uint16_t queue_id, size_t size, unsigned
> >>>> align,
> >>>> int socket_id)
> >>>> {
> >>>> char z_name[RTE_MEMZONE_NAMESIZE];
> >>>> const struct rte_memzone *mz;
> >>>>
> >>>> snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
> >>>> dev->device->driver->name, ring_name,
> >>>> dev->data->port_id, queue_id);
> >>> I see, I missed it.
> >>>
> >>> I think it's strange to use rte_device name for ethdev memory.
> >>> Should we use the ethdev name instead?
> >>>
> >>> snprintf(z_name, sizeof(z_name), "%s_%s_%d_%d",
> >>> - dev->device->driver->name, ring_name,
> >>> + dev->data->name, ring_name,
> >>> dev->data->port_id, queue_id);
> >> data->name could be update to 63 characters (RTE_DEV_NAME_MAX_LEN=64).
> >> RTE_MEMZONE_NAMESIZE is 32. Sounds like a problem.
> >> It is especially a problem if name may be specified/set by user.
> >>
> >> Right now device driver writer knows the driver name, choose ring name and
> >> have limits on port and queue ID. So, the writer at least has possibility
> >> to
> >> be sure that the results will always fit z_name.
> > What about removing the device name from the memzone name?
> > It is already unique thanks to port_id, queue_id and ring_name.
>
> Driver name is nice since it simplify buggy code identification, but
> not that critical. Maybe we should highlight that it is ethdev
> (not other port/queue), i.e. ethdev_%s_%d_%d, to be sure
> that port_id and queue_id uniquely identify it.
OK, I send a patch then.
