ide cleanup

ide cleanup

[Pavel Machek]

Hi!

IDE is
* infested with polish notation
* programmed by cut-copy-paste
* full of unneccessary casts
(there are examples of all these in the patch)
-> totally unreadable

This attempts to clean worst mess in ide-disk.c. Dave, please apply;
or if you feel it is "too big" let me know and I'll feed linus.
									Pavel

--- clean/drivers/ide/ide-disk.c	Thu Jan 31 23:42:14 2002
+++ linux-dm/drivers/ide/ide-disk.c	Wed Feb  6 21:43:51 2002
@@ -123,29 +123,24 @@
  */
 static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, unsigned long block)
 {
-	if (rq->flags & REQ_CMD)
-		goto good_command;
-
-	blk_dump_rq_flags(rq, "do_rw_disk, bad command");
-	ide_end_request(0, HWGROUP(drive));
-	return ide_stopped;
-
-good_command:
+	if (!(rq->flags & REQ_CMD)) {
+		blk_dump_rq_flags(rq, "do_rw_disk, bad command");
+		ide_end_request(0, HWGROUP(drive));
+		return ide_stopped;
+	}
 
-#ifdef CONFIG_BLK_DEV_PDC4030
 	if (IS_PDC4030_DRIVE) {
 		extern ide_startstop_t promise_rw_disk(ide_drive_t *, struct request *, unsigned long);
 		return promise_rw_disk(drive, rq, block);
 	}
-#endif /* CONFIG_BLK_DEV_PDC4030 */
 
 	if ((drive->id->cfs_enable_2 & 0x0400) && (drive->addressing))	/* 48-bit LBA */
-		return lba_48_rw_disk(drive, rq, (unsigned long long) block);
+		return lba_48_rw_disk(drive, rq, block);
 	if (drive->select.b.lba)		/* 28-bit LBA */
-		return lba_28_rw_disk(drive, rq, (unsigned long) block);
+		return lba_28_rw_disk(drive, rq, block);
 
 	/* 28-bit CHS : DIE DIE DIE piece of legacy crap!!! */
-	return chs_rw_disk(drive, rq, (unsigned long) block);
+	return chs_rw_disk(drive, rq, block);
 }
 
 static task_ioreg_t get_command (ide_drive_t *drive, int cmd)
@@ -172,6 +167,30 @@
 		return WIN_NOP;
 }
 
+static int get_sectors (ide_drive_t *drive, struct request *rq, task_ioreg_t command)
+{
+	int sectors = rq->nr_sectors;
+
+	if (sectors == 256)
+		sectors = 0;
+	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
+		sectors = drive->mult_count;
+		if (sectors > rq->current_nr_sectors)
+			sectors = rq->current_nr_sectors;
+	}
+	return sectors;
+}
+
+static void fill_args (ide_task_t *args, struct hd_drive_task_hdr *taskfile, struct hd_drive_hob_hdr *hobfile)
+{
+	memcpy(args->tfRegister, taskfile, sizeof(struct hd_drive_task_hdr));
+	memcpy(args->hobRegister, hobfile, sizeof(struct hd_drive_hob_hdr));
+	args->command_type	= ide_cmd_type_parser(args);
+	args->prehandler	= ide_pre_handler_parser(taskfile, hobfile);
+	args->handler		= ide_handler_parser(taskfile, hobfile);
+	args->posthandler	= NULL;
+}
+
 static ide_startstop_t chs_rw_disk (ide_drive_t *drive, struct request *rq, unsigned long block)
 {
 	struct hd_drive_task_hdr	taskfile;
@@ -186,17 +205,10 @@
 	unsigned int head	= (track % drive->head);
 	unsigned int cyl	= (track / drive->head);
 
-	memset(&taskfile, 0, sizeof(task_struct_t));
-	memset(&hobfile, 0, sizeof(hob_struct_t));
+	memset(&taskfile, 0, sizeof(taskfile));
+	memset(&hobfile, 0, sizeof(hobfile));
 
-	sectors = rq->nr_sectors;
-	if (sectors == 256)
-		sectors = 0;
-	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
-		sectors = drive->mult_count;
-		if (sectors > rq->current_nr_sectors)
-			sectors = rq->current_nr_sectors;
-	}
+	sectors = get_sectors(drive, rq, command);
 
 	taskfile.sector_count	= sectors;
 	taskfile.sector_number	= sect;
@@ -215,16 +227,10 @@
 	printk("buffer=0x%08lx\n", (unsigned long) rq->buffer);
 #endif
 
-	memcpy(args.tfRegister, &taskfile, sizeof(struct hd_drive_task_hdr));
-	memcpy(args.hobRegister, &hobfile, sizeof(struct hd_drive_hob_hdr));
-	args.command_type	= ide_cmd_type_parser(&args);
-	args.prehandler		= ide_pre_handler_parser(&taskfile, &hobfile);
-	args.handler		= ide_handler_parser(&taskfile, &hobfile);
-	args.posthandler	= NULL;
-	args.rq			= (struct request *) rq;
+	fill_args(&args, &taskfile, &hobfile);
+	args.rq			= rq;
 	args.block		= block;
-	rq->special		= NULL;
-	rq->special		= (ide_task_t *)&args;
+	rq->special		= &args;
 
 	return do_rw_taskfile(drive, &args);
 }
@@ -237,18 +243,10 @@
 	int				sectors;
 
 	task_ioreg_t command	= get_command(drive, rq_data_dir(rq));
+	sectors = get_sectors(drive, rq, command);
 
-	sectors = rq->nr_sectors;
-	if (sectors == 256)
-		sectors = 0;
-	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
-		sectors = drive->mult_count;
-		if (sectors > rq->current_nr_sectors)
-			sectors = rq->current_nr_sectors;
-	}
-
-	memset(&taskfile, 0, sizeof(task_struct_t));
-	memset(&hobfile, 0, sizeof(hob_struct_t));
+	memset(&taskfile, 0, sizeof(taskfile));
+	memset(&hobfile, 0, sizeof(hobfile));
 
 	taskfile.sector_count	= sectors;
 	taskfile.sector_number	= block;
@@ -267,16 +265,10 @@
 	printk("buffer=0x%08lx\n", (unsigned long) rq->buffer);
 #endif
 
-	memcpy(args.tfRegister, &taskfile, sizeof(struct hd_drive_task_hdr));
-	memcpy(args.hobRegister, &hobfile, sizeof(struct hd_drive_hob_hdr));
-	args.command_type	= ide_cmd_type_parser(&args);
-	args.prehandler		= ide_pre_handler_parser(&taskfile, &hobfile);
-	args.handler		= ide_handler_parser(&taskfile, &hobfile);
-	args.posthandler	= NULL;
-	args.rq			= (struct request *) rq;
+	fill_args(&args, &taskfile, &hobfile);
+	args.rq			= rq;
 	args.block		= block;
-	rq->special		= NULL;
-	rq->special		= (ide_task_t *)&args;
+	rq->special		= &args;
 
 	return do_rw_taskfile(drive, &args);
 }
@@ -296,17 +288,10 @@
 
 	task_ioreg_t command	= get_command(drive, rq_data_dir(rq));
 
-	memset(&taskfile, 0, sizeof(task_struct_t));
-	memset(&hobfile, 0, sizeof(hob_struct_t));
+	memset(&taskfile, 0, sizeof(taskfile));
+	memset(&hobfile, 0, sizeof(hobfile));
 
-	sectors = rq->nr_sectors;
-	if (sectors == 256)
-		sectors = 0;
-	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
-		sectors = drive->mult_count;
-		if (sectors > rq->current_nr_sectors)
-			sectors = rq->current_nr_sectors;
-	}
+	sectors = get_sectors(drive, rq, command);
 
 	taskfile.sector_count	= sectors;
 	hobfile.sector_count	= sectors >> 8;
@@ -336,16 +321,10 @@
 	printk("buffer=0x%08lx\n", (unsigned long) rq->buffer);
 #endif
 
-	memcpy(args.tfRegister, &taskfile, sizeof(struct hd_drive_task_hdr));
-	memcpy(args.hobRegister, &hobfile, sizeof(struct hd_drive_hob_hdr));
-	args.command_type	= ide_cmd_type_parser(&args);
-	args.prehandler		= ide_pre_handler_parser(&taskfile, &hobfile);
-	args.handler		= ide_handler_parser(&taskfile, &hobfile);
-	args.posthandler	= NULL;
-	args.rq			= (struct request *) rq;
+	fill_args(&args, &taskfile, &hobfile);
+	args.rq			= rq;
 	args.block		= block;
-	rq->special		= NULL;
-	rq->special		= (ide_task_t *)&args;
+	rq->special		= &args;
 
 	return do_rw_taskfile(drive, &args);
 }
--- clean/drivers/ide/ide-proc.c	Thu Jan 31 23:42:14 2002
+++ linux-dm/drivers/ide/ide-proc.c	Mon Feb  4 23:05:09 2002
@@ -591,13 +591,13 @@
 	(char *page, char **start, off_t off, int count, int *eof, void *data)
 {
 	ide_drive_t	*drive = (ide_drive_t *) data;
-	ide_driver_t    *driver = (ide_driver_t *) drive->driver;
+	ide_driver_t    *driver = drive->driver;
 	int		len;
 
 	if (!driver)
 		len = sprintf(page, "(none)\n");
         else
-		len = sprintf(page,"%llu\n", (__u64) ((ide_driver_t *)drive->driver)->capacity(drive));
+		len = sprintf(page,"%llu\n", (__u64) drive->driver->capacity(drive));
 	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
 }
 
@@ -629,7 +629,7 @@
 	(char *page, char **start, off_t off, int count, int *eof, void *data)
 {
 	ide_drive_t	*drive = (ide_drive_t *) data;
-	ide_driver_t	*driver = (ide_driver_t *) drive->driver;
+	ide_driver_t	*driver = drive->driver;
 	int		len;
 
 	if (!driver)
@@ -746,7 +746,6 @@
 	struct proc_dir_entry *ent;
 	struct proc_dir_entry *parent = hwif->proc;
 	char name[64];
-//	ide_driver_t *driver = drive->driver;
 
 	if (drive->present && !drive->proc) {
 		drive->proc = proc_mkdir(drive->name, parent);
--- clean/include/linux/ide.h	Thu Jan 31 23:42:29 2002
+++ linux-dm/include/linux/ide.h	Wed Feb  6 21:32:41 2002
@@ -424,12 +425,12 @@
 	unsigned long	capacity;	/* total number of sectors */
 	unsigned long long capacity48;	/* total number of sectors */
 	unsigned int	drive_data;	/* for use by tuneproc/selectproc as needed */
-	void		  *hwif;	/* actually (ide_hwif_t *) */
+	struct hwif_s   *hwif;		/* actually (ide_hwif_t *) */
 	wait_queue_head_t wqueue;	/* used to wait for drive in open() */
 	struct hd_driveid *id;		/* drive model identification info */
 	struct hd_struct  *part;	/* drive partition table */
 	char		name[4];	/* drive name, such as "hda" */
-	void 		*driver;	/* (ide_driver_t *) */
+	struct ide_driver_s *driver;	/* (ide_driver_t *) */
 	void		*driver_data;	/* extra driver data */
 	devfs_handle_t	de;		/* directory for device */
 	struct proc_dir_entry *proc;	/* /proc/ide/ directory entry */


-- 
(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Re: ide cleanup

[Andre Hedrick]

BZZT, I don't think so.

Since you have not the input or insight to why it was expanded maybe you
should just leave it alone.  Since I am working on how to fix the
MUNGE/KLUDGE that was added to make the PIO work, just maybe you may want
rething the rathole you just created by attempting to compress the code
before it is finished.  What you doing is BORKING the driver to the point
where adding in the TCQ will not work.  Next you have screwed the future
interface for Serial ATA.

Have a good day now!

Andre Hedrick
Linux Disk Certification Project                Linux ATA Development


On Wed, 6 Feb 2002, Pavel Machek wrote:

> Hi!
> 
> IDE is
> * infested with polish notation
> * programmed by cut-copy-paste
> * full of unneccessary casts
> (there are examples of all these in the patch)
> -> totally unreadable
> 
> This attempts to clean worst mess in ide-disk.c. Dave, please apply;
> or if you feel it is "too big" let me know and I'll feed linus.
> 									Pavel
> 
> --- clean/drivers/ide/ide-disk.c	Thu Jan 31 23:42:14 2002
> +++ linux-dm/drivers/ide/ide-disk.c	Wed Feb  6 21:43:51 2002
> @@ -123,29 +123,24 @@
>   */
>  static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, unsigned long block)
>  {
> -	if (rq->flags & REQ_CMD)
> -		goto good_command;
> -
> -	blk_dump_rq_flags(rq, "do_rw_disk, bad command");
> -	ide_end_request(0, HWGROUP(drive));
> -	return ide_stopped;
> -
> -good_command:
> +	if (!(rq->flags & REQ_CMD)) {
> +		blk_dump_rq_flags(rq, "do_rw_disk, bad command");
> +		ide_end_request(0, HWGROUP(drive));
> +		return ide_stopped;
> +	}
>  
> -#ifdef CONFIG_BLK_DEV_PDC4030
>  	if (IS_PDC4030_DRIVE) {
>  		extern ide_startstop_t promise_rw_disk(ide_drive_t *, struct request *, unsigned long);
>  		return promise_rw_disk(drive, rq, block);
>  	}
> -#endif /* CONFIG_BLK_DEV_PDC4030 */
>  
>  	if ((drive->id->cfs_enable_2 & 0x0400) && (drive->addressing))	/* 48-bit LBA */
> -		return lba_48_rw_disk(drive, rq, (unsigned long long) block);
> +		return lba_48_rw_disk(drive, rq, block);
>  	if (drive->select.b.lba)		/* 28-bit LBA */
> -		return lba_28_rw_disk(drive, rq, (unsigned long) block);
> +		return lba_28_rw_disk(drive, rq, block);
>  
>  	/* 28-bit CHS : DIE DIE DIE piece of legacy crap!!! */
> -	return chs_rw_disk(drive, rq, (unsigned long) block);
> +	return chs_rw_disk(drive, rq, block);
>  }
>  
>  static task_ioreg_t get_command (ide_drive_t *drive, int cmd)
> @@ -172,6 +167,30 @@
>  		return WIN_NOP;
>  }
>  
> +static int get_sectors (ide_drive_t *drive, struct request *rq, task_ioreg_t command)
> +{
> +	int sectors = rq->nr_sectors;
> +
> +	if (sectors == 256)
> +		sectors = 0;
> +	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
> +		sectors = drive->mult_count;
> +		if (sectors > rq->current_nr_sectors)
> +			sectors = rq->current_nr_sectors;
> +	}
> +	return sectors;
> +}
> +
> +static void fill_args (ide_task_t *args, struct hd_drive_task_hdr *taskfile, struct hd_drive_hob_hdr *hobfile)
> +{
> +	memcpy(args->tfRegister, taskfile, sizeof(struct hd_drive_task_hdr));
> +	memcpy(args->hobRegister, hobfile, sizeof(struct hd_drive_hob_hdr));
> +	args->command_type	= ide_cmd_type_parser(args);
> +	args->prehandler	= ide_pre_handler_parser(taskfile, hobfile);
> +	args->handler		= ide_handler_parser(taskfile, hobfile);
> +	args->posthandler	= NULL;
> +}
> +
>  static ide_startstop_t chs_rw_disk (ide_drive_t *drive, struct request *rq, unsigned long block)
>  {
>  	struct hd_drive_task_hdr	taskfile;
> @@ -186,17 +205,10 @@
>  	unsigned int head	= (track % drive->head);
>  	unsigned int cyl	= (track / drive->head);
>  
> -	memset(&taskfile, 0, sizeof(task_struct_t));
> -	memset(&hobfile, 0, sizeof(hob_struct_t));
> +	memset(&taskfile, 0, sizeof(taskfile));
> +	memset(&hobfile, 0, sizeof(hobfile));
>  
> -	sectors = rq->nr_sectors;
> -	if (sectors == 256)
> -		sectors = 0;
> -	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
> -		sectors = drive->mult_count;
> -		if (sectors > rq->current_nr_sectors)
> -			sectors = rq->current_nr_sectors;
> -	}
> +	sectors = get_sectors(drive, rq, command);
>  
>  	taskfile.sector_count	= sectors;
>  	taskfile.sector_number	= sect;
> @@ -215,16 +227,10 @@
>  	printk("buffer=0x%08lx\n", (unsigned long) rq->buffer);
>  #endif
>  
> -	memcpy(args.tfRegister, &taskfile, sizeof(struct hd_drive_task_hdr));
> -	memcpy(args.hobRegister, &hobfile, sizeof(struct hd_drive_hob_hdr));
> -	args.command_type	= ide_cmd_type_parser(&args);
> -	args.prehandler		= ide_pre_handler_parser(&taskfile, &hobfile);
> -	args.handler		= ide_handler_parser(&taskfile, &hobfile);
> -	args.posthandler	= NULL;
> -	args.rq			= (struct request *) rq;
> +	fill_args(&args, &taskfile, &hobfile);
> +	args.rq			= rq;
>  	args.block		= block;
> -	rq->special		= NULL;
> -	rq->special		= (ide_task_t *)&args;
> +	rq->special		= &args;
>  
>  	return do_rw_taskfile(drive, &args);
>  }
> @@ -237,18 +243,10 @@
>  	int				sectors;
>  
>  	task_ioreg_t command	= get_command(drive, rq_data_dir(rq));
> +	sectors = get_sectors(drive, rq, command);
>  
> -	sectors = rq->nr_sectors;
> -	if (sectors == 256)
> -		sectors = 0;
> -	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
> -		sectors = drive->mult_count;
> -		if (sectors > rq->current_nr_sectors)
> -			sectors = rq->current_nr_sectors;
> -	}
> -
> -	memset(&taskfile, 0, sizeof(task_struct_t));
> -	memset(&hobfile, 0, sizeof(hob_struct_t));
> +	memset(&taskfile, 0, sizeof(taskfile));
> +	memset(&hobfile, 0, sizeof(hobfile));
>  
>  	taskfile.sector_count	= sectors;
>  	taskfile.sector_number	= block;
> @@ -267,16 +265,10 @@
>  	printk("buffer=0x%08lx\n", (unsigned long) rq->buffer);
>  #endif
>  
> -	memcpy(args.tfRegister, &taskfile, sizeof(struct hd_drive_task_hdr));
> -	memcpy(args.hobRegister, &hobfile, sizeof(struct hd_drive_hob_hdr));
> -	args.command_type	= ide_cmd_type_parser(&args);
> -	args.prehandler		= ide_pre_handler_parser(&taskfile, &hobfile);
> -	args.handler		= ide_handler_parser(&taskfile, &hobfile);
> -	args.posthandler	= NULL;
> -	args.rq			= (struct request *) rq;
> +	fill_args(&args, &taskfile, &hobfile);
> +	args.rq			= rq;
>  	args.block		= block;
> -	rq->special		= NULL;
> -	rq->special		= (ide_task_t *)&args;
> +	rq->special		= &args;
>  
>  	return do_rw_taskfile(drive, &args);
>  }
> @@ -296,17 +288,10 @@
>  
>  	task_ioreg_t command	= get_command(drive, rq_data_dir(rq));
>  
> -	memset(&taskfile, 0, sizeof(task_struct_t));
> -	memset(&hobfile, 0, sizeof(hob_struct_t));
> +	memset(&taskfile, 0, sizeof(taskfile));
> +	memset(&hobfile, 0, sizeof(hobfile));
>  
> -	sectors = rq->nr_sectors;
> -	if (sectors == 256)
> -		sectors = 0;
> -	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
> -		sectors = drive->mult_count;
> -		if (sectors > rq->current_nr_sectors)
> -			sectors = rq->current_nr_sectors;
> -	}
> +	sectors = get_sectors(drive, rq, command);
>  
>  	taskfile.sector_count	= sectors;
>  	hobfile.sector_count	= sectors >> 8;
> @@ -336,16 +321,10 @@
>  	printk("buffer=0x%08lx\n", (unsigned long) rq->buffer);
>  #endif
>  
> -	memcpy(args.tfRegister, &taskfile, sizeof(struct hd_drive_task_hdr));
> -	memcpy(args.hobRegister, &hobfile, sizeof(struct hd_drive_hob_hdr));
> -	args.command_type	= ide_cmd_type_parser(&args);
> -	args.prehandler		= ide_pre_handler_parser(&taskfile, &hobfile);
> -	args.handler		= ide_handler_parser(&taskfile, &hobfile);
> -	args.posthandler	= NULL;
> -	args.rq			= (struct request *) rq;
> +	fill_args(&args, &taskfile, &hobfile);
> +	args.rq			= rq;
>  	args.block		= block;
> -	rq->special		= NULL;
> -	rq->special		= (ide_task_t *)&args;
> +	rq->special		= &args;
>  
>  	return do_rw_taskfile(drive, &args);
>  }
> --- clean/drivers/ide/ide-proc.c	Thu Jan 31 23:42:14 2002
> +++ linux-dm/drivers/ide/ide-proc.c	Mon Feb  4 23:05:09 2002
> @@ -591,13 +591,13 @@
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
>  	ide_drive_t	*drive = (ide_drive_t *) data;
> -	ide_driver_t    *driver = (ide_driver_t *) drive->driver;
> +	ide_driver_t    *driver = drive->driver;
>  	int		len;
>  
>  	if (!driver)
>  		len = sprintf(page, "(none)\n");
>          else
> -		len = sprintf(page,"%llu\n", (__u64) ((ide_driver_t *)drive->driver)->capacity(drive));
> +		len = sprintf(page,"%llu\n", (__u64) drive->driver->capacity(drive));
>  	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
>  }
>  
> @@ -629,7 +629,7 @@
>  	(char *page, char **start, off_t off, int count, int *eof, void *data)
>  {
>  	ide_drive_t	*drive = (ide_drive_t *) data;
> -	ide_driver_t	*driver = (ide_driver_t *) drive->driver;
> +	ide_driver_t	*driver = drive->driver;
>  	int		len;
>  
>  	if (!driver)
> @@ -746,7 +746,6 @@
>  	struct proc_dir_entry *ent;
>  	struct proc_dir_entry *parent = hwif->proc;
>  	char name[64];
> -//	ide_driver_t *driver = drive->driver;
>  
>  	if (drive->present && !drive->proc) {
>  		drive->proc = proc_mkdir(drive->name, parent);
> --- clean/include/linux/ide.h	Thu Jan 31 23:42:29 2002
> +++ linux-dm/include/linux/ide.h	Wed Feb  6 21:32:41 2002
> @@ -424,12 +425,12 @@
>  	unsigned long	capacity;	/* total number of sectors */
>  	unsigned long long capacity48;	/* total number of sectors */
>  	unsigned int	drive_data;	/* for use by tuneproc/selectproc as needed */
> -	void		  *hwif;	/* actually (ide_hwif_t *) */
> +	struct hwif_s   *hwif;		/* actually (ide_hwif_t *) */
>  	wait_queue_head_t wqueue;	/* used to wait for drive in open() */
>  	struct hd_driveid *id;		/* drive model identification info */
>  	struct hd_struct  *part;	/* drive partition table */
>  	char		name[4];	/* drive name, such as "hda" */
> -	void 		*driver;	/* (ide_driver_t *) */
> +	struct ide_driver_s *driver;	/* (ide_driver_t *) */
>  	void		*driver_data;	/* extra driver data */
>  	devfs_handle_t	de;		/* directory for device */
>  	struct proc_dir_entry *proc;	/* /proc/ide/ directory entry */
> 
> 
> -- 
> (about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
> no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Re: ide cleanup

[Vojtech Pavlik]

On Wed, Feb 06, 2002 at 07:34:01PM -0800, Andre Hedrick wrote:

> BZZT, I don't think so.
> 
> Since you have not the input or insight to why it was expanded maybe you
> should just leave it alone.  Since I am working on how to fix the
> MUNGE/KLUDGE that was added to make the PIO work, just maybe you may want
> rething the rathole you just created by attempting to compress the code
> before it is finished.  What you doing is BORKING the driver to the point
> where adding in the TCQ will not work.  Next you have screwed the future
> interface for Serial ATA.
> 
> Have a good day now!

Ok, Andre, how about the other cleanup bits of the patch? I think those
don't "BORK" anything?

> Andre Hedrick
> Linux Disk Certification Project                Linux ATA Development
> 
> 
> On Wed, 6 Feb 2002, Pavel Machek wrote:
> 
> > Hi!
> > 
> > IDE is
> > * infested with polish notation
> > * programmed by cut-copy-paste
> > * full of unneccessary casts
> > (there are examples of all these in the patch)
> > -> totally unreadable
> > 
> > This attempts to clean worst mess in ide-disk.c. Dave, please apply;
> > or if you feel it is "too big" let me know and I'll feed linus.
> > 									Pavel
> > 
> > --- clean/drivers/ide/ide-disk.c	Thu Jan 31 23:42:14 2002
> > +++ linux-dm/drivers/ide/ide-disk.c	Wed Feb  6 21:43:51 2002
> > @@ -123,29 +123,24 @@
> >   */
> >  static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, unsigned long block)
> >  {
> > -	if (rq->flags & REQ_CMD)
> > -		goto good_command;
> > -
> > -	blk_dump_rq_flags(rq, "do_rw_disk, bad command");
> > -	ide_end_request(0, HWGROUP(drive));
> > -	return ide_stopped;
> > -
> > -good_command:
> > +	if (!(rq->flags & REQ_CMD)) {
> > +		blk_dump_rq_flags(rq, "do_rw_disk, bad command");
> > +		ide_end_request(0, HWGROUP(drive));
> > +		return ide_stopped;
> > +	}
> >  
> > -#ifdef CONFIG_BLK_DEV_PDC4030
> >  	if (IS_PDC4030_DRIVE) {
> >  		extern ide_startstop_t promise_rw_disk(ide_drive_t *, struct request *, unsigned long);
> >  		return promise_rw_disk(drive, rq, block);
> >  	}
> > -#endif /* CONFIG_BLK_DEV_PDC4030 */
> >  
> >  	if ((drive->id->cfs_enable_2 & 0x0400) && (drive->addressing))	/* 48-bit LBA */
> > -		return lba_48_rw_disk(drive, rq, (unsigned long long) block);
> > +		return lba_48_rw_disk(drive, rq, block);
> >  	if (drive->select.b.lba)		/* 28-bit LBA */
> > -		return lba_28_rw_disk(drive, rq, (unsigned long) block);
> > +		return lba_28_rw_disk(drive, rq, block);
> >  
> >  	/* 28-bit CHS : DIE DIE DIE piece of legacy crap!!! */
> > -	return chs_rw_disk(drive, rq, (unsigned long) block);
> > +	return chs_rw_disk(drive, rq, block);
> >  }
> >  
> >  static task_ioreg_t get_command (ide_drive_t *drive, int cmd)
> > @@ -172,6 +167,30 @@
> >  		return WIN_NOP;
> >  }
> >  
> > +static int get_sectors (ide_drive_t *drive, struct request *rq, task_ioreg_t command)
> > +{
> > +	int sectors = rq->nr_sectors;
> > +
> > +	if (sectors == 256)
> > +		sectors = 0;
> > +	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
> > +		sectors = drive->mult_count;
> > +		if (sectors > rq->current_nr_sectors)
> > +			sectors = rq->current_nr_sectors;
> > +	}
> > +	return sectors;
> > +}
> > +
> > +static void fill_args (ide_task_t *args, struct hd_drive_task_hdr *taskfile, struct hd_drive_hob_hdr *hobfile)
> > +{
> > +	memcpy(args->tfRegister, taskfile, sizeof(struct hd_drive_task_hdr));
> > +	memcpy(args->hobRegister, hobfile, sizeof(struct hd_drive_hob_hdr));
> > +	args->command_type	= ide_cmd_type_parser(args);
> > +	args->prehandler	= ide_pre_handler_parser(taskfile, hobfile);
> > +	args->handler		= ide_handler_parser(taskfile, hobfile);
> > +	args->posthandler	= NULL;
> > +}
> > +
> >  static ide_startstop_t chs_rw_disk (ide_drive_t *drive, struct request *rq, unsigned long block)
> >  {
> >  	struct hd_drive_task_hdr	taskfile;
> > @@ -186,17 +205,10 @@
> >  	unsigned int head	= (track % drive->head);
> >  	unsigned int cyl	= (track / drive->head);
> >  
> > -	memset(&taskfile, 0, sizeof(task_struct_t));
> > -	memset(&hobfile, 0, sizeof(hob_struct_t));
> > +	memset(&taskfile, 0, sizeof(taskfile));
> > +	memset(&hobfile, 0, sizeof(hobfile));
> >  
> > -	sectors = rq->nr_sectors;
> > -	if (sectors == 256)
> > -		sectors = 0;
> > -	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
> > -		sectors = drive->mult_count;
> > -		if (sectors > rq->current_nr_sectors)
> > -			sectors = rq->current_nr_sectors;
> > -	}
> > +	sectors = get_sectors(drive, rq, command);
> >  
> >  	taskfile.sector_count	= sectors;
> >  	taskfile.sector_number	= sect;
> > @@ -215,16 +227,10 @@
> >  	printk("buffer=0x%08lx\n", (unsigned long) rq->buffer);
> >  #endif
> >  
> > -	memcpy(args.tfRegister, &taskfile, sizeof(struct hd_drive_task_hdr));
> > -	memcpy(args.hobRegister, &hobfile, sizeof(struct hd_drive_hob_hdr));
> > -	args.command_type	= ide_cmd_type_parser(&args);
> > -	args.prehandler		= ide_pre_handler_parser(&taskfile, &hobfile);
> > -	args.handler		= ide_handler_parser(&taskfile, &hobfile);
> > -	args.posthandler	= NULL;
> > -	args.rq			= (struct request *) rq;
> > +	fill_args(&args, &taskfile, &hobfile);
> > +	args.rq			= rq;
> >  	args.block		= block;
> > -	rq->special		= NULL;
> > -	rq->special		= (ide_task_t *)&args;
> > +	rq->special		= &args;
> >  
> >  	return do_rw_taskfile(drive, &args);
> >  }
> > @@ -237,18 +243,10 @@
> >  	int				sectors;
> >  
> >  	task_ioreg_t command	= get_command(drive, rq_data_dir(rq));
> > +	sectors = get_sectors(drive, rq, command);
> >  
> > -	sectors = rq->nr_sectors;
> > -	if (sectors == 256)
> > -		sectors = 0;
> > -	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
> > -		sectors = drive->mult_count;
> > -		if (sectors > rq->current_nr_sectors)
> > -			sectors = rq->current_nr_sectors;
> > -	}
> > -
> > -	memset(&taskfile, 0, sizeof(task_struct_t));
> > -	memset(&hobfile, 0, sizeof(hob_struct_t));
> > +	memset(&taskfile, 0, sizeof(taskfile));
> > +	memset(&hobfile, 0, sizeof(hobfile));
> >  
> >  	taskfile.sector_count	= sectors;
> >  	taskfile.sector_number	= block;
> > @@ -267,16 +265,10 @@
> >  	printk("buffer=0x%08lx\n", (unsigned long) rq->buffer);
> >  #endif
> >  
> > -	memcpy(args.tfRegister, &taskfile, sizeof(struct hd_drive_task_hdr));
> > -	memcpy(args.hobRegister, &hobfile, sizeof(struct hd_drive_hob_hdr));
> > -	args.command_type	= ide_cmd_type_parser(&args);
> > -	args.prehandler		= ide_pre_handler_parser(&taskfile, &hobfile);
> > -	args.handler		= ide_handler_parser(&taskfile, &hobfile);
> > -	args.posthandler	= NULL;
> > -	args.rq			= (struct request *) rq;
> > +	fill_args(&args, &taskfile, &hobfile);
> > +	args.rq			= rq;
> >  	args.block		= block;
> > -	rq->special		= NULL;
> > -	rq->special		= (ide_task_t *)&args;
> > +	rq->special		= &args;
> >  
> >  	return do_rw_taskfile(drive, &args);
> >  }
> > @@ -296,17 +288,10 @@
> >  
> >  	task_ioreg_t command	= get_command(drive, rq_data_dir(rq));
> >  
> > -	memset(&taskfile, 0, sizeof(task_struct_t));
> > -	memset(&hobfile, 0, sizeof(hob_struct_t));
> > +	memset(&taskfile, 0, sizeof(taskfile));
> > +	memset(&hobfile, 0, sizeof(hobfile));
> >  
> > -	sectors = rq->nr_sectors;
> > -	if (sectors == 256)
> > -		sectors = 0;
> > -	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
> > -		sectors = drive->mult_count;
> > -		if (sectors > rq->current_nr_sectors)
> > -			sectors = rq->current_nr_sectors;
> > -	}
> > +	sectors = get_sectors(drive, rq, command);
> >  
> >  	taskfile.sector_count	= sectors;
> >  	hobfile.sector_count	= sectors >> 8;
> > @@ -336,16 +321,10 @@
> >  	printk("buffer=0x%08lx\n", (unsigned long) rq->buffer);
> >  #endif
> >  
> > -	memcpy(args.tfRegister, &taskfile, sizeof(struct hd_drive_task_hdr));
> > -	memcpy(args.hobRegister, &hobfile, sizeof(struct hd_drive_hob_hdr));
> > -	args.command_type	= ide_cmd_type_parser(&args);
> > -	args.prehandler		= ide_pre_handler_parser(&taskfile, &hobfile);
> > -	args.handler		= ide_handler_parser(&taskfile, &hobfile);
> > -	args.posthandler	= NULL;
> > -	args.rq			= (struct request *) rq;
> > +	fill_args(&args, &taskfile, &hobfile);
> > +	args.rq			= rq;
> >  	args.block		= block;
> > -	rq->special		= NULL;
> > -	rq->special		= (ide_task_t *)&args;
> > +	rq->special		= &args;
> >  
> >  	return do_rw_taskfile(drive, &args);
> >  }
> > --- clean/drivers/ide/ide-proc.c	Thu Jan 31 23:42:14 2002
> > +++ linux-dm/drivers/ide/ide-proc.c	Mon Feb  4 23:05:09 2002
> > @@ -591,13 +591,13 @@
> >  	(char *page, char **start, off_t off, int count, int *eof, void *data)
> >  {
> >  	ide_drive_t	*drive = (ide_drive_t *) data;
> > -	ide_driver_t    *driver = (ide_driver_t *) drive->driver;
> > +	ide_driver_t    *driver = drive->driver;
> >  	int		len;
> >  
> >  	if (!driver)
> >  		len = sprintf(page, "(none)\n");
> >          else
> > -		len = sprintf(page,"%llu\n", (__u64) ((ide_driver_t *)drive->driver)->capacity(drive));
> > +		len = sprintf(page,"%llu\n", (__u64) drive->driver->capacity(drive));
> >  	PROC_IDE_READ_RETURN(page,start,off,count,eof,len);
> >  }
> >  
> > @@ -629,7 +629,7 @@
> >  	(char *page, char **start, off_t off, int count, int *eof, void *data)
> >  {
> >  	ide_drive_t	*drive = (ide_drive_t *) data;
> > -	ide_driver_t	*driver = (ide_driver_t *) drive->driver;
> > +	ide_driver_t	*driver = drive->driver;
> >  	int		len;
> >  
> >  	if (!driver)
> > @@ -746,7 +746,6 @@
> >  	struct proc_dir_entry *ent;
> >  	struct proc_dir_entry *parent = hwif->proc;
> >  	char name[64];
> > -//	ide_driver_t *driver = drive->driver;
> >  
> >  	if (drive->present && !drive->proc) {
> >  		drive->proc = proc_mkdir(drive->name, parent);
> > --- clean/include/linux/ide.h	Thu Jan 31 23:42:29 2002
> > +++ linux-dm/include/linux/ide.h	Wed Feb  6 21:32:41 2002
> > @@ -424,12 +425,12 @@
> >  	unsigned long	capacity;	/* total number of sectors */
> >  	unsigned long long capacity48;	/* total number of sectors */
> >  	unsigned int	drive_data;	/* for use by tuneproc/selectproc as needed */
> > -	void		  *hwif;	/* actually (ide_hwif_t *) */
> > +	struct hwif_s   *hwif;		/* actually (ide_hwif_t *) */
> >  	wait_queue_head_t wqueue;	/* used to wait for drive in open() */
> >  	struct hd_driveid *id;		/* drive model identification info */
> >  	struct hd_struct  *part;	/* drive partition table */
> >  	char		name[4];	/* drive name, such as "hda" */
> > -	void 		*driver;	/* (ide_driver_t *) */
> > +	struct ide_driver_s *driver;	/* (ide_driver_t *) */
> >  	void		*driver_data;	/* extra driver data */
> >  	devfs_handle_t	de;		/* directory for device */
> >  	struct proc_dir_entry *proc;	/* /proc/ide/ directory entry */
> > 
> > 
> > -- 
> > (about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
> > no longer is classifiable as a democracy, but rather as a plutocracy." --hpa
> 

-- 
Vojtech Pavlik
SuSE Labs

Re: ide cleanup

[Pavel Machek]

Hi!

> Since you have not the input or insight to why it was expanded maybe you
> should just leave it alone.  Since I am working on how to fix the
> MUNGE/KLUDGE that was added to make the PIO work, just maybe you may want
> rething the rathole you just created by attempting to compress the code
> before it is finished.  What you doing is BORKING the driver to the point
> where adding in the TCQ will not work.  Next you have screwed the future
> interface for Serial ATA.

I'm pretty sure I did not BORK anything. If I did BORK anything, show
me. If you need modified version, you can always just inline it;
programming with cut-copy-paste is bad, *always*.

Perhaps you should clean the drivers up before continuing
development. What is in drivers/ide is a mess. 

I need driverfs support in drivers/ide, but code is such a pain to
look at that it is neccessary to clean it up first.

Let's take a look:

> >  static ide_startstop_t do_rw_disk (ide_drive_t *drive, struct request *rq, unsigned long block)
> >  {
> > -	if (rq->flags & REQ_CMD)
> > -		goto good_command;
> > -
> > -	blk_dump_rq_flags(rq, "do_rw_disk, bad command");
> > -	ide_end_request(0, HWGROUP(drive));
> > -	return ide_stopped;
> > -
> > -good_command:

Goto without reason.

> > -#ifdef CONFIG_BLK_DEV_PDC4030
> >  	if (IS_PDC4030_DRIVE) {
> >  		extern ide_startstop_t promise_rw_disk(ide_drive_t *, struct request *, unsigned long);
> >  		return promise_rw_disk(drive, rq, block);
> >  	}
> > -#endif /* CONFIG_BLK_DEV_PDC4030 */

Completely unneccessary ifdef.

> > +static int get_sectors (ide_drive_t *drive, struct request *rq, task_ioreg_t command)
> > +{
> > +	int sectors = rq->nr_sectors;
> > +
> > +	if (sectors == 256)
> > +		sectors = 0;
> > +	if (command == WIN_MULTWRITE_EXT || command == WIN_MULTWRITE) {
> > +		sectors = drive->mult_count;
> > +		if (sectors > rq->current_nr_sectors)
> > +			sectors = rq->current_nr_sectors;
> > +	}
> > +	return sectors;
> > +}

Get_sectors was included 3 times below. That makes you wonder that
maybe there are small changes there? No, they were not.

> > @@ -186,17 +205,10 @@
> >  	unsigned int head	= (track % drive->head);
> >  	unsigned int cyl	= (track / drive->head);
> >  
> > -	memset(&taskfile, 0, sizeof(task_struct_t));

taskfile was defined as struct hd_drive_task_hdr, above. Very nice way
how to obfuscate code.

> > -	rq->special		= NULL;
> > -	rq->special		= (ide_task_t *)&args;

Two assignments to same variable [yep, these lines are just below each
other], and unneccessary cast as a bonus. [Unneccessary casts are
dangerous: they hide real errors.]

> > -	void		  *hwif;	/* actually (ide_hwif_t *) */
...
> > -	void 		*driver;	/* (ide_driver_t *) */

Two unneccessary pointers. Makes me wonder if original author knew C?
[how old is this code, btw?]

								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: ide cleanup

[Martin Dalecki]

Pavel Machek wrote:

>Hi!
>
>IDE is
>* infested with polish notation
>
I don't see any polish notation there. Could you please explain what you 
mean with this note?
Other then this - the patch does good.... BTW. There is the issue of 
multiple
block strategy routines in ide-disk.c and the selection of 16 ver. 32 
bit transfers could
be simplified as well, since the particular code in question is 
blatantly over-optimized.

Re: ide cleanup

[Vojtech Pavlik]

On Fri, Feb 08, 2002 at 01:34:55PM +0100, Martin Dalecki wrote:
> 
> >Hi!
> >
> >IDE is
> >* infested with polish notation
> >
> I don't see any polish notation there. Could you please explain what you 
> mean with this note?

I think Pavel meant what I think is called "hungarian notation", adding
_t's to type identifiers or adding _i or i_ to integer variables.

> Other then this - the patch does good.... BTW. There is the issue of
> multiple block strategy routines in ide-disk.c and the selection of 16
> ver. 32 bit transfers could be simplified as well, since the
> particular code in question is blatantly over-optimized.

-- 
Vojtech Pavlik
SuSE Labs

Re: ide cleanup

[Martin Dalecki]

Vojtech Pavlik wrote:

>On Fri, Feb 08, 2002 at 01:34:55PM +0100, Martin Dalecki wrote:
>
>>>Hi!
>>>
>>>IDE is
>>>* infested with polish notation
>>>
>>I don't see any polish notation there. Could you please explain what you 
>>mean with this note?
>>
>
>I think Pavel meant what I think is called "hungarian notation", adding
>_t's to type identifiers or adding _i or i_ to integer variables.
>
The encoding of type signatures in function names is indeed called 
hungarian notation.
The _t at the end of type names is a POSIX habit of markup for system 
defined types - this should *NOT*
be used in user land programms but is OK for the kernel. However for the 
ide drivers it's indeed unnecessary
code noise.

Polish notation is the anglo-saxon term for a mathematical expression 
syntax which is avoiding
parents by not using an "in between" operator notation but using 
functional notation instead.
It was invented by ?ukasiewich at the beginning of the last century for 
formal logic and is indeed more
convenient there if you start to deal with long expressions.... Just to 
clarify the terms OK?

Re: ide cleanup

[Wichert Akkerman]

In article <3C63CF54.9090308@evision-ventures.com>,
Martin Dalecki  <dalecki@evision-ventures.com> wrote:
>The _t at the end of type names is a POSIX habit of markup for system 
>defined types - this should *NOT* be used in user land programms but is OK for
>the kernel.

Why, I don't see that. Everyone should use whatever notation he/she
feels most comfortable with.

Wichert.

-- 
  _________________________________________________________________
 /       Nothing is fool-proof to a sufficiently talented fool     \
| wichert@wiggy.net                   http://www.liacs.nl/~wichert/ |
| 1024D/2FA3BC2D 576E 100B 518D 2F16 36B0  2805 3CB8 9250 2FA3 BC2D |

Re: ide cleanup

[Dave Jones]

On Fri, Feb 08, 2002 at 03:50:51PM +0100, Wichert Akkerman wrote:

 > Why, I don't see that. Everyone should use whatever notation he/she
 > feels most comfortable with.

 Documentation/CodingStyle is there for a reason.
 Having code in readable form only to its author isn't good practise.
 (Unfortunatly, drivers/ide/ isn't by far the worse offender..)

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: ide cleanup

[Wichert Akkerman]

Previously Dave Jones wrote:
>  Documentation/CodingStyle is there for a reason.
>  Having code in readable form only to its author isn't good practise.
>  (Unfortunatly, drivers/ide/ isn't by far the worse offender..)

He was talking about userland.. 

Wichert.

-- 
  _________________________________________________________________
 /wichert@wiggy.net         This space intentionally left occupied \
| wichert@deephackmode.org            http://www.liacs.nl/~wichert/ |
| 1024D/2FA3BC2D 576E 100B 518D 2F16 36B0  2805 3CB8 9250 2FA3 BC2D |

Re: ide cleanup

[Martin Dalecki]

Wichert Akkerman wrote:

>In article <3C63CF54.9090308@evision-ventures.com>,
>Martin Dalecki  <dalecki@evision-ventures.com> wrote:
>
>>The _t at the end of type names is a POSIX habit of markup for system 
>>defined types - this should *NOT* be used in user land programms but is OK for
>>the kernel.
>>
>
>Why, I don't see that. Everyone should use whatever notation he/she
>feels most comfortable with.
>

If he intends (or loves to)  to have name-sapce clashes with system 
headers he should feel free indeed.

Re: ide cleanup

[James Antill]

wichert@cistron.nl (Wichert Akkerman) writes:

> In article <3C63CF54.9090308@evision-ventures.com>,
> Martin Dalecki  <dalecki@evision-ventures.com> wrote:
> >The _t at the end of type names is a POSIX habit of markup for system 
> >defined types - this should *NOT* be used in user land programms but is OK for
> >the kernel.
> 
> Why, I don't see that. Everyone should use whatever notation he/she
> feels most comfortable with.

 Err, what?
 Sure mindless programmers can call a function strnew() or strconcat()
if they "feel most comfortable" with that. But it's _wrong_, as that's
a reserved namespace of ISO C. Jut as *_t is a reserved namespace of
POSIX.

 Opengroup seems really slow atm., but hopefully you'll believe one of...

http://sources.redhat.com/ml/libc-alpha/2000-09/msg00185.html
http://www.ioccc.org/1998/data

-- 
# James Antill -- james@and.org
:0:
* ^From: .*james@and\.org
/dev/null

Re: ide cleanup

[Pavel Machek]

Hi!

> >>>IDE is
> >>>* infested with polish notation
> >>>
> >>I don't see any polish notation there. Could you please explain what you 
> >>mean with this note?
> >>
> >
> >I think Pavel meant what I think is called "hungarian notation", adding
> >_t's to type identifiers or adding _i or i_ to integer variables.
> >
> The encoding of type signatures in function names is indeed called 
> hungarian notation.
> The _t at the end of type names is a POSIX habit of markup for system 
> defined types - this should *NOT*
> be used in user land programms but is OK for the kernel. However for the 
> ide drivers it's indeed unnecessary
> code noise.
> 
> Polish notation is the anglo-saxon term for a mathematical expression 
> syntax which is avoiding
> parents by not using an "in between" operator notation but using 
> functional notation instead.
> It was invented by ?ukasiewich at the beginning of the last century for 
> formal logic and is indeed more
> convenient there if you start to deal with long expressions.... Just to 
> clarify the terms OK?

Sorry, I really meant that nasty _t. (The driver even did things like 

struct foo xyzzy;
memset(&xyzzy, 0, sizeof(foo_t));

using both 'struct foo' and foo_t for one variable.
									Pavel
-- 
(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa

Re: ide cleanup

[Bill Davidsen]

On Wed, 6 Feb 2002, Pavel Machek wrote:

> -#ifdef CONFIG_BLK_DEV_PDC4030
>  	if (IS_PDC4030_DRIVE) {
>  		extern ide_startstop_t promise_rw_disk(ide_drive_t *, struct request *, unsigned long);
>  		return promise_rw_disk(drive, rq, block);
>  	}
> -#endif /* CONFIG_BLK_DEV_PDC4030 */

Am I reading this totally wrong, or do you really think it's a good idea
to test for a drive even if the user didn't configure such hardware?

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: ide cleanup

[Vojtech Pavlik]

On Sat, Feb 09, 2002 at 01:19:58PM -0500, Bill Davidsen wrote:
> On Wed, 6 Feb 2002, Pavel Machek wrote:
> 
> > -#ifdef CONFIG_BLK_DEV_PDC4030
> >  	if (IS_PDC4030_DRIVE) {
> >  		extern ide_startstop_t promise_rw_disk(ide_drive_t *, struct request *, unsigned long);
> >  		return promise_rw_disk(drive, rq, block);
> >  	}
> > -#endif /* CONFIG_BLK_DEV_PDC4030 */
> 
> Am I reading this totally wrong, or do you really think it's a good idea
> to test for a drive even if the user didn't configure such hardware?

Well, since IS_PDC4030_DRIVE will always be 0 in that case, the test
will be optimized out ...

-- 
Vojtech Pavlik
SuSE Labs

Re: ide cleanup

[Bill Davidsen]

On Sat, 9 Feb 2002, Vojtech Pavlik wrote:

> On Sat, Feb 09, 2002 at 01:19:58PM -0500, Bill Davidsen wrote:
> > On Wed, 6 Feb 2002, Pavel Machek wrote:
> > 
> > > -#ifdef CONFIG_BLK_DEV_PDC4030
> > >  	if (IS_PDC4030_DRIVE) {
> > >  		extern ide_startstop_t promise_rw_disk(ide_drive_t *, struct request *, unsigned long);
> > >  		return promise_rw_disk(drive, rq, block);
> > >  	}
> > > -#endif /* CONFIG_BLK_DEV_PDC4030 */
> > 
> > Am I reading this totally wrong, or do you really think it's a good idea
> > to test for a drive even if the user didn't configure such hardware?
> 
> Well, since IS_PDC4030_DRIVE will always be 0 in that case, the test
> will be optimized out ...

That's currently true, but since there are other places in the kernel
which use the ifdef, it's unobvious to read because you have to know that
the drive type test macro definition has side effects, it depends on a
behaviour of gcc which may not always be true, and it saves nothing, just
moves the code from the preprocessor to the optimizer, I don't see
removing it in general. 

Just my opinion, code should be written to be EASILY read rather than as
if to be entered in an obfuscated C contest. This is not that bad, but the
test always being false is not obvious without a study of the entire use
of the macro, the ifdef is.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: ide cleanup

[Pavel Machek]

Hi!

> Just my opinion, code should be written to be EASILY read rather than as
> if to be entered in an obfuscated C contest. This is not that bad, but the

#ifdef is *the* tool to make obfuscated code. It does not fit there,
and makes it hard to parse. #ifdefs are to be avoided.
									Pavel
-- 
(about SSSCA) "I don't say this lightly.  However, I really think that the U.S.
no longer is classifiable as a democracy, but rather as a plutocracy." --hpa