Hi all,
while trying to limit the speed of DVD drives during DVD playback I've
come under the impression that GPCMD_SET_STREAMING via SG_IO doesn't yet
work with libata/PATA (AMD/NVIDIA PATA support). I tested on an NForce2
board (all IDE, no SATA) and kernel 2.6.22.12. When I use the "old" ATA
kernel driver my drive slows down, with libata it doesn't. Is this a
known issue or has this slipped by unnoticed?
I tried with mplayer dvd:// -dvd-device /dev/sr0 -dvd-speed BTW . I get
no errors in syslog/dmesg. Also mplayer doesn't report a problem (so
ioctl(fd, SG_IO, &sghdr) doesn't have a return < 0).
Regards
Sebastian
> while trying to limit the speed of DVD drives during DVD playback I've
> come under the impression that GPCMD_SET_STREAMING via SG_IO doesn't yet
> work with libata/PATA (AMD/NVIDIA PATA support). I tested on an NForce2
> board (all IDE, no SATA) and kernel 2.6.22.12. When I use the "old" ATA
> kernel driver my drive slows down, with libata it doesn't. Is this a
> known issue or has this slipped by unnoticed?
It isn't a known issue, and it suprises me as SG_IO basically passes
commands through to the drive. We don't support speed change via xfermode
setting but GPCMD_SET_STREAMING sohuld behave.
>
> I tried with mplayer dvd:// -dvd-device /dev/sr0 -dvd-speed BTW . I get
> no errors in syslog/dmesg. Also mplayer doesn't report a problem (so
> ioctl(fd, SG_IO, &sghdr) doesn't have a return < 0).
Do you have a simple code example that shows the problem ?
Hi Alan!
On Tue, Nov 13, 2007 at 09:22:30PM +0000, Alan Cox wrote:
> It isn't a known issue, and it suprises me as SG_IO basically passes
> commands through to the drive. We don't support speed change via xfermode
> setting but GPCMD_SET_STREAMING sohuld behave.
>
> Do you have a simple code example that shows the problem ?
http://svn.mplayerhq.hu/mplayer/trunk/stream/stream_dvd.c?view=markup
See dvd_set_speed(). The drive I'm using is an Optiarc DVD RW AD-7170A.
With the "old" ATA driver dvd_set_speed() works, with libata it doesn't.
Regards
Sebastian
On Wed, Nov 14, 2007 at 12:28:59AM +0100, Sebastian Kemper wrote:
> http://svn.mplayerhq.hu/mplayer/trunk/stream/stream_dvd.c?view=markup
>
> See dvd_set_speed(). The drive I'm using is an Optiarc DVD RW AD-7170A.
> With the "old" ATA driver dvd_set_speed() works, with libata it doesn't.
Hello Alan,
in the meantime I tried the kernels 2.6.23.1 as well as 2.6.24-rc2. Both
show the same behaviour when using libata with AMD/NVIDIA PATA support:
GPCMD_SET_STREAMING via SG_IO doesn't have any effect. With these two
kernels I didn't try the "old" ATA driver - but I assume it would've
worked out (changing your system from ATA to libata and vice versa
turned out to be quite time consuming ;)).
Regards
Sebastian
Sebastian Kemper wrote:
> Hi Alan!
>
> On Tue, Nov 13, 2007 at 09:22:30PM +0000, Alan Cox wrote:
>> It isn't a known issue, and it suprises me as SG_IO basically passes
>> commands through to the drive. We don't support speed change via xfermode
>> setting but GPCMD_SET_STREAMING sohuld behave.
>>
>> Do you have a simple code example that shows the problem ?
>
> http://svn.mplayerhq.hu/mplayer/trunk/stream/stream_dvd.c?view=markup
>
> See dvd_set_speed(). The drive I'm using is an Optiarc DVD RW AD-7170A.
> With the "old" ATA driver dvd_set_speed() works, with libata it doesn't.
..
I think the problem here might be that libata doesn't actually support SG_IO
for ATAPI drives prior to 2.6.24 (ugh).
Try it with a 2.6.24-rc* kernel from kernel.org, or back-patch the ATA_16
SG_IO support into your older kernel.
Cheers
On Wed, Nov 14, 2007 at 10:09:22AM -0500, Mark Lord wrote:
> I think the problem here might be that libata doesn't actually support
> SG_IO
> for ATAPI drives prior to 2.6.24 (ugh).
>
> Try it with a 2.6.24-rc* kernel from kernel.org, or back-patch the ATA_16
> SG_IO support into your older kernel.
Hi Mark,
I already tried 2.6.24-rc2 and it didn't work, see my last reply:
http://marc.info/?l=linux-kernel&m=119504244805467&w=2
Regards
Sebastian
Sebastian Kemper wrote:
> On Wed, Nov 14, 2007 at 10:09:22AM -0500, Mark Lord wrote:
>> I think the problem here might be that libata doesn't actually support
>> SG_IO
>> for ATAPI drives prior to 2.6.24 (ugh).
>>
>> Try it with a 2.6.24-rc* kernel from kernel.org, or back-patch the ATA_16
>> SG_IO support into your older kernel.
>
> Hi Mark,
>
> I already tried 2.6.24-rc2 and it didn't work, see my last reply:
> http://marc.info/?l=linux-kernel&m=119504244805467&w=2
..
Yeah, I'm a bit drowsy this morning -- the set speed command
doesn't use ATA_16, so it should work with any kernel.
I'll try it here soon-ish, since I need to update "hdparm -E" for libata anyway.
Cheers
Sebastian Kemper wrote:
> Hi Alan!
>
> On Tue, Nov 13, 2007 at 09:22:30PM +0000, Alan Cox wrote:
>> It isn't a known issue, and it suprises me as SG_IO basically passes
>> commands through to the drive. We don't support speed change via xfermode
>> setting but GPCMD_SET_STREAMING sohuld behave.
>>
>> Do you have a simple code example that shows the problem ?
>
> http://svn.mplayerhq.hu/mplayer/trunk/stream/stream_dvd.c?view=markup
..
That code is riddled with bugs, by the way.
It fails to close/clean-up on just about every exit path.
But apart from that, it does appear to issue the command.
Another way to the same thing is with "hdparm -E",
which contrary to my earlier posting actually does
seem to work already with libata.
> See dvd_set_speed(). The drive I'm using is an Optiarc DVD RW AD-7170A.
> With the "old" ATA driver dvd_set_speed() works, with libata it doesn't.
..
Can you define "doesn't work" for me?
How can I test this to see if it works one way or another ?
The command issue (ioctl) is not returning -1.
Mark Lord wrote:
> Sebastian Kemper wrote:
>> Hi Alan!
>>
>> On Tue, Nov 13, 2007 at 09:22:30PM +0000, Alan Cox wrote:
>>> It isn't a known issue, and it suprises me as SG_IO basically passes
>>> commands through to the drive. We don't support speed change via
>>> xfermode
>>> setting but GPCMD_SET_STREAMING sohuld behave.
>>>
>>> Do you have a simple code example that shows the problem ?
>>
>> http://svn.mplayerhq.hu/mplayer/trunk/stream/stream_dvd.c?view=markup
> ..
>
> That code is riddled with bugs, by the way.
> It fails to close/clean-up on just about every exit path.
> But apart from that, it does appear to issue the command.
>
> Another way to the same thing is with "hdparm -E",
> which contrary to my earlier posting actually does
> seem to work already with libata.
>
>> See dvd_set_speed(). The drive I'm using is an Optiarc DVD RW AD-7170A.
>> With the "old" ATA driver dvd_set_speed() works, with libata it doesn't.
> ..
>
> Can you define "doesn't work" for me?
> How can I test this to see if it works one way or another ?
>
> The command issue (ioctl) is not returning -1.
..
Ahh.. got it. The host_status returned (not checked by that code) was 7,
which means "host error".
In this case, that's because the cmd_len is (16), which is too large for ATAPI.
It needs to be changed to (12) instead.
So this line: sghdr.cmd_len = 12;
The code (apart from totally mishandling any kinds of errors) works after that.
-ml
Hi Mark!
On Wed, Nov 14, 2007 at 11:41:37AM -0500, Mark Lord wrote:
> Ahh.. got it. The host_status returned (not checked by that code) was 7,
> which means "host error".
>
> In this case, that's because the cmd_len is (16), which is too large for
> ATAPI.
> It needs to be changed to (12) instead.
I don't understand. You seem to use a cmd_len of 16 in hdparm yourself.
And why does it work with the "old" ATA driver and not with libata if
16 is too large for ATAPI in general?
> The code (apart from totally mishandling any kinds of errors) works after
> that.
Greetings
Sebastian
On Wed, Nov 14, 2007 at 11:26:06AM -0500, Mark Lord wrote:
> Another way to the same thing is with "hdparm -E",
> which contrary to my earlier posting actually does
> seem to work already with libata.
Hi!
There are DVD-ROM drives that don't react to hdparm -E but do react to
SET_STREAMING, for instance some NEC drives.
Regards
Sebastian
Sebastian Kemper wrote:
> Hi Mark!
>
> On Wed, Nov 14, 2007 at 11:41:37AM -0500, Mark Lord wrote:
>> Ahh.. got it. The host_status returned (not checked by that code) was 7,
>> which means "host error".
>>
>> In this case, that's because the cmd_len is (16), which is too large for
>> ATAPI.
>> It needs to be changed to (12) instead.
>
> I don't understand. You seem to use a cmd_len of 16 in hdparm yourself.
..
Really? Where?
> And why does it work with the "old" ATA driver and not with libata if
> 16 is too large for ATAPI in general?
..
Now that *is* the question.
And the answer appears to be that ide-cd.c ignores the passed-in cmd_len,
and replaces it with:
cmd_len = COMMAND_SIZE(rq->cmd[0]);
That's a SCSI macro to calculate the correct cmd_len based on the SCSI opcode,
which is very appropriate here. We should do that too (we don't) in libata.
But not exactly as IDE does it -- that's actually a bug: the code needs to also
check that the new cmd_len is no larger than the original, or we'll get an Oops.
I'll cook up a patch for that shortly and try it before posting it here.
-ml
Sebastian Kemper reported that issuing CD/DVD commands under libata
is not fully compatible with ide-scsi. In particular, the GPCMD_SET_STREAMING
was being rejected at the host level in some instances.
The reason is that libata-scsi insists upon the cmd_len field exactly matching
the SCSI opcode being issued, whereas ide-scsi tolerates 12-byte commands
contained within a 16-byte (cmd_len) CDB.
There doesn't seem to be a good reason for us to not be compatible there,
so here is a patch to fix libata-scsi to permit SCSI opcodes so long as
they fit within whatever size CDB is provided.
Signed-off-by: Mark Lord <[email protected]>
---
Patch is against 2.6.24-rc2-git4.
Sebastian, could you please re-test with this patch
and let us know that it works for you (or not).
--- old/drivers/ata/libata-scsi.c 2007-11-13 23:25:15.000000000 -0500
+++ linux/drivers/ata/libata-scsi.c 2007-11-14 12:32:16.000000000 -0500
@@ -2869,7 +2869,8 @@
xlat_func = NULL;
if (likely((scsi_op != ATA_16) || !atapi_passthru16)) {
/* relay SCSI command to ATAPI device */
- if (unlikely(scmd->cmd_len > dev->cdb_len))
+ int len = COMMAND_SIZE(scsi_op);
+ if (unlikely(len > scmd->cmd_len || len > dev->cdb_len))
goto bad_cdb_len;
xlat_func = atapi_xlat;
Sebastian Kemper wrote:
> On Wed, Nov 14, 2007 at 11:26:06AM -0500, Mark Lord wrote:
>> Another way to the same thing is with "hdparm -E",
>> which contrary to my earlier posting actually does
>> seem to work already with libata.
>
> Hi!
>
> There are DVD-ROM drives that don't react to hdparm -E but do react to
> SET_STREAMING, for instance some NEC drives.
..
Mmm.. that's possible. The NEC drive I have here works with either.
I wonder if an update to sr_select_speed() (inside the SCSI code) is needed?
It currently uses GPCMD_SET_SPEED, with no fallback to GPCMD_SET_STREAMING.
Or I suppose this is best left to userspace.
I may change hdparm to try both opcodes.
On Wed, Nov 14, 2007 at 12:44:23PM -0500, Mark Lord wrote:
> Sebastian Kemper reported that issuing CD/DVD commands under libata
> is not fully compatible with ide-scsi. In particular, the
> GPCMD_SET_STREAMING
> was being rejected at the host level in some instances.
>
> The reason is that libata-scsi insists upon the cmd_len field exactly
> matching
> the SCSI opcode being issued, whereas ide-scsi tolerates 12-byte commands
> contained within a 16-byte (cmd_len) CDB.
>
> There doesn't seem to be a good reason for us to not be compatible there,
> so here is a patch to fix libata-scsi to permit SCSI opcodes so long as
> they fit within whatever size CDB is provided.
>
> Signed-off-by: Mark Lord <[email protected]>
Acked-by: Alan Cox <[email protected]>
Mark Lord wrote:
> Sebastian Kemper reported that issuing CD/DVD commands under libata
> is not fully compatible with ide-scsi. In particular, the
> GPCMD_SET_STREAMING
> was being rejected at the host level in some instances.
>
> The reason is that libata-scsi insists upon the cmd_len field exactly
> matching
> the SCSI opcode being issued, whereas ide-scsi tolerates 12-byte commands
> contained within a 16-byte (cmd_len) CDB.
>
> There doesn't seem to be a good reason for us to not be compatible there,
> so here is a patch to fix libata-scsi to permit SCSI opcodes so long as
> they fit within whatever size CDB is provided.
>
> Signed-off-by: Mark Lord <[email protected]>
applied to #tj-upstream-fixes.
--
tejun
Hi,
On Wednesday 14 November 2007, Mark Lord wrote:
> Sebastian Kemper wrote:
> > Hi Mark!
> >
> > On Wed, Nov 14, 2007 at 11:41:37AM -0500, Mark Lord wrote:
> >> Ahh.. got it. The host_status returned (not checked by that code) was 7,
> >> which means "host error".
> >>
> >> In this case, that's because the cmd_len is (16), which is too large for
> >> ATAPI.
> >> It needs to be changed to (12) instead.
> >
> > I don't understand. You seem to use a cmd_len of 16 in hdparm yourself.
> ..
>
> Really? Where?
>
>
> > And why does it work with the "old" ATA driver and not with libata if
> > 16 is too large for ATAPI in general?
> ..
>
> Now that *is* the question.
> And the answer appears to be that ide-cd.c ignores the passed-in cmd_len,
> and replaces it with:
>
> cmd_len = COMMAND_SIZE(rq->cmd[0]);
>
> That's a SCSI macro to calculate the correct cmd_len based on the SCSI opcode,
> which is very appropriate here. We should do that too (we don't) in libata.
>
> But not exactly as IDE does it -- that's actually a bug: the code needs to also
> check that the new cmd_len is no larger than the original, or we'll get an Oops.
>
> I'll cook up a patch for that shortly and try it before posting it here.
Could you please also fix ide-cd while at it?
Thanks,
Bart
Bartlomiej Zolnierkiewicz wrote:
> Hi,
>
> On Wednesday 14 November 2007, Mark Lord wrote:
>> Sebastian Kemper wrote:
>>> Hi Mark!
>>>
>>> On Wed, Nov 14, 2007 at 11:41:37AM -0500, Mark Lord wrote:
>>>> Ahh.. got it. The host_status returned (not checked by that code) was 7,
>>>> which means "host error".
>>>>
>>>> In this case, that's because the cmd_len is (16), which is too large for
>>>> ATAPI.
>>>> It needs to be changed to (12) instead.
>>> I don't understand. You seem to use a cmd_len of 16 in hdparm yourself.
>> ..
>>
>> Really? Where?
>>
>>
>>> And why does it work with the "old" ATA driver and not with libata if
>>> 16 is too large for ATAPI in general?
>> ..
>>
>> Now that *is* the question.
>> And the answer appears to be that ide-cd.c ignores the passed-in cmd_len,
>> and replaces it with:
>>
>> cmd_len = COMMAND_SIZE(rq->cmd[0]);
>>
>> That's a SCSI macro to calculate the correct cmd_len based on the SCSI opcode,
>> which is very appropriate here. We should do that too (we don't) in libata.
>>
>> But not exactly as IDE does it -- that's actually a bug: the code needs to also
>> check that the new cmd_len is no larger than the original, or we'll get an Oops.
>>
>> I'll cook up a patch for that shortly and try it before posting it here.
>
> Could you please also fix ide-cd while at it?
..
ide-cd already does the COMMAND_SIZE() thingie, so that fix is not needed.
The only bug there is that it doesn't check for the possibility
of a 16-byte opcode being issued with only a 12-byte CDB underneath.
In practice, this doesn't seem to hurt anything.
I'm no longer familiar enough with the code there to reliably fix it.
Cheers