[PATCH 0/5] ide debugging macros

[PATCH 0/5] ide debugging macros

[Borislav Petkov]

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(-)

[PATCH 1/5] ide: unify conversion macros

[Borislav Petkov]

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 *inode, struct file *file,
 			unsigned int cmd, unsigned long arg)
 {
 	struct block_device *bdev = inode->i_bdev;
-	struct cdrom_info *info = ide_cd_g(bdev->bd_disk);
+	struct cdrom_info *info = ide_drv_g(bdev->bd_disk, cdrom_info);
 	int err;
 
 	switch (cmd) {
@@ -2068,13 +2063,13 @@ static int idecd_ioctl(struct inode *inode, struct file *file,
 
 static int idecd_media_changed(struct gendisk *disk)
 {
-	struct cdrom_info *info = ide_cd_g(disk);
+	struct cdrom_info *info = ide_drv_g(disk, cdrom_info);
 	return cdrom_media_changed(&info->devinfo);
 }
 
 static int idecd_revalidate_disk(struct gendisk *disk)
 {
-	struct cdrom_info *info = ide_cd_g(disk);
+	struct cdrom_info *info = ide_drv_g(disk, cdrom_info);
 	struct request_sense sense;
 
 	ide_cd_read_toc(info->drive, &sense);
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 2a34f1a..2a54a25 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -84,11 +84,6 @@
 
 static DEFINE_MUTEX(idefloppy_ref_mutex);
 
-#define to_ide_floppy(obj) container_of(obj, struct ide_floppy_obj, kref)
-
-#define ide_floppy_g(disk) \
-	container_of((disk)->private_data, struct ide_floppy_obj, driver)
-
 static void idefloppy_cleanup_obj(struct kref *);
 
 static struct ide_floppy_obj *ide_floppy_get(struct gendisk *disk)
@@ -96,7 +91,7 @@ static struct ide_floppy_obj *ide_floppy_get(struct gendisk *disk)
 	struct ide_floppy_obj *floppy = NULL;
 
 	mutex_lock(&idefloppy_ref_mutex);
-	floppy = ide_floppy_g(disk);
+	floppy = ide_drv_g(disk, ide_floppy_obj);
 	if (floppy) {
 		if (ide_device_get(floppy->drive))
 			floppy = NULL;
@@ -625,7 +620,7 @@ static void ide_floppy_remove(ide_drive_t *drive)
 
 static void idefloppy_cleanup_obj(struct kref *kref)
 {
-	struct ide_floppy_obj *floppy = to_ide_floppy(kref);
+	struct ide_floppy_obj *floppy = to_ide_drv(kref, ide_floppy_obj);
 	ide_drive_t *drive = floppy->drive;
 	struct gendisk *g = floppy->disk;
 
@@ -733,7 +728,7 @@ out_put_floppy:
 static int idefloppy_release(struct inode *inode, struct file *filp)
 {
 	struct gendisk *disk = inode->i_bdev->bd_disk;
-	struct ide_floppy_obj *floppy = ide_floppy_g(disk);
+	struct ide_floppy_obj *floppy = ide_drv_g(disk, ide_floppy_obj);
 	ide_drive_t *drive = floppy->drive;
 
 	debug_log("Reached %s\n", __func__);
@@ -752,7 +747,8 @@ static int idefloppy_release(struct inode *inode, struct file *filp)
 
 static int idefloppy_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 {
-	struct ide_floppy_obj *floppy = ide_floppy_g(bdev->bd_disk);
+	struct ide_floppy_obj *floppy = ide_drv_g(bdev->bd_disk,
+						     ide_floppy_obj);
 	ide_drive_t *drive = floppy->drive;
 
 	geo->heads = drive->bios_head;
@@ -783,7 +779,8 @@ static int idefloppy_ioctl(struct inode *inode, struct file *file,
 			unsigned int cmd, unsigned long arg)
 {
 	struct block_device *bdev = inode->i_bdev;
-	struct ide_floppy_obj *floppy = ide_floppy_g(bdev->bd_disk);
+	struct ide_floppy_obj *floppy = ide_drv_g(bdev->bd_disk,
+						     ide_floppy_obj);
 	ide_drive_t *drive = floppy->drive;
 	struct ide_atapi_pc pc;
 	void __user *argp = (void __user *)arg;
@@ -812,7 +809,7 @@ static int idefloppy_ioctl(struct inode *inode, struct file *file,
 
 static int idefloppy_media_changed(struct gendisk *disk)
 {
-	struct ide_floppy_obj *floppy = ide_floppy_g(disk);
+	struct ide_floppy_obj *floppy = ide_drv_g(disk, ide_floppy_obj);
 	ide_drive_t *drive = floppy->drive;
 	int ret;
 
@@ -828,7 +825,7 @@ static int idefloppy_media_changed(struct gendisk *disk)
 
 static int idefloppy_revalidate_disk(struct gendisk *disk)
 {
-	struct ide_floppy_obj *floppy = ide_floppy_g(disk);
+	struct ide_floppy_obj *floppy = ide_drv_g(disk, ide_floppy_obj);
 	set_capacity(disk, idefloppy_capacity(floppy->drive));
 	return 0;
 }
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 417e600..9ee93db 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -267,11 +267,6 @@ static DEFINE_MUTEX(idetape_ref_mutex);
 
 static struct class *idetape_sysfs_class;
 
-#define to_ide_tape(obj) container_of(obj, struct ide_tape_obj, kref)
-
-#define ide_tape_g(disk) \
-	container_of((disk)->private_data, struct ide_tape_obj, driver)
-
 static void ide_tape_release(struct kref *);
 
 static struct ide_tape_obj *ide_tape_get(struct gendisk *disk)
@@ -279,7 +274,7 @@ static struct ide_tape_obj *ide_tape_get(struct gendisk *disk)
 	struct ide_tape_obj *tape = NULL;
 
 	mutex_lock(&idetape_ref_mutex);
-	tape = ide_tape_g(disk);
+	tape = ide_drv_g(disk, ide_tape_obj);
 	if (tape) {
 		if (ide_device_get(tape->drive))
 			tape = NULL;
@@ -306,8 +301,6 @@ static void ide_tape_put(struct ide_tape_obj *tape)
  */
 static struct ide_tape_obj *idetape_devs[MAX_HWIFS * MAX_DRIVES];
 
-#define ide_tape_f(file) ((file)->private_data)
-
 static struct ide_tape_obj *ide_tape_chrdev_get(unsigned int i)
 {
 	struct ide_tape_obj *tape = NULL;
@@ -1541,7 +1534,7 @@ static int idetape_space_over_filemarks(ide_drive_t *drive, short mt_op,
 static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
 				   size_t count, loff_t *ppos)
 {
-	struct ide_tape_obj *tape = ide_tape_f(file);
+	struct ide_tape_obj *tape = file->private_data;
 	ide_drive_t *drive = tape->drive;
 	ssize_t bytes_read, temp, actually_read = 0, rc;
 	ssize_t ret = 0;
@@ -1603,7 +1596,7 @@ finish:
 static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
 				     size_t count, loff_t *ppos)
 {
-	struct ide_tape_obj *tape = ide_tape_f(file);
+	struct ide_tape_obj *tape = file->private_data;
 	ide_drive_t *drive = tape->drive;
 	ssize_t actually_written = 0;
 	ssize_t ret = 0;
@@ -1835,7 +1828,7 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)
 static int idetape_chrdev_ioctl(struct inode *inode, struct file *file,
 				unsigned int cmd, unsigned long arg)
 {
-	struct ide_tape_obj *tape = ide_tape_f(file);
+	struct ide_tape_obj *tape = file->private_data;
 	ide_drive_t *drive = tape->drive;
 	struct mtop mtop;
 	struct mtget mtget;
@@ -2012,7 +2005,7 @@ static void idetape_write_release(ide_drive_t *drive, unsigned int minor)
 
 static int idetape_chrdev_release(struct inode *inode, struct file *filp)
 {
-	struct ide_tape_obj *tape = ide_tape_f(filp);
+	struct ide_tape_obj *tape = filp->private_data;
 	ide_drive_t *drive = tape->drive;
 	unsigned int minor = iminor(inode);
 
@@ -2271,7 +2264,7 @@ static void ide_tape_remove(ide_drive_t *drive)
 
 static void ide_tape_release(struct kref *kref)
 {
-	struct ide_tape_obj *tape = to_ide_tape(kref);
+	struct ide_tape_obj *tape = to_ide_drv(kref, ide_tape_obj);
 	ide_drive_t *drive = tape->drive;
 	struct gendisk *g = tape->disk;
 
@@ -2354,7 +2347,7 @@ static int idetape_open(struct inode *inode, struct file *filp)
 static int idetape_release(struct inode *inode, struct file *filp)
 {
 	struct gendisk *disk = inode->i_bdev->bd_disk;
-	struct ide_tape_obj *tape = ide_tape_g(disk);
+	struct ide_tape_obj *tape = ide_drv_g(disk, ide_tape_obj);
 
 	ide_tape_put(tape);
 
@@ -2365,7 +2358,7 @@ static int idetape_ioctl(struct inode *inode, struct file *file,
 			unsigned int cmd, unsigned long arg)
 {
 	struct block_device *bdev = inode->i_bdev;
-	struct ide_tape_obj *tape = ide_tape_g(bdev->bd_disk);
+	struct ide_tape_obj *tape = ide_drv_g(bdev->bd_disk, ide_tape_obj);
 	ide_drive_t *drive = tape->drive;
 	int err = generic_ide_ioctl(drive, file, bdev, cmd, arg);
 	if (err == -EINVAL)
diff --git a/include/linux/ide.h b/include/linux/ide.h
index 2c28f9a..6b5d425 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -568,7 +568,13 @@ struct ide_drive_s {
 
 typedef struct ide_drive_s ide_drive_t;
 
-#define to_ide_device(dev)container_of(dev, ide_drive_t, gendev)
+#define to_ide_device(dev)		container_of(dev, ide_drive_t, gendev)
+
+#define to_ide_drv(obj, cont_type)	\
+	container_of(obj, struct cont_type, kref)
+
+#define ide_drv_g(disk, cont_type)	\
+	container_of((disk)->private_data, struct cont_type, driver)
 
 struct ide_task_s;
 struct ide_port_info;
-- 
1.5.5.4

[PATCH 2/5] ide: add drive->debug_mask switch

[Borislav Petkov]

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

[PATCH 3/5] ide: add a driver-wide debugging macro

[Borislav Petkov]

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

Re: [PATCH 3/5] ide: add a driver-wide debugging macro

[Joe Perches]

On Sun, 2008-08-17 at 19:23 +0200, Borislav Petkov wrote:
> 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
> []
> +/* 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);	\
> +}

Shouldn't a debug printk use KERN_DEBUG?

Re: [PATCH 3/5] ide: add a driver-wide debugging macro

[Borislav Petkov]

On Sun, Aug 17, 2008 at 11:13:06AM -0700, Joe Perches wrote:
> On Sun, 2008-08-17 at 19:23 +0200, Borislav Petkov wrote:
> > 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
> > []
> > +/* 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);	\
> > +}
> 
> Shouldn't a debug printk use KERN_DEBUG?

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.

[PATCH 4/5] ide-floppy: convert driver to the new debugging macro

[Borislav Petkov]

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 ide_floppy_callback(ide_drive_t *drive, int dsc)
 	struct ide_atapi_pc *pc = drive->pc;
 	int uptodate = pc->error ? 0 : 1;
 
-	debug_log("Reached %s\n", __func__);
+	ide_debug_log(IDE_DBG_FUNC, "Call %s\n", __func__);
 
 	if (floppy->failed_pc == pc)
 		floppy->failed_pc = NULL;
@@ -180,13 +185,15 @@ static void ide_floppy_callback(ide_drive_t *drive, int dsc)
 				(u16)get_unaligned((u16 *)&buf[16]) : 0x10000;
 
 			if (floppy->failed_pc)
-				debug_log("pc = %x, ", floppy->failed_pc->c[0]);
+				ide_debug_log(IDE_DBG_PC, "pc = %x, ",
+					      floppy->failed_pc->c[0]);
 
-			debug_log("sense key = %x, asc = %x, ascq = %x\n",
-				  floppy->sense_key, floppy->asc, floppy->ascq);
+			ide_debug_log(IDE_DBG_SENSE, "sense key = %x, asc = %x,"
+				      "ascq = %x\n", floppy->sense_key,
+				      floppy->asc, floppy->ascq);
 		} else
-			printk(KERN_ERR "Error in REQUEST SENSE itself - "
-					"Aborting request!\n");
+			printk(KERN_ERR PFX "Error in REQUEST SENSE itself - "
+			       "Aborting request!\n");
 	}
 
 	idefloppy_end_request(drive, uptodate, 0);
@@ -201,7 +208,7 @@ static void ide_floppy_report_error(idefloppy_floppy_t *floppy,
 	    floppy->ascq      == 0x00)
 		return;
 
-	printk(KERN_ERR "ide-floppy: %s: I/O error, pc = %2x, key = %2x, "
+	printk(KERN_ERR PFX "%s: I/O error, pc = %2x, key = %2x, "
 			"asc = %2x, ascq = %2x\n",
 			floppy->drive->name, pc->c[0], floppy->sense_key,
 			floppy->asc, floppy->ascq);
@@ -231,7 +238,7 @@ static ide_startstop_t idefloppy_issue_pc(ide_drive_t *drive,
 		return ide_stopped;
 	}
 
-	debug_log("Retry number - %d\n", pc->retries);
+	ide_debug_log(IDE_DBG_FUNC, "%s: Retry #%d\n", __func__, pc->retries);
 
 	pc->retries++;
 
@@ -265,23 +272,23 @@ void ide_floppy_create_mode_sense_cmd(struct ide_atapi_pc *pc, u8 page_code)
 		length += 32;
 		break;
 	default:
-		printk(KERN_ERR "ide-floppy: unsupported page code "
-				"in create_mode_sense_cmd\n");
+		printk(KERN_ERR PFX "unsupported page code in %s\n", __func__);
 	}
 	put_unaligned(cpu_to_be16(length), (u16 *) &pc->c[7]);
 	pc->req_xfer = length;
 }
 
-static void idefloppy_create_rw_cmd(idefloppy_floppy_t *floppy,
+static void idefloppy_create_rw_cmd(ide_drive_t *drive,
 				    struct ide_atapi_pc *pc, struct request *rq,
 				    unsigned long sector)
 {
+	idefloppy_floppy_t *floppy = drive->driver_data;
 	int block = sector / floppy->bs_factor;
 	int blocks = rq->nr_sectors / floppy->bs_factor;
 	int cmd = rq_data_dir(rq);
 
-	debug_log("create_rw10_cmd: block == %d, blocks == %d\n",
-		block, blocks);
+	ide_debug_log(IDE_DBG_FUNC, "%s: block: %d, blocks: %d\n", __func__,
+		      block, blocks);
 
 	ide_init_pc(pc);
 	pc->c[0] = cmd == READ ? GPCMD_READ_10 : GPCMD_WRITE_10;
@@ -326,41 +333,42 @@ static ide_startstop_t idefloppy_do_request(ide_drive_t *drive,
 	struct ide_atapi_pc *pc;
 	unsigned long block = (unsigned long)block_s;
 
-	debug_log("%s: dev: %s, cmd: 0x%x, cmd_type: %x, errors: %d\n",
-		  __func__, rq->rq_disk ? rq->rq_disk->disk_name : "?",
-		  rq->cmd[0], rq->cmd_type, rq->errors);
+	ide_debug_log(IDE_DBG_FUNC, "%s: dev: %s, cmd: 0x%x, cmd_type: %x, "
+		      "errors: %d\n",
+		      __func__, rq->rq_disk ? rq->rq_disk->disk_name : "?",
+		      rq->cmd[0], rq->cmd_type, rq->errors);
 
-	debug_log("%s: sector: %ld, nr_sectors: %ld, current_nr_sectors: %d\n",
-		  __func__, (long)rq->sector, rq->nr_sectors,
-		  rq->current_nr_sectors);
+	ide_debug_log(IDE_DBG_FUNC, "%s: sector: %ld, nr_sectors: %ld, "
+		      "current_nr_sectors: %d\n",
+		      __func__, (long)rq->sector, rq->nr_sectors,
+		      rq->current_nr_sectors);
 
 	if (rq->errors >= ERROR_MAX) {
 		if (floppy->failed_pc)
 			ide_floppy_report_error(floppy, floppy->failed_pc);
 		else
-			printk(KERN_ERR "ide-floppy: %s: I/O error\n",
-				drive->name);
+			printk(KERN_ERR PFX "%s: I/O error\n", drive->name);
+
 		idefloppy_end_request(drive, 0, 0);
 		return ide_stopped;
 	}
 	if (blk_fs_request(rq)) {
 		if (((long)rq->sector % floppy->bs_factor) ||
 		    (rq->nr_sectors % floppy->bs_factor)) {
-			printk(KERN_ERR "%s: unsupported r/w request size\n",
-					drive->name);
+			printk(KERN_ERR PFX "%s: unsupported r/w rq size\n",
+				drive->name);
 			idefloppy_end_request(drive, 0, 0);
 			return ide_stopped;
 		}
 		pc = &floppy->queued_pc;
-		idefloppy_create_rw_cmd(floppy, pc, rq, block);
+		idefloppy_create_rw_cmd(drive, pc, rq, block);
 	} else if (blk_special_request(rq)) {
 		pc = (struct ide_atapi_pc *) rq->buffer;
 	} else if (blk_pc_request(rq)) {
 		pc = &floppy->queued_pc;
 		idefloppy_blockpc_cmd(floppy, pc, rq);
 	} else {
-		blk_dump_rq_flags(rq,
-			"ide-floppy: unsupported command in queue");
+		blk_dump_rq_flags(rq, PFX "unsupported command in queue");
 		idefloppy_end_request(drive, 0, 0);
 		return ide_stopped;
 	}
@@ -393,8 +401,7 @@ static int ide_floppy_get_flexible_disk_page(ide_drive_t *drive)
 	ide_floppy_create_mode_sense_cmd(&pc, IDEFLOPPY_FLEXIBLE_DISK_PAGE);
 
 	if (ide_queue_pc_tail(drive, disk, &pc)) {
-		printk(KERN_ERR "ide-floppy: Can't get flexible disk page"
-				" parameters\n");
+		printk(KERN_ERR PFX "Can't get flexible disk page params\n");
 		return 1;
 	}
 
@@ -417,7 +424,7 @@ static int ide_floppy_get_flexible_disk_page(ide_drive_t *drive)
 	capacity = cyls * heads * sectors * sector_size;
 
 	if (memcmp(page, &floppy->flexible_disk_page, 32))
-		printk(KERN_INFO "%s: %dkB, %d/%d/%d CHS, %d kBps, "
+		printk(KERN_INFO PFX "%s: %dkB, %d/%d/%d CHS, %d kBps, "
 				"%d sector size, %d rpm\n",
 				drive->name, capacity / 1024, cyls, heads,
 				sectors, transfer_rate / 8, sector_size, rpm);
@@ -429,7 +436,7 @@ static int ide_floppy_get_flexible_disk_page(ide_drive_t *drive)
 	lba_capacity = floppy->blocks * floppy->block_size;
 
 	if (capacity < lba_capacity) {
-		printk(KERN_NOTICE "%s: The disk reports a capacity of %d "
+		printk(KERN_NOTICE PFX "%s: The disk reports a capacity of %d "
 			"bytes, but the drive only handles %d\n",
 			drive->name, lba_capacity, capacity);
 		floppy->blocks = floppy->block_size ?
@@ -459,7 +466,7 @@ static int ide_floppy_get_capacity(ide_drive_t *drive)
 
 	ide_floppy_create_read_capacity_cmd(&pc);
 	if (ide_queue_pc_tail(drive, disk, &pc)) {
-		printk(KERN_ERR "ide-floppy: Can't get floppy parameters\n");
+		printk(KERN_ERR PFX "Can't get floppy parameters\n");
 		return 1;
 	}
 	header_len = pc.buf[3];
@@ -472,8 +479,9 @@ static int ide_floppy_get_capacity(ide_drive_t *drive)
 		blocks = be32_to_cpup((__be32 *)&pc.buf[desc_start]);
 		length = be16_to_cpup((__be16 *)&pc.buf[desc_start + 6]);
 
-		debug_log("Descriptor %d: %dkB, %d blocks, %d sector size\n",
-				i, blocks * length / 1024, blocks, length);
+		ide_debug_log(IDE_DBG_PROBE, "Descriptor %d: %dkB, %d blocks, "
+			      "%d sector size\n",
+			      i, blocks * length / 1024, blocks, length);
 
 		if (i)
 			continue;
@@ -493,23 +501,24 @@ static int ide_floppy_get_capacity(ide_drive_t *drive)
 		case CAPACITY_CURRENT:
 			/* Normal Zip/LS-120 disks */
 			if (memcmp(cap_desc, &floppy->cap_desc, 8))
-				printk(KERN_INFO "%s: %dkB, %d blocks, %d "
-					"sector size\n", drive->name,
-					blocks * length / 1024, blocks, length);
+				printk(KERN_INFO PFX "%s: %dkB, %d blocks, %d "
+				       "sector size\n",
+				       drive->name, blocks * length / 1024,
+				       blocks, length);
 			memcpy(&floppy->cap_desc, cap_desc, 8);
 
 			if (!length || length % 512) {
-				printk(KERN_NOTICE "%s: %d bytes block size "
-					"not supported\n", drive->name, length);
+				printk(KERN_NOTICE PFX "%s: %d bytes block size"
+				       " not supported\n", drive->name, length);
 			} else {
 				floppy->blocks = blocks;
 				floppy->block_size = length;
 				floppy->bs_factor = length / 512;
 				if (floppy->bs_factor != 1)
-					printk(KERN_NOTICE "%s: warning: non "
-						"512 bytes block size not "
-						"fully supported\n",
-						drive->name);
+					printk(KERN_NOTICE PFX "%s: Warning: "
+					       "non 512 bytes block size not "
+					       "fully supported\n",
+					       drive->name);
 				rc = 0;
 			}
 			break;
@@ -518,15 +527,16 @@ static int ide_floppy_get_capacity(ide_drive_t *drive)
 			 * This is a KERN_ERR so it appears on screen
 			 * for the user to see
 			 */
-			printk(KERN_ERR "%s: No disk in drive\n", drive->name);
+			printk(KERN_ERR PFX "%s: No disk in drive\n",
+			       drive->name);
 			break;
 		case CAPACITY_INVALID:
-			printk(KERN_ERR "%s: Invalid capacity for disk "
+			printk(KERN_ERR PFX "%s: Invalid capacity for disk "
 				"in drive\n", drive->name);
 			break;
 		}
-		debug_log("Descriptor 0 Code: %d\n",
-			  pc.buf[desc_start + 4] & 0x03);
+		ide_debug_log(IDE_DBG_PROBE, "Descriptor 0 Code: %d\n",
+			      pc.buf[desc_start + 4] & 0x03);
 	}
 
 	/* Clik! disk does not support get_flexible_disk_page */
@@ -676,14 +686,14 @@ static int idefloppy_open(struct inode *inode, struct file *filp)
 	ide_drive_t *drive;
 	int ret = 0;
 
-	debug_log("Reached %s\n", __func__);
-
 	floppy = ide_floppy_get(disk);
 	if (!floppy)
 		return -ENXIO;
 
 	drive = floppy->drive;
 
+	ide_debug_log(IDE_DBG_FUNC, "Call %s\n", __func__);
+
 	floppy->openers++;
 
 	if (floppy->openers == 1) {
@@ -731,7 +741,7 @@ static int idefloppy_release(struct inode *inode, struct file *filp)
 	struct ide_floppy_obj *floppy = ide_drv_g(disk, ide_floppy_obj);
 	ide_drive_t *drive = floppy->drive;
 
-	debug_log("Reached %s\n", __func__);
+	ide_debug_log(IDE_DBG_FUNC, "Call %s\n", __func__);
 
 	if (floppy->openers == 1) {
 		ide_set_media_lock(drive, disk, 0);
@@ -852,14 +862,14 @@ static int ide_floppy_probe(ide_drive_t *drive)
 		goto failed;
 
 	if (!ide_check_atapi_device(drive, DRV_NAME)) {
-		printk(KERN_ERR "ide-floppy: %s: not supported by this version"
-				" of ide-floppy\n", drive->name);
+		printk(KERN_ERR PFX "%s: not supported by this version of "
+		       DRV_NAME "\n", drive->name);
 		goto failed;
 	}
 	floppy = kzalloc(sizeof(idefloppy_floppy_t), GFP_KERNEL);
 	if (!floppy) {
-		printk(KERN_ERR "ide-floppy: %s: Can't allocate a floppy"
-				" structure\n", drive->name);
+		printk(KERN_ERR PFX "%s: Can't allocate a floppy structure\n",
+		       drive->name);
 		goto failed;
 	}
 
@@ -902,7 +912,7 @@ static void __exit idefloppy_exit(void)
 
 static int __init idefloppy_init(void)
 {
-	printk("ide-floppy driver " IDEFLOPPY_VERSION "\n");
+	printk(KERN_INFO DRV_NAME " driver " IDEFLOPPY_VERSION "\n");
 	return driver_register(&idefloppy_driver.gen_driver);
 }
 
-- 
1.5.5.4

[PATCH 5/5] ide-floppy: add a debug_mask module parameter

[Borislav Petkov]

... 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

Re: [PATCH 0/5] ide debugging macros

[Bartlomiej Zolnierkiewicz]

Hi,

On Sunday 17 August 2008, Borislav Petkov wrote:
> 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 :)

if you look closely you should already find it :)

[ module parameters are exported through sysfs and it uses 0644 mask ]

> 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.

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

Re: [PATCH 0/5] ide debugging macros

[Bartlomiej Zolnierkiewicz]

On Monday 18 August 2008, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Sunday 17 August 2008, Borislav Petkov wrote:
> > 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 :)
> 
> if you look closely you should already find it :)
> 
> [ module parameters are exported through sysfs and it uses 0644 mask ]

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

Re: [PATCH 0/5] ide debugging macros

[Borislav Petkov]

On Mon, Aug 18, 2008 at 11:08:38PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Sunday 17 August 2008, Borislav Petkov wrote:
> > 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 :)
> 
> if you look closely you should already find it :)
> 
> [ module parameters are exported through sysfs and it uses 0644 mask ]

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
yet this is my hunch what we should do.

> > 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.
> 
> 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?

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.

Re: [PATCH 0/5] ide debugging macros

[Borislav Petkov]

On Mon, Aug 18, 2008 at 11:08:38PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Sunday 17 August 2008, Borislav Petkov wrote:
> > 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 :)
> 
> if you look closely you should already find it :)
> 
> [ module parameters are exported through sysfs and it uses 0644 mask ]

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.