From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752505AbeB1KdQ (ORCPT ); Wed, 28 Feb 2018 05:33:16 -0500 Received: from mga12.intel.com ([192.55.52.136]:46006 "EHLO mga12.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752096AbeB1KdO (ORCPT ); Wed, 28 Feb 2018 05:33:14 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,405,1515484800"; d="scan'208";a="207610027" Message-ID: <1519813990.10722.262.camel@linux.intel.com> Subject: Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper From: Andy Shevchenko To: Jean Delvare Cc: Bjorn Helgaas , linux-pci@vger.kernel.org, "Rafael J. Wysocki" , linux-acpi@vger.kernel.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, linux-kernel@vger.kernel.org Date: Wed, 28 Feb 2018 12:33:10 +0200 In-Reply-To: <20180227101422.7c0efb40@endymion> References: <20180222125923.57385-1-andriy.shevchenko@linux.intel.com> <20180227101422.7c0efb40@endymion> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.26.5-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2018-02-27 at 10:14 +0100, Jean Delvare wrote: > Hi Andy, > > On Thu, 22 Feb 2018 14:59:20 +0200, Andy Shevchenko wrote: > > It's used in several places and more users may come. > > By using this helper they may create a slightly cleaner code. > > In general I'm not a big fan of arbitrary API shortcuts. I won't > object > if you think it's a good one to have, however I'm a bit concerned > about > the silent handling of the "error case", which could cause unexpected > default decisions as seen in patch 2/4 of this series. Yes, we better to return -ERRNO in such case and allow callers to behave correspondingly. > The fact that > dmi_get_bios_year() returns 0 if there is no DMI BIOS date provided > should at least be documented, to limit the risk. > > I also wonder if dmi_get_date() itself couldn't be optimized a bit if > it turns out that most callers are only interested in the year. > Currently it will parse the whole string even if the caller isn't > interested in the month and day. > > The fact that dmi_get_date() returns true even if it couldn't parse > the > date string at all is also strange, although unrelated with your > current work. So, what I consider is to - move inline function to be regular one - optimize it with current dmi_get_date() - return error code when year is not parsable - consider current use cases where we do compare for less than a given year Does it sound a correct approach? > > checkpatch complains: > > WARNING: Missing a blank line after declarations > #61: FILE: include/linux/dmi.h:153: > + int year; > + dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL); > > And I would tend to agree. Just because it is an inline function in a > header file doesn't mean we don't stick to the usual coding style > policy. Ingo fixed that. -- Andy Shevchenko Intel Finland Oy