[v3,1/3] PCI: Create minor pci_dev log wrapper functions

Add PCI-specific dev_printk() wrappers so we can do:

  pci_info(dev, "message\n");

instead of

  dev_info(&dev->dev, "message\n");

Signed-off-by: Frederick Lawler <fred@fredlawl.com>
Signed-off-by: Bjorn Helgaas <helgaas@kernel.org>
---
V2:
* Replace static inline varadic functions with macros instead
V3:
* No changes to this file

 include/linux/pci.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

[+to Joe]

On Fri, Jan 12, 2018 at 10:58:36PM -0600, Frederick Lawler wrote:
> Add PCI-specific dev_printk() wrappers so we can do:
> 
>   pci_info(dev, "message\n");
> 
> instead of
> 
>   dev_info(&dev->dev, "message\n");

Hi Joe,

I proposed that Fred add pci_info(), pci_warn(), pci_err(), etc.
You've done sort of similar things in the past:

  256df2f3879e ("netdevice.h net/core/dev.c: Convert netdev_<level> logging macros to functions")
  a9a79dfec239 ("ata: Convert ata_<foo>_printk(KERN_<LEVEL> to ata_<foo>_<level>")

Do you have any implementation advice for us?  Fred started with inline
functions, but tripped over some issues with varargs in inlines, so
switched to the simple #defines below.

I notice your commits above use either multiple non-inline functions
(netdev) or a single non-inline *_printk() with macros to insert the
loglevel (ata).

Would you recommend:

  - simple #defines as below,
  - multiple non-inline functions like netdev,
  - single non-inline *_printk() with macros like ata,
  - something else?

Thanks for any advice!

Bjorn

> ---
> V2:
> * Replace static inline varadic functions with macros instead
> V3:
> * No changes to this file
> 
>  include/linux/pci.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c170c92..7a5012b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2281,4 +2281,31 @@ static inline bool pci_is_thunderbolt_attached(struct pci_dev *pdev)
>  /* provide the legacy pci_dma_* API */
>  #include <linux/pci-dma-compat.h>
>  
> +#define pci_printk(level, pdev, fmt, arg...)		\
> +	dev_printk(level, &(pdev)->dev, fmt, ##arg)
> +
> +#define pci_emerg(pdev, fmt, arg...)			\
> +	dev_emerg(&(pdev)->dev, fmt, ##arg)
> +
> +#define pci_crit(pdev, fmt, arg...)			\
> +	dev_crit(&(pdev)->dev, fmt, ##arg)
> +
> +#define pci_alert(pdev, fmt, arg...)			\
> +	dev_alert(&(pdev)->dev, fmt, ##arg)
> +
> +#define pci_err(pdev, fmt, arg...)			\
> +	dev_err(&(pdev)->dev, fmt, ##arg)
> +
> +#define pci_notice(pdev, fmt, arg...)			\
> +	dev_notice(&(pdev)->dev, fmt, ##arg)
> +
> +#define pci_warn(pdev, fmt, arg...)			\
> +	dev_warn(&(pdev)->dev, fmt, ##arg)
> +
> +#define pci_info(pdev, fmt, arg...)			\
> +	 dev_info(&(pdev)->dev, fmt, ##arg)
> +
> +#define pci_dbg(pdev, fmt, arg...)			\
> +	dev_dbg(&(pdev)->dev, fmt, ##arg)
> +
>  #endif /* LINUX_PCI_H */
> -- 
> 2.7.4
>

On Wed, 2018-01-17 at 14:12 -0600, Bjorn Helgaas wrote:
> [+to Joe]
> 
> On Fri, Jan 12, 2018 at 10:58:36PM -0600, Frederick Lawler wrote:
> > Add PCI-specific dev_printk() wrappers so we can do:
> > 
> >   pci_info(dev, "message\n");
> > 
> > instead of
> > 
> >   dev_info(&dev->dev, "message\n");
> 
> Hi Joe,
> 
> I proposed that Fred add pci_info(), pci_warn(), pci_err(), etc.
> You've done sort of similar things in the past:
> 
>   256df2f3879e ("netdevice.h net/core/dev.c: Convert netdev_<level> logging macros to functions")
>   a9a79dfec239 ("ata: Convert ata_<foo>_printk(KERN_<LEVEL> to ata_<foo>_<level>")
> 
> Do you have any implementation advice for us?

I try to use separate functions only when there is
some object code savings that is greater than the
increase in object size of the new function(s).

In this case, only the object indirection dev->dev is
being saved/reused so likely unless this is going to
be used multiple dozens of times, most likely a simple
macro wrapper might be best.

> Would you recommend:
> 
>   - simple #defines as below,

For this case, yes.

>   - multiple non-inline functions like netdev,

Probably not unless you want to additionally prefix
the output with some new PCI specific content.

>   - single non-inline *_printk() with macros like ata,

Individual ata_<foo>_printk functions were used along
with multiple ata_<foo>_<level> macros there.

ata_<foo>_printk was used because there weren't enough
callers of individual ata_<foo>_<level> functions to
justify their creation to reduce overall object code size
and macros created overall smaller objects.

>   - something else?
> 
> Thanks for any advice!

> diff --git a/include/linux/pci.h b/include/linux/pci.h
[]
> > @@ -2281,4 +2281,31 @@ static inline bool pci_is_thunderbolt_attached(struct pci_dev *pdev)
> >  /* provide the legacy pci_dma_* API */
> >  #include <linux/pci-dma-compat.h>
> >  
> > +#define pci_printk(level, pdev, fmt, arg...)		\
> > +	dev_printk(level, &(pdev)->dev, fmt, ##arg)

I would generally use the ##__VA_ARGS__ form

#define pci_printk(level, pdev, fmt, ...)			\
	dev_printk(level, &(pdev)->dev, fmt, ##__VA_ARGS__)

> > +#define pci_emerg(pdev, fmt, arg...)			\
> > +	dev_emerg(&(pdev)->dev, fmt, ##arg)

#define pci_emerg(level, pdev, fmt, ...)			\
	dev_emerg(level
, &(pdev)->dev, fmt, ##__VA_ARGS__)

etc...

cheers, Joe