Re: [dpdk-dev] [PATCH v4 05/10] qede: Add core driver

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 31 Mar 2016 19:36:55 +0000
Harish Patil <harish.patil@qlogic.com> wrote:

> >
> 
> >On Wed, 30 Mar 2016 22:16:51 +0000
> >Harish Patil <harish.patil@qlogic.com> wrote:
> >
> >> >
> >> >On Tue, 29 Mar 2016 22:28:20 -0700
> >> >Rasesh Mody <rasesh.mody@qlogic.com> wrote:
> >> >
> >> >> +
> >> >> +static void qede_print_adapter_info(struct qede_dev *qdev)
> >> >> +{
> >> >> +	struct ecore_dev *edev = &qdev->edev;
> >> >> +	struct qed_dev_info *info = &qdev->dev_info.common;
> >> >> +	char ver_str[QED_DRV_VER_STR_SIZE] = { 0 };
> >> >> +
> >> >> +	RTE_LOG(INFO, PMD,
> >> >> +		  " Chip details : %s%d\n",
> >> >> +		  ECORE_IS_BB(edev) ? "BB" : "AH",
> >> >> +		  CHIP_REV_IS_A0(edev) ? 0 : 1);
> >> >> +
> >> >> +	sprintf(ver_str, "%s %s_%d.%d.%d.%d", QEDE_PMD_VER_PREFIX,
> >> >> +		edev->ver_str, QEDE_PMD_VERSION_MAJOR, QEDE_PMD_VERSION_MINOR,
> >> >> +		QEDE_PMD_VERSION_REVISION, QEDE_PMD_VERSION_PATCH);
> >> >> +	strcpy(qdev->drv_ver, ver_str);
> >> >> +	RTE_LOG(INFO, PMD, " Driver version : %s\n", ver_str);
> >> >> +
> >> >> +	ver_str[0] = '\0';
> >> >> +	sprintf(ver_str, "%d.%d.%d.%d", info->fw_major, info->fw_minor,
> >> >> +		info->fw_rev, info->fw_eng);
> >> >> +	RTE_LOG(INFO, PMD, " Firmware version : %s\n", ver_str);
> >> >> +
> >> >> +	ver_str[0] = '\0';
> >> >> +	sprintf(ver_str, "%d.%d.%d.%d",
> >> >> +		(info->mfw_rev >> 24) & 0xff,
> >> >> +		(info->mfw_rev >> 16) & 0xff,
> >> >> +		(info->mfw_rev >> 8) & 0xff, (info->mfw_rev) & 0xff);
> >> >> +	RTE_LOG(INFO, PMD, " Management firmware version : %s\n", ver_str);
> >> >> +
> >> >> +	RTE_LOG(INFO, PMD, " Firmware file : %s\n", QEDE_FW_FILE_NAME);
> >> >
> >> >This means the driver is far too chatty in the logs.
> >> >Can't this be made DEBUG level?
> >> >
> >> Not clear what is the issue here?
> >> RTE_LOG is used here to display basic adapter info like firmware/driver
> >> versions etc without the need to enable any debug flags.
> >> The driver debug logging is under the control of appropriate debug
> >>flags.
> >> 
> >
> >The DPDK log messages end up being examined by tech support and customers.
> >Too much code puts extra stuff in so it is hard to find the real problem.
> >This is obviously debug info, so either:
> >  1) make it conditionally compiled
> >  2) change log level to DEBUG
> >  3) remove it.
> >
> 
> This is not really a debug msg per se. We want it to be printed on the
> stdout unconditionally (displayed only once) so that we can readily know
> what firmware/driver/hw versions we are dealing with. We don’t want to
> depend on the apps to print those and many of the apps may not do so
> including testpmd. Even the linux drivers prints basic stuff  under dmesg.
> We have found this to be useful for triage and would like to retain it if
> there is no objection.

And Linux drivers are wrong to do this. This a case where developer thinks printing stuff
is important, but for operations it isn't. Better to provide this info through some API,
(maybe DPDK needs extensions to get_info).

Also, you don't have to format twice, once into a buffer (with sprintf) then again by
calling RTE_LOG.  Not only that sprintf() is guaranteed to null terminate the string,
so your paranoid ver_str[0] = 0; is not needed either.

Sorry for being so picky, but you are getting the feedback that comes from
experience dealing with large volumes of log info.