[dpdk-dev] [PATCH v3 1/3] drivers/bus: move driver assignment to end of probing

Andrew Rybchenko arybchenko at solarflare.com
Thu Oct 11 17:41:52 CEST 2018


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 <thomas at 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.



More information about the dev mailing list