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    [W:0.025 / U:0.928 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site