From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753845AbbGTFaP (ORCPT ); Mon, 20 Jul 2015 01:30:15 -0400 Received: from mail-pa0-f44.google.com ([209.85.220.44]:33538 "EHLO mail-pa0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752217AbbGTFaM (ORCPT ); Mon, 20 Jul 2015 01:30:12 -0400 Date: Mon, 20 Jul 2015 10:59:54 +0530 From: Sudip Mukherjee To: Jakub Sitnicki Cc: Greg Kroah-Hartman , linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org Subject: Re: [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E Message-ID: <20150720052954.GA5522@sudip-PC> References: <1437046092-16363-1-git-send-email-sudipm.mukherjee@gmail.com> <1437046092-16363-5-git-send-email-sudipm.mukherjee@gmail.com> <87si8m94t8.fsf@frog.home> <20150718044607.GA14213@sudip-PC> <87lhedjqc0.fsf@frog.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87lhedjqc0.fsf@frog.home> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Jul 18, 2015 at 08:03:27PM +0200, Jakub Sitnicki wrote: > On Sat, Jul 18, 2015 at 06:46 AM CEST, Sudip Mukherjee wrote: > > On Fri, Jul 17, 2015 at 05:33:55PM +0200, Jakub Sitnicki wrote: > >> On Thu, Jul 16, 2015 at 01:28 PM CEST, Sudip Mukherjee wrote: > >> > Stop using DBG_88E which is a custom macro for printing debugging > >> > messages. Instead start using pr_debug and in the process define > >> > pr_fmt. > >> > >> In the end, don't we want to use netdev_dbg() everywhere where we work > >> with a struct net_device? And use dev_dbg() everywhere where we work > >> with a struct device (or a struct usb_interface)? > > Looks like in some places we can get net_device from usb_interface. > > > > struct dvobj_priv *dvobj = usb_get_intfdata(pusb_intf); > > struct adapter *padapter = dvobj->if1; > > struct net_device *pnetdev = padapter->pnetdev; > > You're right providing that the net_device has been successfully > allocated and hasn't been deallocated yet. There are at least a couple > of pr_debug() calls that couldn't be converted to netdev_dbg(), one in > rtw_drv_init() and another in rtw_dev_remove(), because the net_device > is not available, if I'm not wrong. yes. That is why I said in some places. > > >> At least that's how I understand commit 8f26b8376faa ("checkpatch: > >> update suggested printk conversions") description: > >> > >> Direct conversion of printk(KERN_... to pr_ isn't the > >> preferred conversion when a struct net_device or struct device is > >> available. > >> > >> Do you think it is worth going straight for netdev_dbg()/dev_dbg() to > >> avoid redoing it later? > > At the end it should be netdev_* or dev_* and if both are not available > > then pr_*. Here my main intention was to remove the custom defined > > macro. And while doing this it is easier to use a script to reduce the > > chances of error. Now that the custom macro is out of the way we can > > concentrate on converting it to netdev_* or dev_*. > >> > >> > > > > >> > > >> > +#define pr_fmt(fmt) "R8188EU: " fmt > >> > #include > >> > #include > >> > #include > >> > >> If we're going to stay with pr_debug(), using KBUILD_MODNAME seems to be > >> the convention among drivers when defining pr_fmt(): > >> > >> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > yes, KBUILD_MODNAME is the usual convention but you will also find many > > places where that has not been used. Here I have used R8188EU to keep it > > same for all the messages that will be printed from the other files of > > this driver else it might be confusing for the user. > > I see your point. If consistent log prefix is the goal do you think it > would make sense to change it to: > > #define pr_fmt(fmt) DRIVER_PREFIX ": " fmt > > ... so the prefix is not defined in two places? Well, then we need to define DRIVER_PREFIX in some header file and by norm all local header files should be included at the end. And pr_fmt should be at the top so we will not have DRIVER_PREFIX at that point. regards sudip