On Thu, 31 Mar 2016 19:36:55 +0000 Harish Patil <harish.patil at qlogic.com> wrote:
> > > > >On Wed, 30 Mar 2016 22:16:51 +0000 > >Harish Patil <harish.patil at qlogic.com> wrote: > > > >> > > >> >On Tue, 29 Mar 2016 22:28:20 -0700 > >> >Rasesh Mody <rasesh.mody at 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.