Re: ide cleanup

From: Andre Hedrick (andre@linuxdiskcert.org)
Date: Wed Feb 06 2002 - 22:34:01 EST


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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Thu Feb 07 2002 - 21:00:57 EST