Messages in this thread |  | | | Date | Wed, 2 Mar 2005 01:49:52 +0900 | | From | Tejun Heo <> | | Subject | Re: [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/
|  |