diff mbox

[OPW,kernel] staging: rts5139 replace printk calls by dev_dbg

Message ID b0ae1627-a940-4ad8-bd7d-79d1db198c41@googlegroups.com
State New, archived
Headers show

Commit Message

asemahfouz@gmail.com Nov. 11, 2013, 3:58 a.m. UTC
This patch was made after running checkpatch.pl on drivers/staging/rts5139. All printk calls in debug.h were replaced by dev_dbg.

Comments

Waskiewicz Jr, Peter P Nov. 11, 2013, 4:32 a.m. UTC | #1
On Sun, 2013-11-10 at 19:58 -0800, asemahfouz@gmail.com wrote:
> This patch was made after running checkpatch.pl on drivers/staging/rts5139. All printk calls in debug.h were replaced by dev_dbg.

There's a few issues with this patch.

First, it's general practice to include the error that you get from
checkpatch.pl in the commit message.

Second, you did not include a Signed-off-by: line, which is required as
the certificate of origin saying you are responsible for this code.
Please see http://kernelnewbies.org/PatchPhilosophy, which is linked
from the OPW patch tutorial.  It also references the SubmittingPatches
documentation in the kernel, which has everything you need to know on
how to submit a properly constructed patch.

The biggest issue is below.

> diff --git a/drivers/staging/rts5139/debug.h b/drivers/staging/rts5139/debug.h
> index 73dec13..15f1737 100644
> --- a/drivers/staging/rts5139/debug.h
> +++ b/drivers/staging/rts5139/debug.h
> @@ -32,9 +32,9 @@
>  #define RTS51X_TIP "rts51x: "
>  
>  #ifdef CONFIG_RTS5139_DEBUG
> -#define RTS51X_DEBUGP(x...) printk(KERN_DEBUG RTS51X_TIP x)
> -#define RTS51X_DEBUGPN(x...) printk(KERN_DEBUG x)
> -#define RTS51X_DEBUGPX(x...) printk(x)
> +#define RTS51X_DEBUGP(x...) dev_dbg(dev RTS51X_TIP x)
> +#define RTS51X_DEBUGPN(x...) dev_dbg(dev x)
> +#define RTS51X_DEBUGPX(x...) dev_dbg(x)

This wasn't compile-tested.  You may have compiled the kernel after
making this change, but I'm guessing that in your .config,
CONFIG_RTS5139=n, and CONFIG_RTS5139_DEBUG=n.  Here's a snippet of the
build errors (there's quite a few due to the macro expansion after
preprocessing):

make[3]: `arch/x86/realmode/rm/realmode.bin' is up to date.
  CC [M]  drivers/staging/rts5139/rts51x_transport.o
In file included from include/linux/kernel.h:14:0,
                 from include/linux/sched.h:15,
                 from include/linux/blkdev.h:4,
                 from drivers/staging/rts5139/rts51x_transport.c:26:
drivers/staging/rts5139/rts51x_transport.c: In function
‘rts51x_msg_common’:
include/linux/dynamic_debug.h:64:16: error: initializer element is not
constant
  static struct _ddebug  __aligned(8)   \
                ^
include/linux/dynamic_debug.h:84:2: note: in expansion of macro
‘DEFINE_DYNAMIC_DEBUG_METADATA’
  DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);  \
  ^
include/linux/device.h:1076:2: note: in expansion of macro
‘dynamic_dev_dbg’
  dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
  ^
drivers/staging/rts5139/debug.h:35:29: note: in expansion of macro
‘dev_dbg’
 #define RTS51X_DEBUGP(x...) dev_dbg(dev RTS51X_TIP x)
                             ^
drivers/staging/rts5139/trace.h:40:2: note: in expansion of macro
‘RTS51X_DEBUGP’
  RTS51X_DEBUGP("[%s][%s]:[%d]\n", _file, __func__, __LINE__); \
  ^
drivers/staging/rts5139/rts51x_transport.c:188:3: note: in expansion of
macro ‘TRACE_RET’
   TRACE_RET(chip, -EIO);
   ^

The main issue is you changed what these macros expand to, but didn't
didn't change the parameter list at all, or how they're called.  For
example, this is what you have RTS51X_DEBUGP() re-defined to:

#define RTS51X_DEBUGP(x...) dev_dbg(dev RTS51X_TIP x)

and here's how it's being called from within the driver:

RTS51X_DEBUGP("[%s][%s]:[%d]\n", _file, __func__, __LINE__);

How does the "dev" parameter get passed into the macro?  So in these
cases, the macros have to change, plus how they're called in the driver.

Given how many build errors I received, there are quite a few places
you'll need to update.

-PJ
asemahfouz@gmail.com Nov. 11, 2013, 8:19 a.m. UTC | #2
Thanks for the reply Peter and the extensive comments. Concerning the
submitted patch, I will be more diligent concerning the format. Apologies
for not abiding by the specifications. As for the patch itself, what does
it mean to have a config parameter set to n e.g. CONFIG_RTS5139_DEBUG=n?
You can link me to a tutorial or an rfc instead of explaining it. Last but
not least, I will fix the debug.h by adding some extra definitions and
including some header files. Also I'll make sure that all the calls are
expanded correctly before resubmitting the patch.


On Mon, Nov 11, 2013 at 6:32 AM, Waskiewicz Jr, Peter P <
peter.p.waskiewicz.jr@intel.com> wrote:

> On Sun, 2013-11-10 at 19:58 -0800, asemahfouz@gmail.com wrote:
> > This patch was made after running checkpatch.pl on
> drivers/staging/rts5139. All printk calls in debug.h were replaced by
> dev_dbg.
>
> There's a few issues with this patch.
>
> First, it's general practice to include the error that you get from
> checkpatch.pl in the commit message.
>
> Second, you did not include a Signed-off-by: line, which is required as
> the certificate of origin saying you are responsible for this code.
> Please see http://kernelnewbies.org/PatchPhilosophy, which is linked
> from the OPW patch tutorial.  It also references the SubmittingPatches
> documentation in the kernel, which has everything you need to know on
> how to submit a properly constructed patch.
>
> The biggest issue is below.
>
> > diff --git a/drivers/staging/rts5139/debug.h
> b/drivers/staging/rts5139/debug.h
> > index 73dec13..15f1737 100644
> > --- a/drivers/staging/rts5139/debug.h
> > +++ b/drivers/staging/rts5139/debug.h
> > @@ -32,9 +32,9 @@
> >  #define RTS51X_TIP "rts51x: "
> >
> >  #ifdef CONFIG_RTS5139_DEBUG
> > -#define RTS51X_DEBUGP(x...) printk(KERN_DEBUG RTS51X_TIP x)
> > -#define RTS51X_DEBUGPN(x...) printk(KERN_DEBUG x)
> > -#define RTS51X_DEBUGPX(x...) printk(x)
> > +#define RTS51X_DEBUGP(x...) dev_dbg(dev RTS51X_TIP x)
> > +#define RTS51X_DEBUGPN(x...) dev_dbg(dev x)
> > +#define RTS51X_DEBUGPX(x...) dev_dbg(x)
>
> This wasn't compile-tested.  You may have compiled the kernel after
> making this change, but I'm guessing that in your .config,
> CONFIG_RTS5139=n, and CONFIG_RTS5139_DEBUG=n.  Here's a snippet of the
> build errors (there's quite a few due to the macro expansion after
> preprocessing):
>
> make[3]: `arch/x86/realmode/rm/realmode.bin' is up to date.
>   CC [M]  drivers/staging/rts5139/rts51x_transport.o
> In file included from include/linux/kernel.h:14:0,
>                  from include/linux/sched.h:15,
>                  from include/linux/blkdev.h:4,
>                  from drivers/staging/rts5139/rts51x_transport.c:26:
> drivers/staging/rts5139/rts51x_transport.c: In function
> ‘rts51x_msg_common’:
> include/linux/dynamic_debug.h:64:16: error: initializer element is not
> constant
>   static struct _ddebug  __aligned(8)   \
>                 ^
> include/linux/dynamic_debug.h:84:2: note: in expansion of macro
> ‘DEFINE_DYNAMIC_DEBUG_METADATA’
>   DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);  \
>   ^
> include/linux/device.h:1076:2: note: in expansion of macro
> ‘dynamic_dev_dbg’
>   dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
>   ^
> drivers/staging/rts5139/debug.h:35:29: note: in expansion of macro
> ‘dev_dbg’
>  #define RTS51X_DEBUGP(x...) dev_dbg(dev RTS51X_TIP x)
>                              ^
> drivers/staging/rts5139/trace.h:40:2: note: in expansion of macro
> ‘RTS51X_DEBUGP’
>   RTS51X_DEBUGP("[%s][%s]:[%d]\n", _file, __func__, __LINE__); \
>   ^
> drivers/staging/rts5139/rts51x_transport.c:188:3: note: in expansion of
> macro ‘TRACE_RET’
>    TRACE_RET(chip, -EIO);
>    ^
>
> The main issue is you changed what these macros expand to, but didn't
> didn't change the parameter list at all, or how they're called.  For
> example, this is what you have RTS51X_DEBUGP() re-defined to:
>
> #define RTS51X_DEBUGP(x...) dev_dbg(dev RTS51X_TIP x)
>
> and here's how it's being called from within the driver:
>
> RTS51X_DEBUGP("[%s][%s]:[%d]\n", _file, __func__, __LINE__);
>
> How does the "dev" parameter get passed into the macro?  So in these
> cases, the macros have to change, plus how they're called in the driver.
>
> Given how many build errors I received, there are quite a few places
> you'll need to update.
>
> -PJ
>
Waskiewicz Jr, Peter P Nov. 11, 2013, 5 p.m. UTC | #3
Hi Aya,

On Mon, 2013-11-11 at 10:19 +0200, Aya Mahfouz wrote:
> Thanks for the reply Peter and the extensive comments. Concerning the
> submitted patch, I will be more diligent concerning the format.
> Apologies for not abiding by the specifications. As for the patch
> itself, what does it mean to have a config parameter set to n e.g.
> CONFIG_RTS5139_DEBUG=n? You can link me to a tutorial or an rfc
> instead of explaining it. Last but not least, I will fix the debug.h
> by adding some extra definitions and including some header files. Also
> I'll make sure that all the calls are expanded correctly before
> resubmitting the patch.

Please don't top-post in email replies to kernel mailing lists.  The
email format to follow is reply in-line to the specific pieces of
feedback you'd like to address, much like I did your original email.

What I meant about the config parameter is your kernel configuration
probably doesn't have RTS5139 selected to be part of the build, hence
you didn't see the compile failures.  Please refer to
http://kernelnewbies.org/OPWfirstpatch and look at "Recompiling the
driver."  That shows the make menuconfig command, which will bring up
the kernel configuration screen.  From there, you'd need to drill down
into Device Drivers -> Staging, then select the Realtek RTS5139 driver.
Once you select that, another option will appear, which is the debug
part of RTS5139.  Without that selected, the code you're touching will
never be compiled.

I hope this helps clarify.

-PJ
diff mbox

Patch

diff --git a/drivers/staging/rts5139/debug.h b/drivers/staging/rts5139/debug.h
index 73dec13..15f1737 100644
--- a/drivers/staging/rts5139/debug.h
+++ b/drivers/staging/rts5139/debug.h
@@ -32,9 +32,9 @@ 
 #define RTS51X_TIP "rts51x: "
 
 #ifdef CONFIG_RTS5139_DEBUG
-#define RTS51X_DEBUGP(x...) printk(KERN_DEBUG RTS51X_TIP x)
-#define RTS51X_DEBUGPN(x...) printk(KERN_DEBUG x)
-#define RTS51X_DEBUGPX(x...) printk(x)
+#define RTS51X_DEBUGP(x...) dev_dbg(dev RTS51X_TIP x)
+#define RTS51X_DEBUGPN(x...) dev_dbg(dev x)
+#define RTS51X_DEBUGPX(x...) dev_dbg(x)
 #define RTS51X_DEBUG(x) x
 #else
 #define RTS51X_DEBUGP(x...)