(Jeff has already merged this in his tree.)
From: Borislav Petkov <[email protected]>
libata new debugging macro definitions
Signed-off-by: Borislav Petkov <[email protected]>
Signed-off-by: Randy Dunlap <[email protected]>
---
include/linux/libata.h | 52 ++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 45 insertions(+), 7 deletions(-)
--- linux-2616-rc4-ata.orig/include/linux/libata.h
+++ linux-2616-rc4-ata/include/linux/libata.h
@@ -36,7 +36,8 @@
#include <acpi/acpi.h>
/*
- * compile-time options
+ * compile-time options: to be removed as soon as all the drivers are
+ * converted to the new debugging mechanism
*/
#undef ATA_DEBUG /* debugging output */
#undef ATA_VERBOSE_DEBUG /* yet more debugging output */
@@ -72,6 +73,38 @@
}
#endif
+/* NEW: debug levels */
+#define HAVE_LIBATA_MSG 1
+
+enum {
+ ATA_MSG_DRV = 0x0001,
+ ATA_MSG_INFO = 0x0002,
+ ATA_MSG_PROBE = 0x0004,
+ ATA_MSG_WARN = 0x0008,
+ ATA_MSG_MALLOC = 0x0010,
+ ATA_MSG_CTL = 0x0020,
+ ATA_MSG_INTR = 0x0040,
+ ATA_MSG_ERR = 0x0080,
+};
+
+#define ata_msg_drv(p) ((p)->msg_enable & ATA_MSG_DRV)
+#define ata_msg_info(p) ((p)->msg_enable & ATA_MSG_INFO)
+#define ata_msg_probe(p) ((p)->msg_enable & ATA_MSG_PROBE)
+#define ata_msg_warn(p) ((p)->msg_enable & ATA_MSG_WARN)
+#define ata_msg_malloc(p) ((p)->msg_enable & ATA_MSG_MALLOC)
+#define ata_msg_ctl(p) ((p)->msg_enable & ATA_MSG_CTL)
+#define ata_msg_intr(p) ((p)->msg_enable & ATA_MSG_INTR)
+#define ata_msg_err(p) ((p)->msg_enable & ATA_MSG_ERR)
+
+static inline u32 ata_msg_init(int dval, int default_msg_enable_bits)
+{
+ if (dval < 0 || dval >= (sizeof(u32) * 8))
+ return default_msg_enable_bits; /* should be 0x1 - only driver info msgs */
+ if (!dval)
+ return 0;
+ return (1 << dval) - 1;
+}
+
/* defines only for the constants which don't work well as enums */
#define ATA_TAG_POISON 0xfafbfcfdU
@@ -365,6 +398,8 @@ struct ata_port {
unsigned int hsm_task_state;
unsigned long pio_task_timeout;
+ u32 msg_enable;
+
void *private_data;
};
@@ -651,9 +686,9 @@ static inline u8 ata_wait_idle(struct at
if (status & (ATA_BUSY | ATA_DRQ)) {
unsigned long l = ap->ioaddr.status_addr;
- printk(KERN_WARNING
- "ATA: abnormal status 0x%X on port 0x%lX\n",
- status, l);
+ if (ata_msg_warn(ap))
+ printk(KERN_WARNING "ATA: abnormal status 0x%X on port 0x%lX\n",
+ status, l);
}
return status;
@@ -745,7 +780,8 @@ static inline u8 ata_irq_ack(struct ata_
status = ata_busy_wait(ap, bits, 1000);
if (status & bits)
- DPRINTK("abnormal status 0x%X\n", status);
+ if (ata_msg_err(ap))
+ printk(KERN_ERR "abnormal status 0x%X\n", status);
/* get controller status; clear intr, err bits */
if (ap->flags & ATA_FLAG_MMIO) {
@@ -763,8 +799,10 @@ static inline u8 ata_irq_ack(struct ata_
post_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
}
- VPRINTK("irq ack: host_stat 0x%X, new host_stat 0x%X, drv_stat 0x%X\n",
- host_stat, post_stat, status);
+ if (ata_msg_intr(ap))
+ printk(KERN_INFO "%s: irq ack: host_stat 0x%X, new host_stat 0x%X, drv_stat 0x%X\n",
+ __FUNCTION__,
+ host_stat, post_stat, status);
return status;
}
> --- linux-2616-rc4-ata.orig/include/linux/libata.h
> +++ linux-2616-rc4-ata/include/linux/libata.h
> @@ -36,7 +36,8 @@
> #include <acpi/acpi.h>
>
> /*
> - * compile-time options
> + * compile-time options: to be removed as soon as all the drivers are
> + * converted to the new debugging mechanism
> */
> #undef ATA_DEBUG /* debugging output */
> #undef ATA_VERBOSE_DEBUG /* yet more debugging output */
> @@ -72,6 +73,38 @@
> }
> #endif
>
> +/* NEW: debug levels */
> +#define HAVE_LIBATA_MSG 1
What is new with them?
> +enum {
> + ATA_MSG_DRV = 0x0001,
> + ATA_MSG_INFO = 0x0002,
> + ATA_MSG_PROBE = 0x0004,
> + ATA_MSG_WARN = 0x0008,
> + ATA_MSG_MALLOC = 0x0010,
> + ATA_MSG_CTL = 0x0020,
> + ATA_MSG_INTR = 0x0040,
> + ATA_MSG_ERR = 0x0080,
> +};
> +
> +#define ata_msg_drv(p) ((p)->msg_enable & ATA_MSG_DRV)
> +#define ata_msg_info(p) ((p)->msg_enable & ATA_MSG_INFO)
> +#define ata_msg_probe(p) ((p)->msg_enable & ATA_MSG_PROBE)
> +#define ata_msg_warn(p) ((p)->msg_enable & ATA_MSG_WARN)
> +#define ata_msg_malloc(p) ((p)->msg_enable & ATA_MSG_MALLOC)
> +#define ata_msg_ctl(p) ((p)->msg_enable & ATA_MSG_CTL)
> +#define ata_msg_intr(p) ((p)->msg_enable & ATA_MSG_INTR)
> +#define ata_msg_err(p) ((p)->msg_enable & ATA_MSG_ERR)
I hate to see debugging infrastructure like this. We already have it
in ACPI and it is nasty/useless. It hides serious errors during normal
run, while if you turn on the debugging, it floods logs so that
it is unusable, too. I end up having to replace dprintks with
printks... nasty.
Pavel
--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...
Pavel Machek wrote:
> I hate to see debugging infrastructure like this. We already have it
> in ACPI and it is nasty/useless. It hides serious errors during normal
> run, while if you turn on the debugging, it floods logs so that
> it is unusable, too. I end up having to replace dprintks with
> printks... nasty.
Then you clearly don't understand what the code is doing. Fine-grained
message selection allows one to turn on only the messages needed, and
only for the controller desired. Otherwise, it is nearly impossible to
debug one SATA controller while booting off another.
Jeff
On ?t 28-02-06 07:00:14, Jeff Garzik wrote:
> Pavel Machek wrote:
> >I hate to see debugging infrastructure like this. We already have it
> >in ACPI and it is nasty/useless. It hides serious errors during normal
> >run, while if you turn on the debugging, it floods logs so that
> >it is unusable, too. I end up having to replace dprintks with
> >printks... nasty.
>
> Then you clearly don't understand what the code is doing.
> Fine-grained
No, I do not... code is so full of printk()s that it is unreadable.
> message selection allows one to turn on only the messages needed, and
> only for the controller desired. Otherwise, it is nearly impossible to
> debug one SATA controller while booting off another.
Now, maybe message selection is neccessary, but having printk at
begining of each function is not way to go.
Pavel
--
Web maintainer for suspend.sf.net (http://www.sf.net/projects/suspend) wanted...
Pavel Machek wrote:
> Now, maybe message selection is neccessary, but having printk at
> begining of each function is not way to go.
Clearly you have not read much libata code at all...
Jeff
Jeff Garzik <[email protected]> wrote:
>
> Fine-grained
> message selection allows one to turn on only the messages needed, and
> only for the controller desired.
Except
- There's (presently) no way of making all the messages go away for a
non-debug build.
- The code is structured as
if (ata_msg_foo(p))
printk("something");
So if we later do
#define ata_msg_foo(p) 0
We'll still get copies of "something" in the kernel image (may be fixed
in later gcc, dunno).
- The new debug stuff isn't documented. One has funble around in the
source to work out how to even turn it on. Can it be altered at runtime?
Dunno - the changelogs are risible. What effect do the various flags
have?
Having spent (and re-spent) time grovelling through the ALSA source
working out how to enable their debug stuff during a maintainer snooze
I'd prefer we didn't have to do that with libata as well.
Andrew Morton wrote:
> Jeff Garzik <[email protected]> wrote:
>
>>Fine-grained
>> message selection allows one to turn on only the messages needed, and
>> only for the controller desired.
>
>
> Except
>
> - There's (presently) no way of making all the messages go away for a
> non-debug build.
They aren't supposed to go away.
> - The code is structured as
>
> if (ata_msg_foo(p))
> printk("something");
>
> So if we later do
>
> #define ata_msg_foo(p) 0
>
> We'll still get copies of "something" in the kernel image (may be fixed
> in later gcc, dunno).
We don't do that in net driver land, and I don't wish to do it for
libata either. Its just a bit test, that jumps over code if the message
class isn't enabled (see link below).
We want users to be able to enable specific messages for specific
controllers, without recompiling their kernel.
grep for msg_enable in various net drivers. ethtool(8) is used to
select specific controllers and messages to print.
> - The new debug stuff isn't documented. One has funble around in the
> source to work out how to even turn it on. Can it be altered at runtime?
> Dunno - the changelogs are risible. What effect do the various flags
> have?
The model has always been documented:
http://www.scyld.com/pipermail/vortex/2001-November/001426.html
(scroll down a tad)
Jeff
Andrew Morton wrote:
> Jeff Garzik <[email protected]> wrote:
>> Fine-grained
>> message selection allows one to turn on only the messages needed, and
>> only for the controller desired.
>
> Except
>
> - There's (presently) no way of making all the messages go away for a
> non-debug build.
Agreed. We need a way to make them all really go away
for embedded builds -- memory matters there.
Cheers
Andrew Morton wrote:
> Except
>
> - There's (presently) no way of making all the messages go away for a
> non-debug build.
>
I agree, there should be a config option to build the kernel with the
debug support entirely shut off, though it's a good idea to leave it on
if you aren't really cramped for space.
> - The code is structured as
>
> if (ata_msg_foo(p))
> printk("something");
>
> So if we later do
>
> #define ata_msg_foo(p) 0
>
> We'll still get copies of "something" in the kernel image (may be fixed
> in later gcc, dunno).
>
> - The new debug stuff isn't documented. One has funble around in the
> source to work out how to even turn it on. Can it be altered at runtime?
> Dunno - the changelogs are risible. What effect do the various flags
> have?
>
> Having spent (and re-spent) time grovelling through the ALSA source
> working out how to enable their debug stuff during a maintainer snooze
> I'd prefer we didn't have to do that with libata as well.
>
Would you prefer there not be any debug messages at all, rather than
ones you have to figure out how to turn on and interpret? Documentation
is always a good thing, but if you are at least somewhat familiar with
the code, turning on the debug messages should be easy and rather helpful.
BTW, didn't I see something recently in the kernel about a debug fs?
Sounded like that was intended for this sort of thing to provide a
standard interface to configuring fine grained debug message filtering.
Jeff Garzik <[email protected]> wrote:
>
> Andrew Morton wrote:
> > Jeff Garzik <[email protected]> wrote:
> >
> >>Fine-grained
> >> message selection allows one to turn on only the messages needed, and
> >> only for the controller desired.
> >
> >
> > Except
> >
> > - There's (presently) no way of making all the messages go away for a
> > non-debug build.
>
> They aren't supposed to go away.
>
It is legitimate to elect to waste memory on every machine so as to make
the system more easily debugged by remote maintainers. But that's an
unusual choice in the kernel context.
They can still get you by setting CONFIG_PRINTK=n ;)
> > - The code is structured as
> >
> > if (ata_msg_foo(p))
> > printk("something");
> >
> > So if we later do
> >
> > #define ata_msg_foo(p) 0
> >
> > We'll still get copies of "something" in the kernel image (may be fixed
> > in later gcc, dunno).
>
> We don't do that in net driver land, and I don't wish to do it for
> libata either. Its just a bit test, that jumps over code if the message
> class isn't enabled (see link below).
>
> We want users to be able to enable specific messages for specific
> controllers, without recompiling their kernel.
>
> grep for msg_enable in various net drivers. ethtool(8) is used to
> select specific controllers and messages to print.
>
umm, that's unrelated to my point, but whatever.
>
> > - The new debug stuff isn't documented. One has funble around in the
> > source to work out how to even turn it on. Can it be altered at runtime?
> > Dunno - the changelogs are risible. What effect do the various flags
> > have?
>
> The model has always been documented:
> http://www.scyld.com/pipermail/vortex/2001-November/001426.html
> (scroll down a tad)
That's useless.
On Tue, 28 Feb 2006 09:43:54 -0500
Mark Lord <[email protected]> wrote:
> Andrew Morton wrote:
> > Jeff Garzik <[email protected]> wrote:
> >> Fine-grained
> >> message selection allows one to turn on only the messages needed, and
> >> only for the controller desired.
> >
> > Except
> >
> > - There's (presently) no way of making all the messages go away for a
> > non-debug build.
>
> Agreed. We need a way to make them all really go away
> for embedded builds -- memory matters there.
That's a libata infrastructure issue, not an ATA-ACPI issue.
I'll go with whatever $maintainer decides.
~Randy
Andrew Morton wrote:
> Jeff Garzik <[email protected]> wrote:
>
>>Andrew Morton wrote:
>>
>>>Jeff Garzik <[email protected]> wrote:
>>>
>>>
>>>>Fine-grained
>>>>message selection allows one to turn on only the messages needed, and
>>>>only for the controller desired.
>>>
>>>
>>>Except
>>>
>>>- There's (presently) no way of making all the messages go away for a
>>> non-debug build.
>>
>>They aren't supposed to go away.
>>
>
>
> It is legitimate to elect to waste memory on every machine so as to make
> the system more easily debugged by remote maintainers. But that's an
> unusual choice in the kernel context.
Not unusual, as I said, its done in a ton of net drivers.
That said, I suppose its OK to do
#define ata_msg_foo() 0
for wacky embedded situations. But the default will be enabled for all
users, not just debug kernels.
>>>- The new debug stuff isn't documented. One has funble around in the
>>> source to work out how to even turn it on. Can it be altered at runtime?
>>> Dunno - the changelogs are risible. What effect do the various flags
>>> have?
>>
>>The model has always been documented:
>>http://www.scyld.com/pipermail/vortex/2001-November/001426.html
>>(scroll down a tad)
>
>
> That's useless.
Not useless at all: It documents the model that is being implemented
quite well. libata will use the same method of bitmasks, same method of
increasing verbosity as set by debug level, same method of masking the
more verbose messages by default, but always compiling the messages into
the driver. Its highly similar.
Jeff
Andrew Morton wrote:
> - The new debug stuff isn't documented. One has funble around in the
> source to work out how to even turn it on. Can it be altered at runtime?
> Dunno - the changelogs are risible. What effect do the various flags
> have?
>
> Having spent (and re-spent) time grovelling through the ALSA source
> working out how to enable their debug stuff during a maintainer snooze
> I'd prefer we didn't have to do that with libata as well.
>
>
Is there a particular debugging coding style that we should adopt for
all the kernel code.
For example,
kconfig option in order to compile a module/section of core code for
debug work.
A sysfs file to then control the debug level for each module.
A debug module option, in the cases where a particular level of debug is
required at module load time, and before the sysfs entry exists.
If particularly fine grained debug control is needed, the module could
have multiple entries in the sysfs to control different classes of debug
output.
One then implements all this debug support code at a global level,
making it easy for each kernel module to make use of it.
With regard to ALSA, we can make it fit in with the kernel preferred method.
James
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.
James Courtier-Dutton <[email protected]> wrote:
>
> Is there a particular debugging coding style that we should adopt for
> all the kernel code.
Err, probably. But we'd need to have a 1000-email argument first.
Right now many subsystems and often many individual drivers go and
implement their own set of debugging macros and knobs to twiddle. This was
a great source of fun for me in trying to support gcc-2.95.x - each time a
new debug macro got implemented I had to go in there (again) and apply the
gcc-2.95.x-macro-expansion-bug-workaround to it.
Yes, one common toolset with a common way of controlling it would be much
more sensible than the present chaos. I count 163 separate definitions of
dprintk(), and that's excluding all the non-x86 arch and include dirs.
> For example,
> kconfig option in order to compile a module/section of core code for
> debug work.
> A sysfs file to then control the debug level for each module.
> A debug module option, in the cases where a particular level of debug is
> required at module load time, and before the sysfs entry exists.
> If particularly fine grained debug control is needed, the module could
> have multiple entries in the sysfs to control different classes of debug
> output.
>
Something like that.. Just don't cc me while you work it out ;)