Hi Alan,
Jens seems too busy to check out this patch. I post it here again
hopefully to get boarder audiences:
VMware try to use the generic packet interface (CDROM_SEND_PACKET)
for cdrom simulation. There are some packet command used by windows
can return different data size, depend on the type of CD in the CDROM.
Current linux kernel will fail the ioctl call if packet command return
data less than expected.
ide-scsi driver do not have this problem.
We make a patch allow kernel return successful and return the actual
transfer data size. Is it the prefer behavior in this case? If not,
what is the best way to solve this problem?
The original email and the patch as follows.
Thanks
Chris
P.S. I am very surprised to find out that, vmware suffers from bugs
in cdrom driver for years. Developers give up after some attempt to
submit patches to kernel, it is not easy to make it right at the first
time. The broken sense data bug should have been fix long time ago if
they try hard enough.
"Alexander (Sasha) Malchik" wrote:
>
> Hello Jens,
> I would like to propose another patch to the Linux IDE CD driver.
> The driver fails any packet command operation in case the amount of data
> written into the buffer was less than the buflen passed with the packet
> command. It happens in the interrupt handler ide-cd:cdrom_pc_intr() in
> these lines:
> /* If DRQ is clear, the command has completed.
> Complain if we still have data left to transfer. */
> if ((stat & DRQ_STAT) == 0) {
> /* ... deleted - pad buffer with 0's for REQUEST_SENSE command */
> if (pc->buflen == 0)
> cdrom_end_request (1, drive);
> else {
> /* Comment this out, because this always happens
> right after a reset occurs, and it is annoying to
> always print expected stuff. */
> /*
> printk ("%s: cdrom_pc_intr: data underrun %d\n",
> drive->name, pc->buflen);
> */
> pc->stat = 1;
> cdrom_end_request (1, drive);
> }
> return ide_stopped;
> The SCSI driver does no such thing. It does not care if less than
> pc->buflen amount of data existed. It simply writes to the buffer and
> returns the whole thing to userland.
>
> We are trying to use the generic packet interface (CDROM_SEND_PACKET)
> for all CDROM accesses in VMware. The idea is to pass through all ATAPI
> packets to the host driver. But this strict behaviour of ide-cd driver
> presents a big problem to using this interface. There is no way to know
> the amount of data in flight for most packets in PIO mode, other than
> trying to figure it out from the Allocation Length part of the packets
> themselves. Unfortunately, the packets routinely issued by Windows OSes
> have allocation length far greater than the amount of data available
> from the device. For example, it issues READ_TOC packet (0x43) with
> allocation length 0x03 0x24, i.e. 804 bytes. In case data CD is in the
> drive, there are in fact only 20 bytes returned by the device, and in
> case of audio CD - 124 bytes. Passing cgc->buflen any larger than 20 or
> 124 bytes, correspondingly, fails that ioctl() call. The READ_TOC packet
> in question coming from the guest OS is identical. There are similar
> problems for MODE_SENSE, READ_CD, and many other commands - including
> multiple variations for all of their parameters. In the DMA case, those
> "incorrect" buflen's are also the values determined from the scatter
> gather table, so the situation is the same.c
>
> I don't see a reason why IDE CD driver has to fail the requests in this
> case, and differ from the SCSI driver. Because ioctl() is flagged as
> failing, the buffer is then NOT copied into the userland in
> cdrom.c:cdrom_do_cmd():
> ret = cdi->ops->generic_packet(cdi, cgc);
> __copy_to_user(usense, cgc->sense, sizeof(*usense));
> if (!ret && cgc->data_direction == CGC_DATA_READ) <== ret is !=0
> __copy_to_user(ubuf, cgc->buffer, cgc->buflen);
>
> Why not just return buflen actually written into the buffer instead?
> Here is the patch that does just that.
> Thank you very much for your review,
> Sasha Malchik.
>
--- 1.17/drivers/ide/ide-cd.c Mon Sep 16 22:39:10 2002
+++ edited/ide-cd.c Thu Oct 31 14:08:08 2002
@@ -1322,8 +1322,7 @@
ireason = IN_BYTE (IDE_NSECTOR_REG);
len = IN_BYTE (IDE_LCYL_REG) + 256 * IN_BYTE (IDE_HCYL_REG);
- /* If DRQ is clear, the command has completed.
- Complain if we still have data left to transfer. */
+ /* If DRQ is clear, the command has completed. */
if ((stat & DRQ_STAT) == 0) {
/* Some of the trailing request sense fields are optional, and
some drives don't send them. Sigh. */
@@ -1336,19 +1335,8 @@
}
}
- if (pc->buflen == 0)
- cdrom_end_request (1, drive);
- else {
- /* Comment this out, because this always happens
- right after a reset occurs, and it is annoying to
- always print expected stuff. */
- /*
- printk ("%s: cdrom_pc_intr: data underrun %d\n",
- drive->name, pc->buflen);
- */
- pc->stat = 1;
- cdrom_end_request (1, drive);
- }
+ cdrom_end_request (1, drive);
+
return ide_stopped;
}
@@ -2202,6 +2190,7 @@
pc.quiet = cgc->quiet;
pc.timeout = cgc->timeout;
pc.sense = cgc->sense;
+ cgc->buflen -= pc.buflen;
return cgc->stat = cdrom_queue_packet_command(drive, &pc);
}
On Thu, Oct 31 2002, [email protected] wrote:
> Hi Alan,
>
> Jens seems too busy to check out this patch. I post it here again
> hopefully to get boarder audiences:
Yes sorry, it's fallen through the cracks.
> VMware try to use the generic packet interface (CDROM_SEND_PACKET)
> for cdrom simulation. There are some packet command used by windows
> can return different data size, depend on the type of CD in the CDROM.
> Current linux kernel will fail the ioctl call if packet command return
> data less than expected.
>
> ide-scsi driver do not have this problem.
>
> We make a patch allow kernel return successful and return the actual
> transfer data size. Is it the prefer behavior in this case? If not,
> what is the best way to solve this problem?
The patch does look good, thanks.
> P.S. I am very surprised to find out that, vmware suffers from bugs
> in cdrom driver for years. Developers give up after some attempt to
> submit patches to kernel, it is not easy to make it right at the first
> time. The broken sense data bug should have been fix long time ago if
> they try hard enough.
I can only say resend and resend. It's no secret that I regurlarly loose
patches and don't respond to emails, because there are just so many of
them.
--
Jens Axboe
On Fri, Nov 01, 2002 at 01:10:45PM +0100, Jens Axboe wrote:
> On Thu, Oct 31 2002, [email protected] wrote:
>
> Yes sorry, it's fallen through the cracks.
Thanks for your quick reply. I am sorry that I forget to CC you
on the mail, my bad.
>
> > VMware try to use the generic packet interface (CDROM_SEND_PACKET)
> > for cdrom simulation. There are some packet command used by windows
> > can return different data size, depend on the type of CD in the CDROM.
> > Current linux kernel will fail the ioctl call if packet command return
> > data less than expected.
> >
> > ide-scsi driver do not have this problem.
> >
> > We make a patch allow kernel return successful and return the actual
> > transfer data size. Is it the prefer behavior in this case? If not,
> > what is the best way to solve this problem?
>
> The patch does look good, thanks.
Thank you so much for evaluate the patch. Any idea which kernel it will
get in? We can update our support document:
"for complete raw cdrom support, please upgrade to kernel 2.4.xx or later."
>
> I can only say resend and resend. It's no secret that I regurlarly loose
> patches and don't respond to emails, because there are just so many of
> them.
We all understand the importance of the resubmit mechanism now.
Actually every body here is very exciting about the patch been accepted.
It solve some long lasting problem and those ugly hack can go away soon.
That exactly why I love open source.
Thanks
Chris
On Fri, Nov 01, 2002 at 01:10:45PM +0100, Jens Axboe wrote:
> > We make a patch allow kernel return successful and return the actual
> > transfer data size. Is it the prefer behavior in this case? If not,
> > what is the best way to solve this problem?
>
> The patch does look good, thanks.
Sasha just find my change to the patch has some fault. The pc.buflen
changed after cdrom_queue_packet_command. So his original patch
is more correct.
I paste it here again. I am sorry for the confusion.
Chris
--- 1.17/drivers/ide/ide-cd.c Mon Sep 16 22:39:10 2002
+++ edited/ide-cd.c Sat Nov 2 01:02:43 2002
@@ -1322,8 +1322,7 @@
ireason = IN_BYTE (IDE_NSECTOR_REG);
len = IN_BYTE (IDE_LCYL_REG) + 256 * IN_BYTE (IDE_HCYL_REG);
- /* If DRQ is clear, the command has completed.
- Complain if we still have data left to transfer. */
+ /* If DRQ is clear, the command has completed. */
if ((stat & DRQ_STAT) == 0) {
/* Some of the trailing request sense fields are optional, and
some drives don't send them. Sigh. */
@@ -1336,19 +1335,8 @@
}
}
- if (pc->buflen == 0)
- cdrom_end_request (1, drive);
- else {
- /* Comment this out, because this always happens
- right after a reset occurs, and it is annoying to
- always print expected stuff. */
- /*
- printk ("%s: cdrom_pc_intr: data underrun %d\n",
- drive->name, pc->buflen);
- */
- pc->stat = 1;
- cdrom_end_request (1, drive);
- }
+ cdrom_end_request (1, drive);
+
return ide_stopped;
}
@@ -2202,7 +2190,9 @@
pc.quiet = cgc->quiet;
pc.timeout = cgc->timeout;
pc.sense = cgc->sense;
- return cgc->stat = cdrom_queue_packet_command(drive, &pc);
+ cgc->stat = cdrom_queue_packet_command(drive, &pc);
+ cgc->buflen -= pc.buflen;
+ return cgc->stat;
}
On Sat, Nov 02 2002, [email protected] wrote:
> On Fri, Nov 01, 2002 at 01:10:45PM +0100, Jens Axboe wrote:
> > > We make a patch allow kernel return successful and return the actual
> > > transfer data size. Is it the prefer behavior in this case? If not,
> > > what is the best way to solve this problem?
> >
> > The patch does look good, thanks.
>
> Sasha just find my change to the patch has some fault. The pc.buflen
> changed after cdrom_queue_packet_command. So his original patch
> is more correct.
>
> I paste it here again. I am sorry for the confusion.
Depends on whether you want the residual or the amount transferred.
Amount transferred probably makes more sense here, I agree.
--
Jens Axboe