Re: [PATCH-RFC] README 1ST - New problem logging macros (2.5.38)

[Rusty Russell <rusty@rustcorp.com.au>]

In message <3D8FD0A9.1010906@pobox.com> you write:
> > @@ -325,7 +326,8 @@
> >  	while(inb(cmd_ioaddr) && --wait >= 0);
> >  #ifndef final_version
> >  	if (wait < 0)
> > -		printk(KERN_ALERT "eepro100: wait_for_cmd_done timeout!\n");
> > +		problem(LOG_ALERT, "eepro100: wait_for_cmd_done timeout!",
> > +				detail(ioaddr, "%lx", cmd_ioaddr));
> 
> bloat, the ioaddr can easily be deduced

No!  That's *exactly* the problem: you see:
	eepro100: wait_for_cmd_done timeout!

in your logs, now *which* of the 5 eepro100 cards was it?

wait_for_cmd_done(long cmd_ioaddr) should take a 'struct net_device *'
and use net_problem, then no details needed.

> > +	pci_introduce(pdev);
> 
> bloat, we don't need foo_introduce() functions for every subsystem, when 
> every subsystem always has an attach-new-device function.

Yes, this should be moved up into the generic pci/networking code, but I
guess they wanted a self-contained example.

> > -		printk (KERN_ERR "eepro100: cannot reserve MMIO region\n");
> > +		pci_problem(LOG_ERR, pdev, "eepro100: cannot reserve MMIO regio
n");
> 
> bloat, no advantage over printk

Now, which of those 5 cards was it again?

> > -			printk(KERN_WARNING "%s: Invalid EEPROM checksum %#4.4x
, "
> > -				   "check settings before activating this devic
e!\n",
> > -				   dev->name, sum);
> > +			net_pci_problem(LOG_WARNING, dev, pdev, "Invalid EEPROM
 checksum, "
> > +				   "check settings before activating this devic
e!",
> 
> > +				   detail(checksum, "%#4.4x", sum));
> 
> bloat, checksum is purely informational, and can be obtained through 
> other means

It's a direct translation from the printk ferchissakes!

> likewise
> 
> etcetera...

Exactly.

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.