lkml.org 
[lkml]   [2005]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2.6.11-rc3 01/11] ide: task_end_request() fix
     Hello, Bartlomiej.

    On Tue, Mar 01, 2005 at 03:30:32PM +0100, Bartlomiej Zolnierkiewicz wrote:
    > On Sun, 27 Feb 2005 15:49:22 +0900, Tejun Heo <htejun@gmail.com> wrote:
    > >
    > > Taskfile DMA path is still broken. Also calling ide_end_request()
    > > will work there, but IMHO it's just cleaner to finish special commands
    > > inside ide_end_drive_cmd(). Currently,
    >
    > I agree but please note that your patch makes *all* taskfile registers to
    > be exposed through HDIO_DRIVE_TASKFILE regardless of ->rf_in_flags
    > (and obviously later you can't revert this change).
    >
    > > * Successful flagged taskfile -> ide_end_drive_cmd()
    > > * All other successful non-DMA special cmds -> ide_end_request()
    > > * Successful DMA taskfile -> segfault
    >
    > Have you tested it? Why would it segfault?
    >

    It's the same reason why PIO taskfiles were broken. rq->rq_disk is
    NULL for taskfile requests.

    ide_startstop_t ide_dma_intr (ide_drive_t *drive)
    {
    ...
    printk("**HERE0***\n");
    drv = *(ide_driver_t **)rq->rq_disk->private_data;;
    printk("**HERE1***\n");
    drv->end_request(drive, 1, rq->nr_sectors);
    return ide_stopped;
    ...
    }

    And, my test program does the following TASKFILE ioctl.

    memset(&task, 0, sizeof(task));
    task.req.out_size = sizeof(task.outbuf);
    task.req.in_size = sizeof(task.inbuf);

    task.req.io_ports[2] = 1; /* nsector */
    task.req.io_ports[3] = sector; /* LBA [7:0] */
    task.req.io_ports[4] = sector >> 8; /* LBA [15:8] */
    task.req.io_ports[5] = sector >> 16; /* LBA [23:16] */
    task.req.io_ports[6] = 0x40 | ((sector >> 24) & 0xf); /* select */
    task.req.io_ports[7] = WIN_READDMA; /* command */
    task.req.req_cmd = IDE_DRIVE_TASK_IN;
    task.req.data_phase = TASKFILE_IN_DMA;

    Then, the kernel oopses...

    **HERE0***
    Unable to handle kernel NULL pointer dereference at virtual address 00000038
    printing eip:
    c026bce4
    *pde = 00000000
    Oops: 0000 [#1]
    PREEMPT SMP
    Modules linked in: nfs lockd sunrpc e1000 eepro100 e100 mii floppy
    CPU: 0
    EIP: 0060:[<c026bce4>] Not tainted VLI
    EFLAGS: 00010082 (2.6.11-rc3-work-i386)
    EIP is at ide_dma_intr+0x84/0xd0
    eax: 00000000 ebx: f5e79d6c ecx: c03a678c edx: 00000001
    esi: c048b3b0 edi: c048b3b0 ebp: c0435efc esp: c0435ee0
    ds: 007b es: 007b ss: 0068


    > > * All failed special cmds -> ide_end_drive_cmd()
    > >
    > > It just shouldn't be like this. :-(
    >
    > Yep.
    >
    > > >
    > > > > * enables TASKFILE ioctls to get valid register outputs on
    > > > > successful completion.
    > > >
    > > > This change makes *all* taskfile registers to be read on completion
    > > > of *any* command. Currently this is done only for flagged taskfiles
    > > > and commands using no-data protocol.
    > > >
    > > > With all your changes it will be also done for:
    > > > * HDIO_DRIVE_[TASKFILE,CMD] ioctls
    > > > * /proc/ide/hd?/{identify,smart_thresholds,smart_values}
    > > > but reading back all registers is not always needed.
    > >
    > > None is on a hot path or even near to one, but maybe I don't have
    > > enough experience with old hardware. Are there some old hardware
    > > which make the additional reads stand out?
    >
    > I dunno, better be safe then sorry, after all we are working
    > on the *stable* kernel tree.
    >
    > Converting HDIO_DRIVE_CMD and in-kernel users to
    > soon-to-be-implemented ;-) sane discrete interface shouldn't
    > require much effort.
    >
    > > > It is already bad enough (and we can't fix it cause it is exported
    > > > to user-space through HDIO_DRIVE_TASKFILE), we shouldn't
    > > > make it worse.
    > >
    > > Yeah, the whole IDE ioctl interface seems disturbingly messy. :-(
    > >
    > > * Register output is available only if
    > > 1. The command fails.
    > > 2. The command is a flagged taskfile.
    >
    > 3. the command uses no-data protocol
    >

    Yes, that one. :-)

    > > * taskfile->device_head is used regardless of outflags setting.
    > > * In flagged taskfile, taskfile->device_head can turn on the device
    > > bit, so we can issue commands to hdb with permissions to hda.
    > > * In TASK and TASKFILE, LBA commands can be issued to drives in CHS
    > > mode but the reverse isn't true. However, in TASKFILE, if the
    > > command isn't flagged, the lower nibble of device register is
    > > zeroed depending on addressing setting.
    > > * taskfile->data endianess is reversed on big endian machines.
    > > * ide_reg_valid_t endianess issue.
    > > * And, none of above is documented.
    > >
    > > So, I don't know. Do you think we should keep all of the above
    > > behaviors? Please let me know; then, I'll update ioctl/hdio.txt so
    >
    > Now read again the list above and think about amount of time required
    > to *safely* fix all the issues mentioned vs introducing sane interface.
    > Please also note that once we expose something to user-space
    > applications may depend on the broken behavior so we can't fix all
    > issues anyway.

    So, I guess we're keeping most of them.

    > If somebody implements SG_IO ioctl and SCSI command pass-through
    > from libata for IDE driver (and add possibility for discrete taskfiles), we can
    > just deprecate HDIO_DRIVE_TASKFILE, forget about it and some time later
    > remove this FPOS.

    That sounds sweet.

    > > that people can at least know these gotchas.
    >
    > Please do so, thanks.

    Yes, I'll do.

    Thanks.

    --
    tejun

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