lkml.org 
[lkml]   [2002]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: ide cleanup
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.
-
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/

\
 
 \ /
  Last update: 2005-03-22 13:24    [from the cache]
©2003-2011 Jasper Spaans