[PATCH] CCISS: Don't print driver version until we actually find a device

[PATCH] CCISS: Don't print driver version until we actually find a device

[Bjorn Helgaas]

If we don't find any devices, we shouldn't print anything.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work-mm2/drivers/block/cciss.c
===================================================================
--- work-mm2.orig/drivers/block/cciss.c	2006-07-20 16:27:34.000000000 -0600
+++ work-mm2/drivers/block/cciss.c	2006-07-25 16:16:27.000000000 -0600
@@ -3109,12 +3109,16 @@
 static int __devinit cciss_init_one(struct pci_dev *pdev,
 				    const struct pci_device_id *ent)
 {
+	static int cciss_version_printed = 0;
 	request_queue_t *q;
 	int i;
 	int j;
 	int rc;
 	int dac;
 
+	if (cciss_version_printed++ == 0)
+		printk(KERN_INFO DRIVER_NAME "\n");
+
 	i = alloc_cciss_hba();
 	if (i < 0)
 		return -1;
@@ -3370,9 +3374,6 @@
  */
 static int __init cciss_init(void)
 {
-	printk(KERN_INFO DRIVER_NAME "\n");
-
-	/* Register for our PCI devices */
 	return pci_register_driver(&cciss_pci_driver);
 }
 

Re: [PATCH] CCISS: Don't print driver version until we actually find a device

[Jesper Juhl]

On 26/07/06, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> If we don't find any devices, we shouldn't print anything.
>
I disagree.
I find it quite nice to be able to see that the driver loaded even if
it finds nothing. At least then when there's a problem, I can quickly
see that at least it is not because I didn't forget to load the
driver, it's something else. Saves time since I can start looking for
reasons why the driver didn't find anything without first spending
additional time checking if I failed to cause it to load for some
reason.


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] CCISS: Don't print driver version until we actually find a device

[Arjan van de Ven]

On Wed, 2006-07-26 at 00:43 +0200, Jesper Juhl wrote:
> On 26/07/06, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> > If we don't find any devices, we shouldn't print anything.
> >
> I disagree.
> I find it quite nice to be able to see that the driver loaded even if
> it finds nothing. At least then when there's a problem, I can quickly
> see that at least it is not because I didn't forget to load the
> driver, it's something else. Saves time since I can start looking for
> reasons why the driver didn't find anything without first spending
> additional time checking if I failed to cause it to load for some
> reason.

I'll add a second reason: it is a REALLY nice property to be able to see
which driver is started last in case of a crash/hang, so that the guilty
party is more obvious..

> 
> 
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

Re: [PATCH] CCISS: Don't print driver version until we actually find a device

[Bjorn Helgaas]

On Tuesday 25 July 2006 16:47, Arjan van de Ven wrote:
> On Wed, 2006-07-26 at 00:43 +0200, Jesper Juhl wrote:
> > On 26/07/06, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> > > If we don't find any devices, we shouldn't print anything.
> > >
> > I disagree.
> > I find it quite nice to be able to see that the driver loaded even if
> > it finds nothing. At least then when there's a problem, I can quickly
> > see that at least it is not because I didn't forget to load the
> > driver, it's something else. Saves time since I can start looking for
> > reasons why the driver didn't find anything without first spending
> > additional time checking if I failed to cause it to load for some
> > reason.
> 
> I'll add a second reason: it is a REALLY nice property to be able to see
> which driver is started last in case of a crash/hang, so that the guilty
> party is more obvious..

initcall_debug is a more reliable way to find that.  Do you want
all drivers to print something in their init function?  Right now,
there's really no consistency.  My guess is that the majority don't
print anything until a device is found.  But I admit I didn't try
to count them.

Re: [PATCH] CCISS: Don't print driver version until we actually find a device

[Jesper Juhl]

On 26/07/06, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> On Tuesday 25 July 2006 16:47, Arjan van de Ven wrote:
> > On Wed, 2006-07-26 at 00:43 +0200, Jesper Juhl wrote:
> > > On 26/07/06, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> > > > If we don't find any devices, we shouldn't print anything.
> > > >
> > > I disagree.
> > > I find it quite nice to be able to see that the driver loaded even if
> > > it finds nothing. At least then when there's a problem, I can quickly
> > > see that at least it is not because I didn't forget to load the
> > > driver, it's something else. Saves time since I can start looking for
> > > reasons why the driver didn't find anything without first spending
> > > additional time checking if I failed to cause it to load for some
> > > reason.
> >
> > I'll add a second reason: it is a REALLY nice property to be able to see
> > which driver is started last in case of a crash/hang, so that the guilty
> > party is more obvious..
>
> initcall_debug is a more reliable way to find that.  Do you want
> all drivers to print something in their init function?

It's not my call, but that would be my personal preference, yes.


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] CCISS: Don't print driver version until we actually find a device

[Jeff Garzik]

Arjan van de Ven wrote:
> On Wed, 2006-07-26 at 00:43 +0200, Jesper Juhl wrote:
>> On 26/07/06, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
>>> If we don't find any devices, we shouldn't print anything.
>>>
>> I disagree.
>> I find it quite nice to be able to see that the driver loaded even if
>> it finds nothing. At least then when there's a problem, I can quickly
>> see that at least it is not because I didn't forget to load the
>> driver, it's something else. Saves time since I can start looking for
>> reasons why the driver didn't find anything without first spending
>> additional time checking if I failed to cause it to load for some
>> reason.
> 
> I'll add a second reason: it is a REALLY nice property to be able to see
> which driver is started last in case of a crash/hang, so that the guilty
> party is more obvious..

OTOH, it is not a property that scales well at all.

When you build extra drivers into the kernel, or distros load drivers 
you don't need (_every_ distro does this), you wind up with a bunch of 
version strings for drivers for hardware you don't have.

	Jeff

Re: [PATCH] CCISS: Don't print driver version until we actually find a device

[Dmitry Torokhov]

On 7/26/06, Jeff Garzik <jeff@garzik.org> wrote:
> Arjan van de Ven wrote:
> > On Wed, 2006-07-26 at 00:43 +0200, Jesper Juhl wrote:
> >> On 26/07/06, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> >>> If we don't find any devices, we shouldn't print anything.
> >>>
> >> I disagree.
> >> I find it quite nice to be able to see that the driver loaded even if
> >> it finds nothing. At least then when there's a problem, I can quickly
> >> see that at least it is not because I didn't forget to load the
> >> driver, it's something else. Saves time since I can start looking for
> >> reasons why the driver didn't find anything without first spending
> >> additional time checking if I failed to cause it to load for some
> >> reason.
> >
> > I'll add a second reason: it is a REALLY nice property to be able to see
> > which driver is started last in case of a crash/hang, so that the guilty
> > party is more obvious..
>
> OTOH, it is not a property that scales well at all.
>
> When you build extra drivers into the kernel, or distros load drivers
> you don't need (_every_ distro does this), you wind up with a bunch of
> version strings for drivers for hardware you don't have.
>

Given that boot tracing is best done with initcall_debug and
drivers that care about their version string can report it through
/sys/modules/<driver>/version why should version string be printed at
load time at all?

-- 
Dmitry

Re: [PATCH] CCISS: Don't print driver version until we actually find a device

[gmu 2k6]

On 7/27/06, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On 7/26/06, Jeff Garzik <jeff@garzik.org> wrote:
> > Arjan van de Ven wrote:
> > > On Wed, 2006-07-26 at 00:43 +0200, Jesper Juhl wrote:
> > >> On 26/07/06, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> > >>> If we don't find any devices, we shouldn't print anything.
> > >>>
> > >> I disagree.
> > >> I find it quite nice to be able to see that the driver loaded even if
> > >> it finds nothing. At least then when there's a problem, I can quickly
> > >> see that at least it is not because I didn't forget to load the
> > >> driver, it's something else. Saves time since I can start looking for
> > >> reasons why the driver didn't find anything without first spending
> > >> additional time checking if I failed to cause it to load for some
> > >> reason.
> > >
> > > I'll add a second reason: it is a REALLY nice property to be able to see
> > > which driver is started last in case of a crash/hang, so that the guilty
> > > party is more obvious..
> >
> > OTOH, it is not a property that scales well at all.
> >
> > When you build extra drivers into the kernel, or distros load drivers
> > you don't need (_every_ distro does this), you wind up with a bunch of
> > version strings for drivers for hardware you don't have.
> >
>
> Given that boot tracing is best done with initcall_debug and
> drivers that care about their version string can report it through
> /sys/modules/<driver>/version why should version string be printed at
> load time at all?

not every driver provides that file (btw, I guess you mean
/sys/module, don't you?) there anyway so it's still inconsistent.
what if you can see up until loading of the driver and it halts there
without /sys being mounted yet. I don't think you can rely on sysfs
being mounted or modules being loaded.
if I may vote as a CCISS user I say print the version number even if
no device is present.

Re: [PATCH] CCISS: Don't print driver version until we actually find a device

[Dmitry Torokhov]

On 7/27/06, gmu 2k6 <gmu2006@gmail.com> wrote:
> On 7/27/06, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On 7/26/06, Jeff Garzik <jeff@garzik.org> wrote:
> > > Arjan van de Ven wrote:
> > > > On Wed, 2006-07-26 at 00:43 +0200, Jesper Juhl wrote:
> > > >> On 26/07/06, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> > > >>> If we don't find any devices, we shouldn't print anything.
> > > >>>
> > > >> I disagree.
> > > >> I find it quite nice to be able to see that the driver loaded even if
> > > >> it finds nothing. At least then when there's a problem, I can quickly
> > > >> see that at least it is not because I didn't forget to load the
> > > >> driver, it's something else. Saves time since I can start looking for
> > > >> reasons why the driver didn't find anything without first spending
> > > >> additional time checking if I failed to cause it to load for some
> > > >> reason.
> > > >
> > > > I'll add a second reason: it is a REALLY nice property to be able to see
> > > > which driver is started last in case of a crash/hang, so that the guilty
> > > > party is more obvious..
> > >
> > > OTOH, it is not a property that scales well at all.
> > >
> > > When you build extra drivers into the kernel, or distros load drivers
> > > you don't need (_every_ distro does this), you wind up with a bunch of
> > > version strings for drivers for hardware you don't have.
> > >
> >
> > Given that boot tracing is best done with initcall_debug and
> > drivers that care about their version string can report it through
> > /sys/modules/<driver>/version why should version string be printed at
> > load time at all?
>
> not every driver provides that file (btw, I guess you mean
> /sys/module, don't you?) there anyway so it's still inconsistent.

Yes, you are right, we drop version string when !MODULE. We need to
keep it if we want to have consistent sysfs data.

> what if you can see up until loading of the driver and it halts there
> without /sys being mounted yet. I don't think you can rely on sysfs
> being mounted or modules being loaded.

Right, so you boot with debug_initcall to see what was the last thing
that we tried to initialize. Hmm, we might need to add debug_modcall
to print names of modules being loaded as well.

-- 
Dmitry

RE: [PATCH] CCISS: Don't print driver version until we actually find a device

[Miller, Mike (OS Dev)]

 

> -----Original Message-----
> From: Helgaas, Bjorn 
> Sent: Tuesday, July 25, 2006 5:37 PM
> To: Andrew Morton
> Cc: Miller, Mike (OS Dev); ISS StorageDev; 
> linux-kernel@vger.kernel.org
> Subject: [PATCH] CCISS: Don't print driver version until we 
> actually find a device
> 
> If we don't find any devices, we shouldn't print anything.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Acked-by: Mike Miller <mike.miller@hp.com>

> 
> Index: work-mm2/drivers/block/cciss.c
> ===================================================================
> --- work-mm2.orig/drivers/block/cciss.c	2006-07-20 
> 16:27:34.000000000 -0600
> +++ work-mm2/drivers/block/cciss.c	2006-07-25 
> 16:16:27.000000000 -0600
> @@ -3109,12 +3109,16 @@
>  static int __devinit cciss_init_one(struct pci_dev *pdev,
>  				    const struct pci_device_id *ent)  {
> +	static int cciss_version_printed = 0;
>  	request_queue_t *q;
>  	int i;
>  	int j;
>  	int rc;
>  	int dac;
>  
> +	if (cciss_version_printed++ == 0)
> +		printk(KERN_INFO DRIVER_NAME "\n");
> +
>  	i = alloc_cciss_hba();
>  	if (i < 0)
>  		return -1;
> @@ -3370,9 +3374,6 @@
>   */
>  static int __init cciss_init(void)
>  {
> -	printk(KERN_INFO DRIVER_NAME "\n");
> -
> -	/* Register for our PCI devices */
>  	return pci_register_driver(&cciss_pci_driver);
>  }
>  
> 

Re: [PATCH] CCISS: Don't print driver version until we actually find a device

[Bodo Eggert]

Jesper Juhl <jesper.juhl@gmail.com> wrote:
> On 26/07/06, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:

>> If we don't find any devices, we shouldn't print anything.
>>
> I disagree.
> I find it quite nice to be able to see that the driver loaded even if
> it finds nothing. At least then when there's a problem, I can quickly
> see that at least it is not because I didn't forget to load the
> driver, it's something else. Saves time since I can start looking for
> reasons why the driver didn't find anything without first spending
> additional time checking if I failed to cause it to load for some
> reason.

I disagree differently: the driver version is a function of the kernel
version, and the load status can easily be obtained. Therefore I suggest
never printing the version number from in-kernel drivers.

Maybe adding a debug kernel parameter that will cause module loads to be more
verbose for all modules is more reasonable.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

http://david.woodhou.se/why-not-spf.html