netdev - Re: [net-next 08/15] iavf: Fix up debug print macro

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1203ebd7c815a4aba4cf6ff12c4d7a8548c51952.camel@intel.com>
Date:   Mon, 01 Jul 2019 10:27:17 -0700
From:   Jeff Kirsher <jeffrey.t.kirsher@...el.com>
To:     Joe Perches <joe@...ches.com>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
        Andrew Bowers <andrewx.bowers@...el.com>
Subject: Re: [net-next 08/15] iavf: Fix up debug print macro

On Sat, 2019-06-29 at 13:42 -0700, Joe Perches wrote:
> On Fri, 2019-06-28 at 15:49 -0700, Jeff Kirsher wrote:
> > This aligns the iavf_debug() macro with the other Intel drivers.
> > 
> > Add the bus number, bus_id field to i40e_bus_info so output shows
> > each physical port(i.e func) in following format:
> >   [[[[<domain>]:]<bus>]:][<slot>][.[<func>]]
> > domains are numbered from 0 to ffff), bus (0-ff), slot (0-1f) and
> > function (0-7).
> > 
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> > Tested-by: Andrew Bowers <andrewx.bowers@...el.com>
> > ---
> >  drivers/net/ethernet/intel/iavf/iavf_osdep.h | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> > b/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> > index d39684558597..a452ce90679a 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> > @@ -44,8 +44,12 @@ struct iavf_virt_mem {
> >  #define iavf_allocate_virt_mem(h, m, s)
> > iavf_allocate_virt_mem_d(h, m, s)
> >  #define iavf_free_virt_mem(h, m) iavf_free_virt_mem_d(h, m)
> >  
> > -#define iavf_debug(h, m, s, ...)  iavf_debug_d(h, m, s,
> > ##__VA_ARGS__)
> > -extern void iavf_debug_d(void *hw, u32 mask, char *fmt_str, ...)
> > -	__printf(3, 4);
> > +#define iavf_debug(h, m, s, ...)				\
> > +do {								
> > \
> > +	if (((m) & (h)->debug_mask))				\
> > +		pr_info("iavf %02x:%02x.%x " s,			\
> > +			(h)->bus.bus_id, (h)->bus.device,	\
> > +			(h)->bus.func, ##__VA_ARGS__);		\
> > +} while (0)
> 
> Why not change the function to do this?
> 
> And if this is really wanted this particular way
> the now unused function should be removed too.
> 
> But I suggest emitting at KERN_DEBUG and using
> the more typical %pV vsprintf extension.

I see what you are saying, I was only looking at the macro in the osdep
to align with our other drivers and sync up with our internal driver
code.  Let me review the iavf driver debug function to see if there are
additional fix-ups/sync-ups needed.

> 
> ---
> 
>  drivers/net/ethernet/intel/iavf/iavf_main.c  | 25 ++++++++++++++--
> ---------
>  drivers/net/ethernet/intel/iavf/iavf_osdep.h |  9 ++++++---
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 881561b36083..8504fd71d398 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -143,25 +143,28 @@ enum iavf_status iavf_free_virt_mem_d(struct
> iavf_hw *hw,
>  }
>  
>  /**
> - * iavf_debug_d - OS dependent version of debug printing
> + * _iavf_debug - OS dependent version of debug printing
>   * @hw:  pointer to the HW structure
>   * @mask: debug level mask
> - * @fmt_str: printf-type format description
> + * @fmt: printf-type format description
>   **/
> -void iavf_debug_d(void *hw, u32 mask, char *fmt_str, ...)
> +void _iavf_debug(const struct iavf_hw *hw, u32 mask, const char
> *fmt, ...)
>  {
> -	char buf[512];
> -	va_list argptr;
> +	struct va_format vaf;
> +	va_list args;
>  
> -	if (!(mask & ((struct iavf_hw *)hw)->debug_mask))
> +	if (!(hw->debug_mask & mask))
>  		return;
>  
> -	va_start(argptr, fmt_str);
> -	vsnprintf(buf, sizeof(buf), fmt_str, argptr);
> -	va_end(argptr);
> +	va_start(args, fmt);
>  
> -	/* the debug string is already formatted with a newline */
> -	pr_info("%s", buf);
> +	vaf.fmt = fmt;
> +	vaf.va = &args;
> +
> +	pr_debug("iavf %02x:%02x.%x %pV",
> +		 hw->bus.bus_id, hw->bus.device, hw->bus.func, &vaf);
> +
> +	va_end(args);
>  }
>  
>  /**
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> b/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> index d39684558597..0e6ac7d262c8 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf_osdep.h
> @@ -44,8 +44,11 @@ struct iavf_virt_mem {
>  #define iavf_allocate_virt_mem(h, m, s) iavf_allocate_virt_mem_d(h,
> m, s)
>  #define iavf_free_virt_mem(h, m) iavf_free_virt_mem_d(h, m)
>  
> -#define iavf_debug(h, m, s, ...)  iavf_debug_d(h, m, s,
> ##__VA_ARGS__)
> -extern void iavf_debug_d(void *hw, u32 mask, char *fmt_str, ...)
> -	__printf(3, 4);
> +struct iavf_hw;
> +
> +__printf(3, 4)
> +void _iavf_debug(const struct iavf_hw *hw, u32 mask, const char
> *fmt, ...);
> +#define iavf_debug(hw, mask, fmt, ...)				
> 	\
> +	_iavf_debug(hw, mask, fmt, ##__VA_ARGS__)
>  
>  #endif /* _IAVF_OSDEP_H_ */
> 
> 


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)