From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v3 1/3] drivers/bus: move driver assignment to end of probing Date: Thu, 11 Oct 2018 18:00:58 +0200 Message-ID: <1710338.3EmpsgTpP4@xps> References: <20180907230958.21402-1-thomas@monjalon.net> <1830556.armPUsWzaN@xps> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org, gaetan.rivet@6wind.com, ophirmu@mellanox.com, qi.z.zhang@intel.com, ferruh.yigit@intel.com To: Andrew Rybchenko Return-path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 7E2681B575 for ; Thu, 11 Oct 2018 18:01:01 +0200 (CEST) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > >>>>>>> --- > >>>>>>> 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.