Re: [patch 0/5] dev_printk() conversions

Re: [patch 0/5] dev_printk() conversions

[Jesse Barnes]

On Friday, June 13, 2008 9:52 am Bjorn Helgaas wrote:
> These convert some printks to dev_printks.  They are completely
> independent of each other; I'm just posting them together because
> it probably makes sense to review them together.
>
> It'd be nice if a PCI person looked at pci_request_region().  To
> me, it looks like it prints the wrong BAR number (it prints "bar + 1").

Thanks Bjorn, I applied 2-4 to my linux-next tree.  The x86 bits didn't apply 
for me, probably because my tree is slightly out of date, but those changes 
can come through Ingo & co.

Jesse

Re: [patch 1/5] ACPI: use dev_printk when possible

[Len Brown]

On Fri, 13 Jun 2008, Bjorn Helgaas wrote:

> Convert printks to use dev_printk().  The most obvious change will
> be messages like this:
> 
>    -ACPI: PCI Interrupt 0000:00:04.0[A] -> GSI 31 (level, low) -> IRQ 31
>    +cciss 0000:00:04.0: PCI INTA# -> GSI 31 (level, low) -> IRQ 31

I like the part identifying the driver.
I'm also okay with having fewer messages tagged with ACPI:

> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Index: work11/drivers/acpi/pci_irq.c
> ===================================================================
> --- work11.orig/drivers/acpi/pci_irq.c	2008-06-12 16:53:52.000000000 -0600
> +++ work11/drivers/acpi/pci_irq.c	2008-06-13 09:32:19.000000000 -0600
> @@ -162,7 +162,7 @@ do_prt_fixups(struct acpi_prt_entry *ent
>  		    !strcmp(prt->source, quirk->source) &&
>  		    strlen(prt->source) >= strlen(quirk->actual_source)) {
>  			printk(KERN_WARNING PREFIX "firmware reports "
> -				"%04x:%02x:%02x[%c] connected to %s; "
> +				"%04x:%02x:%02x PCI INT%c# connected to %s; "

Can we delete the '#' from INTA# in this patch?
I don't think that using hardware polarity notation in kernel messages
clarifies anything, particularly since it is hard-coded and thus possibly
conflicting with the subsequent '(level, low)'...

otherwise,
Acked-by: Len Brown <len.brown@intel.com>

thanks,
-Len

>  				"changing to %s\n",
>  				entry->id.segment, entry->id.bus,
>  				entry->id.device, 'A' + entry->pin,
> @@ -429,7 +429,7 @@ acpi_pci_irq_derive(struct pci_dev *dev,
>  {
>  	struct pci_dev *bridge = dev;
>  	int irq = -1;
> -	u8 bridge_pin = 0;
> +	u8 bridge_pin = 0, orig_pin = pin;
>  
>  
>  	if (!dev)
> @@ -463,8 +463,8 @@ acpi_pci_irq_derive(struct pci_dev *dev,
>  	}
>  
>  	if (irq < 0) {
> -		printk(KERN_WARNING PREFIX "Unable to derive IRQ for device %s\n",
> -			      pci_name(dev));
> +		dev_warn(&dev->dev, "can't derive routing for PCI INT%c#\n",
> +			 'A' + orig_pin);
>  		return -1;
>  	}
>  
> @@ -487,6 +487,7 @@ int acpi_pci_irq_enable(struct pci_dev *
>  	int triggering = ACPI_LEVEL_SENSITIVE;
>  	int polarity = ACPI_ACTIVE_LOW;
>  	char *link = NULL;
> +	char link_desc[16];
>  	int rc;
>  
>  
> @@ -503,7 +504,7 @@ int acpi_pci_irq_enable(struct pci_dev *
>  	pin--;
>  
>  	if (!dev->bus) {
> -		printk(KERN_ERR PREFIX "Invalid (NULL) 'bus' field\n");
> +		dev_err(&dev->dev, "invalid (NULL) 'bus' field\n");
>  		return -ENODEV;
>  	}
>  
> @@ -538,8 +539,7 @@ int acpi_pci_irq_enable(struct pci_dev *
>  	 * driver reported one, then use it. Exit in any case.
>  	 */
>  	if (irq < 0) {
> -		printk(KERN_WARNING PREFIX "PCI Interrupt %s[%c]: no GSI",
> -		       pci_name(dev), ('A' + pin));
> +		dev_warn(&dev->dev, "PCI INT%c#: no GSI", 'A' + pin);
>  		/* Interrupt Line values above 0xF are forbidden */
>  		if (dev->irq > 0 && (dev->irq <= 0xF)) {
>  			printk(" - using IRQ %d\n", dev->irq);
> @@ -554,21 +554,21 @@ int acpi_pci_irq_enable(struct pci_dev *
>  
>  	rc = acpi_register_gsi(irq, triggering, polarity);
>  	if (rc < 0) {
> -		printk(KERN_WARNING PREFIX "PCI Interrupt %s[%c]: failed "
> -		       "to register GSI\n", pci_name(dev), ('A' + pin));
> +		dev_warn(&dev->dev, "PCI INT%c#: failed to register GSI\n",
> +			 'A' + pin);
>  		return rc;
>  	}
>  	dev->irq = rc;
>  
> -	printk(KERN_INFO PREFIX "PCI Interrupt %s[%c] -> ",
> -	       pci_name(dev), 'A' + pin);
> -
>  	if (link)
> -		printk("Link [%s] -> ", link);
> +		snprintf(link_desc, sizeof(link_desc), " -> Link[%s]", link);
> +	else
> +		link_desc[0] = '\0';
>  
> -	printk("GSI %u (%s, %s) -> IRQ %d\n", irq,
> -	       (triggering == ACPI_LEVEL_SENSITIVE) ? "level" : "edge",
> -	       (polarity == ACPI_ACTIVE_LOW) ? "low" : "high", dev->irq);
> +	dev_info(&dev->dev, "PCI INT%c#%s -> GSI %u (%s, %s) -> IRQ %d\n",
> +		 'A' + pin, link_desc, irq,
> +		 (triggering == ACPI_LEVEL_SENSITIVE) ? "level" : "edge",
> +		 (polarity == ACPI_ACTIVE_LOW) ? "low" : "high", dev->irq);
>  
>  	return 0;
>  }
> @@ -616,10 +616,6 @@ void acpi_pci_irq_disable(struct pci_dev
>  	 * (e.g. PCI_UNDEFINED_IRQ).
>  	 */
>  
> -	printk(KERN_INFO PREFIX "PCI interrupt for device %s disabled\n",
> -	       pci_name(dev));
> -
> +	dev_info(&dev->dev, "PCI INT%c# disabled\n", 'A' + pin);
>  	acpi_unregister_gsi(gsi);
> -
> -	return;
>  }
> 
> -- 
> 

Re: [patch 5/5] x86 PCI: use dev_printk when possible

[Len Brown]

I guess the 'INTA#; rather than 'INTA' syntax I'm not fond of appears in 
this x86 patch too.  I think if we don't really have anything useful 
to say about polarity, then we should not add an active low symbol.

thanks,
-Len

-       printk(KERN_INFO "PCI: %s IRQ %d for device %s\n", msg, irq, 
pci_name(dev));
+       dev_info(&dev->dev, "%s PCI INT%c# -> IRQ %d\n", msg, 'A' + pin, 
irq);

Re: [patch 5/5] x86 PCI: use dev_printk when possible

[Andrew Morton]

On Fri, 13 Jun 2008 10:52:14 -0600
Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:

> I converted DBG() to dev_dbg().  This DBG() is from arch/x86/pci/pci.h
> and requires source-code modification to enable, so dev_dbg() seems
> roughly equivalent.
> 
> The printks in arch/x86/pci/irq.c were a little hairy, with lines
> printed in several pieces.  I straightened it out a little, but
> another set of eyes would be good.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> ---
>  arch/x86/pci/fixup.c |    3 +
>  arch/x86/pci/i386.c  |   26 ++++++--------
>  arch/x86/pci/irq.c   |   91 ++++++++++++++++++++++-----------------------------
>  arch/x86/pci/numa.c  |    5 +-

Got a bunch of rejects in arch/x86/pci/irq.c due to pending whitespace
fixes.  I fixed a couple of them but then I got bored, so the
conversion in there is only partial.

Please prefer to raise patches against linux-next if poss, especially
during late -rc's.

Re: [patch 5/5] x86 PCI: use dev_printk when possible

[Jesse Barnes]

On Monday, June 30, 2008 4:57 pm Andrew Morton wrote:
> On Fri, 13 Jun 2008 10:52:14 -0600
>
> Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> > I converted DBG() to dev_dbg().  This DBG() is from arch/x86/pci/pci.h
> > and requires source-code modification to enable, so dev_dbg() seems
> > roughly equivalent.
> >
> > The printks in arch/x86/pci/irq.c were a little hairy, with lines
> > printed in several pieces.  I straightened it out a little, but
> > another set of eyes would be good.
> >
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> >
> > ---
> >  arch/x86/pci/fixup.c |    3 +
> >  arch/x86/pci/i386.c  |   26 ++++++--------
> >  arch/x86/pci/irq.c   |   91
> > ++++++++++++++++++++++----------------------------- arch/x86/pci/numa.c 
> > |    5 +-
>
> Got a bunch of rejects in arch/x86/pci/irq.c due to pending whitespace
> fixes.  I fixed a couple of them but then I got bored, so the
> conversion in there is only partial.
>
> Please prefer to raise patches against linux-next if poss, especially
> during late -rc's.

Yeah, the x86 tree(s) move too fast for me to keep up with.  The change seems 
worthwhile though, maybe Ingo or one of the x86 team could just search & 
replace?

Jesse

Re: [patch 5/5] x86 PCI: use dev_printk when possible

[Bjorn Helgaas]

On Tuesday 01 July 2008 07:57:41 pm Jesse Barnes wrote:
> On Monday, June 30, 2008 4:57 pm Andrew Morton wrote:
> > On Fri, 13 Jun 2008 10:52:14 -0600
> >
> > Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> > > I converted DBG() to dev_dbg().  This DBG() is from arch/x86/pci/pci.h
> > > and requires source-code modification to enable, so dev_dbg() seems
> > > roughly equivalent.
> > >
> > > The printks in arch/x86/pci/irq.c were a little hairy, with lines
> > > printed in several pieces.  I straightened it out a little, but
> > > another set of eyes would be good.
> > >
> > > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > >
> > > ---
> > >  arch/x86/pci/fixup.c |    3 +
> > >  arch/x86/pci/i386.c  |   26 ++++++--------
> > >  arch/x86/pci/irq.c   |   91
> > > ++++++++++++++++++++++----------------------------- arch/x86/pci/numa.c 
> > > |    5 +-
> >
> > Got a bunch of rejects in arch/x86/pci/irq.c due to pending whitespace
> > fixes.  I fixed a couple of them but then I got bored, so the
> > conversion in there is only partial.
> >
> > Please prefer to raise patches against linux-next if poss, especially
> > during late -rc's.
> 
> Yeah, the x86 tree(s) move too fast for me to keep up with.  The change seems 
> worthwhile though, maybe Ingo or one of the x86 team could just search & 
> replace?

I'll fix it up.

Bjorn