From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751415AbcGPCYj (ORCPT ); Fri, 15 Jul 2016 22:24:39 -0400 Received: from mail-oi0-f42.google.com ([209.85.218.42]:32817 "EHLO mail-oi0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751247AbcGPCYh (ORCPT ); Fri, 15 Jul 2016 22:24:37 -0400 MIME-Version: 1.0 In-Reply-To: <1580087.dlcVOb6fx1@vostro.rjw.lan> References: <1468432402-4872-1-git-send-email-fu.wei@linaro.org> <5101961.eCg7ixLQWc@vostro.rjw.lan> <1580087.dlcVOb6fx1@vostro.rjw.lan> From: Fu Wei Date: Sat, 16 Jul 2016 10:24:35 +0800 Message-ID: Subject: Re: [PATCH v7 4/9] acpi/arm64: Add GTDT table parse driver To: "Rafael J. Wysocki" Cc: "Rafael J. Wysocki" , Len Brown , Daniel Lezcano , Thomas Gleixner , Marc Zyngier , Lorenzo Pieralisi , Sudeep Holla , Hanjun Guo , "linux-arm-kernel@lists.infradead.org" , "linaro-acpi@lists.linaro.org" , Linux Kernel Mailing List , ACPI Devel Maling List , rruigrok@codeaurora.org, harba@codeaurora.org, Christopher Covington , Timur Tabi , G Gregory , Al Stone , Jon Masters , wei@redhat.com, Arnd Bergmann , Wim Van Sebroeck , Catalin Marinas , Will Deacon , Suravee Suthikulanit , Leo Duran , Guenter Roeck , "linux-watchdog@vger.kernel.org" , David Miller , Andrew Morton , Greg Kroah-Hartman , kvalo@codeaurora.org, Jiri Slaby , Christoffer Dall , Julien Grall Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Rafeal, On 16 July 2016 at 05:22, Rafael J. Wysocki wrote: > On Saturday, July 16, 2016 12:32:14 AM Fu Wei wrote: >> Hi Rafael, >> >> On 15 July 2016 at 21:07, Rafael J. Wysocki wrote: >> > On Friday, July 15, 2016 02:15:27 PM Rafael J. Wysocki wrote: >> >> On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote: >> >> > Hi Rafael, >> >> > >> > >> > [cut] >> > >> >> > > >> >> > >> + return 0; >> >> > >> + } >> >> > >> + >> >> > >> + if (!gtdt->platform_timer_count) { >> >> > >> + pr_info("No Platform Timer.\n"); >> >> > >> + return 0; >> >> > >> + } >> >> > >> + >> >> > >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt + >> >> > >> + gtdt->platform_timer_offset; >> >> > >> + if (acpi_gtdt_desc.platform_timer_start < >> >> > >> + (void *)table + sizeof(struct acpi_table_gtdt)) { >> >> > >> + pr_err(FW_BUG "Platform Timer pointer error.\n"); >> >> > > >> >> > > Why pr_err()? >> >> > >> >> > if (true), that means the GTDT table has bugs. >> >> > >> >> >> >> And that's not a very useful piece of information unless you're debugging the >> >> platform, is it? >> > >> > FWIW, I'm not a big fan of printing "your firmware is buggy" type of messages >> > (especially at the "error" log level or higher) unless they can be clearly >> > connected to a specific type of functional failure. >> > >> > So if you want to pring an error-level message, something like "I cannot do X >> > because of the firmware bug Y" would be better IMO. >> >> So can I do this: >> pr_err(FW_BUG "Can NOT init platform_timer pointer, because of the >> GTDT table bug\n"); >> >> or pr_debug(FW_BUG "Can NOT init platform_timer_start, because of >> platform_timer_offset bug in GTDT\n"); >> >> or just delete it? >> >> which one do you prefer? I think maybe should provide some clue for >> users to fix the problem :-) > > And how exactly would they fix it then? > >> >> any thought ? > > If you print variable or function names and the like, the message should be > a debug one, because that's information that can only be understood by > developers (some developers are users too, but they are a minority). > > If you want to report an error, say what is not working (or not available > etc) and why (if you know the reason at the time the message is printed). Great thanks, I guess I got you point. maybe just a very simple message like: pr_err(FW_BUG "Failed to init table: GTDT table is buggy.\n"); I will also check other pr_* , if I can update them > > Thanks, > Rafael > -- Best regards, Fu Wei Software Engineer Red Hat