Hi Bart,

here's something i've been wanting to do for a long time: debugging macros. The
reason for it is that i got tired of adding debug printk's everytime i'm testing
something so here we go.

The debugging macro is similar to the old ones but is one for all drivers
(currently only ide-floppy), is nice on branch prediction and is controlled by a
drive->debug_mask switch which is a module parameter and as such can be set at
module load time, of course. I've been thinking of adding also a sysfs attribute
too but can't seem to find quite the justification for it so no sysfs for now :)

In addition, one can still optimize away all the debug calls in the old manner
and i'm sure those will be removed completely too when ide generic conversion is
done.

Please tell me what you think, what can be changed/improved and after we've
figured out the details I'll do the other drivers too.

Thanks.

 drivers/ide/ide-cd.c     |   17 ++---
 drivers/ide/ide-floppy.c |  168 +++++++++++++++++++++++++---------------------
 drivers/ide/ide-tape.c   |   23 ++----
 include/linux/ide.h      |   31 ++++++++-
 4 files changed, 135 insertions(+), 104 deletions(-)
--

[ message continues ]

Add a debugging on/off switch for controlling driver debugging
messages dynamically.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 include/linux/ide.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/linux/ide.h b/include/linux/ide.h
index 6b5d425..c161840 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -543,6 +543,9 @@ struct ide_drive_s {
 
 	int		lun;		/* logical unit */
 	int		crc_count;	/* crc counter to reduce drive speed */
+
+	unsigned long	debug_mask;	/* debugging levels switch */
+
 #ifdef CONFIG_BLK_DEV_IDEACPI
 	struct ide_acpi_drive_link *acpidata;
 #endif
-- 
1.5.5.4

--

[ message continues ]

Add __ide_debug_log() debugging macro which is controlled by drive->debug_mask.
The macro has to have the macro DRV_NAME defined in each driver before use.
Also, add different debugging levels depending on the functionality debugged.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 include/linux/ide.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/linux/ide.h b/include/linux/ide.h
index c161840..b6d714d 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -925,6 +925,26 @@ static inline void ide_proc_unregister_driver(ide_drive_t *drive, ide_driver_t *
 #define PROC_IDE_READ_RETURN(page,start,off,count,eof,len) return 0;
 #endif
 
+enum {
+	/* enter/exit functions */
+	IDE_DBG_FUNC =			(1 << 0),
+	/* sense key/asc handling */
+	IDE_DBG_SENSE =			(1 << 1),
+	/* packet commands handling */
+	IDE_DBG_PC =			(1 << 2),
+	/* request handling */
+	IDE_DBG_RQ =			(1 << 3),
+	/* driver probing/setup */
+	IDE_DBG_PROBE =			(1 << 4),
+};
+
+/* DRV_NAME has to be defined in the driver before using the macro below */
+#define __ide_debug_log(lvl, fmt, args...)			\
+{								\
+	if (unlikely(drive->debug_mask & lvl))			\
+		printk(KERN_INFO DRV_NAME ": " fmt, ## args);	\
+}
+
 /*
  * Power Management step value (rq->pm->pm_step).
  *
-- 
1.5.5.4

--

[ message continues ]

Shouldn't a debug printk use KERN_DEBUG?

--

[ message continues ]

Yes, it should. However, i'm going to have to change the default console
loglevel to see those, aka boot with loglevel=7 and there's the possibility of
other debug messages getting in the way too. Instead, the debugging messages
here are enabled only when you do a debug build of the driver _and_ when enable
them through the debug_mask so you get to see debug messages only you're
interested in without changing anything else in the kernel boot.

-- 
Regards/Gruss,
    Boris.
--

[ message continues ]

Introduce to_ide_dev() and ide_drv_g() macros and replace the respective
definitions of similar ones in each driver.

There should be no functionality change resulting from this patch.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-cd.c     |   17 ++++++-----------
 drivers/ide/ide-floppy.c |   21 +++++++++------------
 drivers/ide/ide-tape.c   |   23 ++++++++---------------
 include/linux/ide.h      |    8 +++++++-
 4 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 5c23ec9..5e675f3 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -52,11 +52,6 @@
 
 static DEFINE_MUTEX(idecd_ref_mutex);
 
-#define to_ide_cd(obj) container_of(obj, struct cdrom_info, kref)
-
-#define ide_cd_g(disk) \
-	container_of((disk)->private_data, struct cdrom_info, driver)
-
 static void ide_cd_release(struct kref *);
 
 static struct cdrom_info *ide_cd_get(struct gendisk *disk)
@@ -64,7 +59,7 @@ static struct cdrom_info *ide_cd_get(struct gendisk *disk)
 	struct cdrom_info *cd = NULL;
 
 	mutex_lock(&idecd_ref_mutex);
-	cd = ide_cd_g(disk);
+	cd = ide_drv_g(disk, cdrom_info);
 	if (cd) {
 		if (ide_device_get(cd->drive))
 			cd = NULL;
@@ -1937,7 +1932,7 @@ static void ide_cd_remove(ide_drive_t *drive)
 
 static void ide_cd_release(struct kref *kref)
 {
-	struct cdrom_info *info = to_ide_cd(kref);
+	struct cdrom_info *info = to_ide_drv(kref, cdrom_info);
 	struct cdrom_device_info *devinfo = &info->devinfo;
 	ide_drive_t *drive = info->drive;
 	struct gendisk *g = info->disk;
@@ -1995,7 +1990,7 @@ static int idecd_open(struct inode *inode, struct file *file)
 static int idecd_release(struct inode *inode, struct file *file)
 {
 	struct gendisk *disk = inode->i_bdev->bd_disk;
-	struct cdrom_info *info = ide_cd_g(disk);
+	struct cdrom_info *info = ide_drv_g(disk, cdrom_info);
 
 	cdrom_release(&info->devinfo, file);
 
@@ -2047,7 +2042,7 @@ static int idecd_ioctl(struct inode ...
[ message continues ]

Also:

- leave in the possibility for optimizing away all debugging macros
- add a PFX macro and prepend all printk calls with it for consistency
- change idefloppy_create_rw_cmd's 1st arg from idefloppy_floppy_t * to
ide_drive_t *.
- add a missing printk-level in idefloppy_init
- fix minor checkpatch warnings

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-floppy.c |  140 ++++++++++++++++++++++++---------------------
 1 files changed, 75 insertions(+), 65 deletions(-)

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 2a54a25..3de9478 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -16,6 +16,7 @@
  */
 
 #define DRV_NAME "ide-floppy"
+#define PFX DRV_NAME ": "
 
 #define IDEFLOPPY_VERSION "1.00"
 
@@ -49,16 +50,12 @@
 #include "ide-floppy.h"
 
 /* define to see debug info */
-#define IDEFLOPPY_DEBUG_LOG		0
-
-/* #define IDEFLOPPY_DEBUG(fmt, args...) printk(KERN_INFO fmt, ## args) */
-#define IDEFLOPPY_DEBUG(fmt, args...)
+#define IDEFLOPPY_DEBUG_LOG	0
 
 #if IDEFLOPPY_DEBUG_LOG
-#define debug_log(fmt, args...) \
-	printk(KERN_INFO "ide-floppy: " fmt, ## args)
+#define ide_debug_log(lvl, fmt, args...) __ide_debug_log(lvl, fmt, args)
 #else
-#define debug_log(fmt, args...) do {} while (0)
+#define ide_debug_log(lvl, fmt, args...) do {} while (0)
 #endif
 
 /*
@@ -122,13 +119,21 @@ static int idefloppy_end_request(ide_drive_t *drive, int uptodate, int nsecs)
 	struct request *rq = HWGROUP(drive)->rq;
 	int error;
 
-	debug_log("Reached %s\n", __func__);
+	ide_debug_log(IDE_DBG_FUNC, "Call %s\n", __func__);
 
 	switch (uptodate) {
-	case 0: error = IDEFLOPPY_ERROR_GENERAL; break;
-	case 1: error = 0; break;
-	default: error = uptodate;
+	case 0:
+		error = IDEFLOPPY_ERROR_GENERAL;
+		break;
+
+	case 1:
+		error = 0;
+		break;
+
+	default:
+		error = uptodate;
 	}
+
 	if (error)
 		floppy->failed_pc = NULL;
 	/* Why does this happen? */
@@ -161,7 +166,7 @@ static void ...
[ message continues ]

... with which to control to verbosity of debug messages on module load time.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-floppy.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 3de9478..85022b8 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -49,6 +49,9 @@
 
 #include "ide-floppy.h"
 
+/* module parameters */
+static unsigned long debug_mask = 0x0;
+
 /* define to see debug info */
 #define IDEFLOPPY_DEBUG_LOG	0
 
@@ -679,6 +682,8 @@ static ide_driver_t idefloppy_driver = {
 #endif
 };
 
+module_param(debug_mask, ulong, 0644);
+
 static int idefloppy_open(struct inode *inode, struct file *filp)
 {
 	struct gendisk *disk = inode->i_bdev->bd_disk;
@@ -692,6 +697,8 @@ static int idefloppy_open(struct inode *inode, struct file *filp)
 
 	drive = floppy->drive;
 
+	drive->debug_mask = debug_mask;
+
 	ide_debug_log(IDE_DBG_FUNC, "Call %s\n", __func__);
 
 	floppy->openers++;
-- 
1.5.5.4

--

[ message continues ]

Hi,


if you look closely you should already find it :)


Looks pretty good to me (after quick look) and I applied to pata tree.

PS I wonder whether we may also want to have global debug_mask for core code
messages ('drive' is not always available) in the longer-term.  Or maybe even
only global debug_mask for both core code and driver messages?

Thanks,
Bart
--

[ message continues ]

Together with sysfs device-driver binding/unbinding support it
can probably substitute for lack of sysfs attribute for now. ;)

Anyway adding a proper device sysfs attribute will be trivial
(ide_dev_attrs[]) but lets wait with it until things settle down.

Thanks,
Bart
--

[ message continues ]

Those I saw. (You mean /sys/bus/ide/drivers/ide-cdrom/, for example, right?)
But if so, those come from the gen_driver's ide_bus_type and those are device
attributes and I was thinking of adding ide_bus_type.drv_attrs which are driver-
and not device-specific but since i haven't read into the sysfs code too much

Let me answer you with a question :) : is a u64 bitmask going to be enough for
all the debugging levels, both core and drivers ? The way I see it, hmm, probably ...

-- 
Regards/Gruss,
    Boris.
--

[ message continues ]

Aah, i get it now, it's /sys/bus/ide/drivers/ide-floppy/module/parameters/debug_mask.
However, it'd be better if it were /sys/bus/ide/drivers/ide-floppy/debug_mask.
Probably a symlink or a true /sysfs parameter? Maybe later :).

-- 
Regards/Gruss,
    Boris.
--

[ message continues ]