RE: Re: [PATCH v2 2/4] mwifiex: fall back mwifiex_dbg to pr_info when adapter->dev not set

[Xinming Hu]

Hi Dmitry,

> -Original Message-
> From: Dmitry Torokhov [mailto:d...@google.com]
> Sent: 2017年4月8日 1:28
> To: Xinming Hu
> Cc: Linux Wireless; Kalle Valo; Brian Norris; Rajat Jain; Amitkumar Karwar;
> Cathy Luo; Xinming Hu
> Subject: [EXT] Re: [PATCH v2 2/4] mwifiex: fall back mwifiex_dbg to pr_info
> when adapter->dev not set
> 
> External Email
> 
> --
> Hi Xinming,
> 
> On Fri, Apr 7, 2017 at 3:51 AM, Xinming Hu <huxinming...@gmail.com>
> wrote:
> > From: Xinming Hu <h...@marvell.com>
> >
> > mwifiex_dbg will do nothing before adapter->dev get assigned. several
> > logs lost in this case. it can be avoided by fall back to pr_info.
> >
> > Signed-off-by: Xinming Hu <h...@marvell.com>
> > ---
> > v2: enhance adapter->dev null case.(Brain)
> > ---
> >  drivers/net/wireless/marvell/mwifiex/main.c | 17 -
> > drivers/net/wireless/marvell/mwifiex/main.h |  2 ++
> >  2 files changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c
> > b/drivers/net/wireless/marvell/mwifiex/main.c
> > index 98fd491..f3e772f 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1749,18 +1749,25 @@ void _mwifiex_dbg(const struct
> mwifiex_adapter
> > *adapter, int mask,  {
> > struct va_format vaf;
> > va_list args;
> > +   char msg[MWIFIEX_LOG_LEN];
> >
> > -   if (!adapter->dev || !(adapter->debug_mask & mask))
> > +   if (!(adapter->debug_mask & mask))
> > return;
> >
> > va_start(args, fmt);
> >
> > -   vaf.fmt = fmt;
> > -   vaf.va = 
> > -
> > -   dev_info(adapter->dev, "%pV", );
> > +   if (!adapter->dev) {
> > +   vsnprintf(msg, MWIFIEX_LOG_LEN, fmt, args);
> > +   } else {
> > +   vaf.fmt = fmt;
> > +   vaf.va = 
> > +   dev_info(adapter->dev, "%pV", );
> > +   }
> >
> > va_end(args);
> > +
> > +   if (!adapter->dev)
> > +   pr_info("%s", msg);
> 
> Why not:
> 
> vaf.fmt = fmt;
> vaf.va = 
> 
> if (adapter->dev)
> dev_info(adapter->dev, "%pV", ); else
> pr_info("mwifiex: %pV", );
> 

Simple change here looks better, have applied in v3.

> va_end(args);
> 
> Also, instead of static "mwifiex" prefix maybe make sure you have
> pr_fmt() set properly (I am not sure if it is set or not).
> 

Yes, we have the below define:
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Thanks
Simon

> Thanks.
> 
> --
> Dmitry

Re: [PATCH v2 2/4] mwifiex: fall back mwifiex_dbg to pr_info when adapter->dev not set

[Dmitry Torokhov]

Hi Xinming,

On Fri, Apr 7, 2017 at 3:51 AM, Xinming Hu  wrote:
> From: Xinming Hu 
>
> mwifiex_dbg will do nothing before adapter->dev get assigned. several logs
> lost in this case. it can be avoided by fall back to pr_info.
>
> Signed-off-by: Xinming Hu 
> ---
> v2: enhance adapter->dev null case.(Brain)
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 17 -
>  drivers/net/wireless/marvell/mwifiex/main.h |  2 ++
>  2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index 98fd491..f3e772f 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1749,18 +1749,25 @@ void _mwifiex_dbg(const struct mwifiex_adapter 
> *adapter, int mask,
>  {
> struct va_format vaf;
> va_list args;
> +   char msg[MWIFIEX_LOG_LEN];
>
> -   if (!adapter->dev || !(adapter->debug_mask & mask))
> +   if (!(adapter->debug_mask & mask))
> return;
>
> va_start(args, fmt);
>
> -   vaf.fmt = fmt;
> -   vaf.va = 
> -
> -   dev_info(adapter->dev, "%pV", );
> +   if (!adapter->dev) {
> +   vsnprintf(msg, MWIFIEX_LOG_LEN, fmt, args);
> +   } else {
> +   vaf.fmt = fmt;
> +   vaf.va = 
> +   dev_info(adapter->dev, "%pV", );
> +   }
>
> va_end(args);
> +
> +   if (!adapter->dev)
> +   pr_info("%s", msg);

Why not:

vaf.fmt = fmt;
vaf.va = 

if (adapter->dev)
dev_info(adapter->dev, "%pV", );
else
pr_info("mwifiex: %pV", );

va_end(args);

Also, instead of static "mwifiex" prefix maybe make sure you have
pr_fmt() set properly (I am not sure if it is set or not).

Thanks.

-- 
Dmitry

[PATCH v2 2/4] mwifiex: fall back mwifiex_dbg to pr_info when adapter->dev not set

[Xinming Hu]

From: Xinming Hu 

mwifiex_dbg will do nothing before adapter->dev get assigned. several logs
lost in this case. it can be avoided by fall back to pr_info.

Signed-off-by: Xinming Hu 
---
v2: enhance adapter->dev null case.(Brain)
---
 drivers/net/wireless/marvell/mwifiex/main.c | 17 -
 drivers/net/wireless/marvell/mwifiex/main.h |  2 ++
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index 98fd491..f3e772f 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1749,18 +1749,25 @@ void _mwifiex_dbg(const struct mwifiex_adapter 
*adapter, int mask,
 {
struct va_format vaf;
va_list args;
+   char msg[MWIFIEX_LOG_LEN];
 
-   if (!adapter->dev || !(adapter->debug_mask & mask))
+   if (!(adapter->debug_mask & mask))
return;
 
va_start(args, fmt);
 
-   vaf.fmt = fmt;
-   vaf.va = 
-
-   dev_info(adapter->dev, "%pV", );
+   if (!adapter->dev) {
+   vsnprintf(msg, MWIFIEX_LOG_LEN, fmt, args);
+   } else {
+   vaf.fmt = fmt;
+   vaf.va = 
+   dev_info(adapter->dev, "%pV", );
+   }
 
va_end(args);
+
+   if (!adapter->dev)
+   pr_info("%s", msg);
 }
 EXPORT_SYMBOL_GPL(_mwifiex_dbg);
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
b/drivers/net/wireless/marvell/mwifiex/main.h
index f1cb875..d4289a9 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -164,6 +164,8 @@ enum {
 /* Address alignment */
 #define MWIFIEX_ALIGN_ADDR(p, a) (((long)(p) + (a) - 1) & ~((a) - 1))
 
+#define MWIFIEX_LOG_LEN 120
+
 /**
  *enum mwifiex_debug_level  -  marvell wifi debug level
  */
-- 
1.8.1.4