From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751165AbdAMHYl (ORCPT ); Fri, 13 Jan 2017 02:24:41 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:56286 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751099AbdAMHYk (ORCPT ); Fri, 13 Jan 2017 02:24:40 -0500 Date: Fri, 13 Jan 2017 08:22:35 +0100 From: Greg KH To: Serge Semin Cc: srinivas.kandagatla@linaro.org, andrew@lunn.ch, robh+dt@kernel.org, mark.rutland@arm.com, Sergey.Semin@t-platforms.ru, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [PATCH v4 1/2] eeprom: Add IDT 89HPESx EEPROM/CSR driver Message-ID: <20170113072235.GA12825@kroah.com> References: <1480458434-22523-1-git-send-email-fancer.lancer@gmail.com> <1481638971-6247-1-git-send-email-fancer.lancer@gmail.com> <1481638971-6247-2-git-send-email-fancer.lancer@gmail.com> <20170111082119.GA26387@kroah.com> <20170112225417.GA21220@mobilestation> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170112225417.GA21220@mobilestation> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 13, 2017 at 01:54:17AM +0300, Serge Semin wrote: > On Wed, Jan 11, 2017 at 09:21:19AM +0100, Greg KH wrote: > > > + /* Return failure if root directory doesn't exist */ > > > + if (!csr_dbgdir) { > > > + dev_dbg(dev, "No Debugfs root directory"); > > > + return -EINVAL; > > > + } > > > > If debugfs is not enabled, don't error out, just keep going, it should > > never stop kernel code from running properly. > > > > Also, this test isn't really doing what you think it is doing... > > > > I see, it must be replaced with IS_ERR_OR_NULL() test. No! That's a pain, when the debugfs interface was created its goal was to make it _easy_ to use, not hard. IS_ERR_OR_NULL() is hard, and messy, don't do that. > But I don't think, > it would be good to get rid of dev_dbg() completely here. In case if > debugging is enabled, user would understand why csr-node isn't created within > DebugFS directory. I don't see the reasoning why one shouldn't know a source > of possible problems. > (See the next comment as continue of the discussion) Why would a user care about debugfs? > > > + /* Create Debugfs directory for CSR file */ > > > + snprintf(fname, CSRNAME_LEN, "%d-%04hx", cli->adapter->nr, cli->addr); > > > + pdev->csr_dir = debugfs_create_dir(fname, csr_dbgdir); > > > + if (IS_ERR_OR_NULL(pdev->csr_dir)) { > > > + dev_err(dev, "Failed to create CSR node directory"); > > > + return -EINVAL; > > > > Again, don't do this, you really don't care if debugfs worked or not. > > > > Actually the driver doesn't stop the kernel code from running, if it finds out > any problem with DebugFS CSR-node creation. The function just logs the error > and return error status. Take a look the place the method is called: > 1489 /* Create debugfs files */ > 1490 (void)idt_create_dbgfs_files(pdev); > The initialization code doesn't check the return value at all, so the driver > will proceed with further code. > Why did I make the function with return value? Because it's a good style to > always return a status of function code execution if it may fail, but only > caller will decide whether to check the return value or not. There is only one type of error that a debugfs call can return, and that is if debugfs is not enabled in the build. That's it, you don't need to care about any of that. > Regarding the error printing. In case if the code gets to this check, one can > be sure the DebugFS works properly, so in case if the driver failed to create > the corresponding sub-directory or node, it is really error to have any failure > at this point, and a user should be notified. But still the driver won't stop > functioning, since the caller doesn't check the return value. > > Hopefully you'll understand my point. Please understand mine, debugfs is supposed to be easy to use, you are not testing things properly here, and when you are, it doesn't matter. Just call the functions, save the return results if you need to (for dentries and the like), and move on. No error handling needed AT ALL! Yes, it feels "odd" for kernel code, but remember, this is only for debugging. Your code should not have any different codepaths for if the debugging logic worked or not. It doesn't care at all. So please, make it simple. > > > + dev_dbg(dev, "Debugfs-files created"); > > > > You do know about ftrace, right? Please remove all of these > > "trace-like" debugging lines, they aren't needed for anyone. > > > > Ok, I'll remove all these prints, even though I do find these prints being > handy to have initialization process printed on debugging stage. Then use ftrace, that is what it is there for, don't roll your own driver-specific-functionality please. thanks, greg k-h