| Message ID | 1471513714-11709-1-git-send-email-hpoussin@reactos.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Message-id: 1471513714-11709-1-git-send-email-hpoussin@reactos.org
Subject: [Qemu-devel] [PATCH] atapi: allow 0 transfer bytes for read_cd command
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/1471513714-11709-1-git-send-email-hpoussin@reactos.org -> patchew/1471513714-11709-1-git-send-email-hpoussin@reactos.org
Switched to a new branch 'test'
e2d1ba2 atapi: allow 0 transfer bytes for read_cd command
=== OUTPUT BEGIN ===
Checking PATCH 1/1: atapi: allow 0 transfer bytes for read_cd command...
ERROR: space prohibited after that open square bracket '['
#24: FILE: hw/ide/atapi.c:1292:
+ [ 0xbe ] = { cmd_read_cd, CHECK_READY | NONDATA },
ERROR: space prohibited before that close square bracket ']'
#24: FILE: hw/ide/atapi.c:1292:
+ [ 0xbe ] = { cmd_read_cd, CHECK_READY | NONDATA },
total: 2 errors, 0 warnings, 8 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Am 18.08.2016 um 11:48 hat Hervé Poussineau geschrieben: > This fixes Windows NT4 startup when a cdrom is inserted. > > Fixes: 9ef2e93f9b1888c7d0deb4a105149138e6ad2e98 > Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> Hm, which of the paths in cmd_read_cd() does this hit? Is it the one that directly calls ide_atapi_cmd_ok() without doing anything? I think adding NONDATA is okay, but we may need to add explicit atapi_byte_count_limit() == 0 checks to those paths that do transfer some data. At least at first sight I'm not sure that ide_atapi_cmd_read() can handle this. Kevin > hw/ide/atapi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > index 6189675..63312f2 100644 > --- a/hw/ide/atapi.c > +++ b/hw/ide/atapi.c > @@ -1289,7 +1289,7 @@ static const struct AtapiCmd { > [ 0xad ] = { cmd_read_dvd_structure, CHECK_READY }, > [ 0xbb ] = { cmd_set_speed, NONDATA }, > [ 0xbd ] = { cmd_mechanism_status, 0 }, > - [ 0xbe ] = { cmd_read_cd, CHECK_READY }, > + [ 0xbe ] = { cmd_read_cd, CHECK_READY | NONDATA }, > /* [1] handler detects and reports not ready condition itself */ > };
On 08/18/2016 05:48 AM, Hervé Poussineau wrote: > This fixes Windows NT4 startup when a cdrom is inserted. > > Fixes: 9ef2e93f9b1888c7d0deb4a105149138e6ad2e98 > Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> > --- > hw/ide/atapi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > index 6189675..63312f2 100644 > --- a/hw/ide/atapi.c > +++ b/hw/ide/atapi.c > @@ -1289,7 +1289,7 @@ static const struct AtapiCmd { > [ 0xad ] = { cmd_read_dvd_structure, CHECK_READY }, > [ 0xbb ] = { cmd_set_speed, NONDATA }, > [ 0xbd ] = { cmd_mechanism_status, 0 }, > - [ 0xbe ] = { cmd_read_cd, CHECK_READY }, > + [ 0xbe ] = { cmd_read_cd, CHECK_READY | NONDATA }, > /* [1] handler detects and reports not ready condition itself */ > }; > > What's the exact nature of the problem? I intended the "NONDATA" flag to be used exclusively for commands that do not return ANY information except for status return codes: /* * Commands flagged with NONDATA do not in any circumstances return * any data via ide_atapi_cmd_reply. These commands are exempt from * the normal byte_count_limit constraints. * See ATA8-ACS3 "7.21.5 Byte Count Limit" */ NONDATA = 0x04, I wouldn't be comfortable applying it to a command that DID indeed return data under some circumstances... though you're right that if the command as delivered returns no data, the ATAPI layer is allowed to process that request absent byte_count_limit being programmed. You may need to adjust near this line in ide_atapi_cmd: if (cmd->handler && !(cmd->flags & NONDATA)) { to allow or disallow more commands as appropriate. It looks to me sadly as if there is no hard and fast rule available to tell which commands must set the BCL mandatorily ... and putting the check in the data transfer itself puts us at risk for not aborting the command early enough. I'll try to address this post-KVM forum, if you don't solve it by then. --js
Le 18/08/2016 à 16:24, Kevin Wolf a écrit : > Hm, which of the paths in cmd_read_cd() does this hit? Is it the one > that directly calls ide_atapi_cmd_ok() without doing anything? This is in ide_atapi_cmd, at line: if (cmd->handler && !(cmd->flags & NONDATA)) { handler is cmd_read_cd and flags doesn't contain NONDATA and atapi_byte_count_limit is 0 and atapi_dma is false, so command is aborted. Adding NONDATA flag prevents this command abort. > > I think adding NONDATA is okay, but we may need to add explicit > atapi_byte_count_limit() == 0 checks to those paths that do transfer > some data. At least at first sight I'm not sure that > ide_atapi_cmd_read() can handle this. > ATAPI packet is: ATAPI limit=0x0 packet: be 00 00 00 00 00 00 00 00 00 00 00 Note that byte count limit is 0x0. I also checked that s->packet_dma is false. cmd_read_cd calculates nb_sectors using buf[6], buf[7] and buf[8] => nb_sectors = 0. There is a specific case in cmd_read_cd if nb_sectors == 0, which succeeds the command. So, we have four cases: a) byte limit == 0 && nb_sectors == 0 -> used by NT4, currently is aborting the command in ide_atapi_cmd b) byte limit == 0 && nb_sectors != 0 -> command is aborted in ide_atapi_cmd c) byte limit != 0 && nb_sectors == 0 -> command succeeds in cmd_read_cd d) byte limit != 0 && nb_sectors != 0 -> usual case, works fine Maybe we should add NONDATA flag for cmd_read_cd command, and add on top of cmd_read_cd - if nb_sectors == 0, succeed command (for cases a and c) - if byte limit == 0 && nb_sectors != 0, abort command (for case b) - otherwise, process as usual (for case d) Hervé
Am 21.08.2016 um 23:16 hat Hervé Poussineau geschrieben: > Le 18/08/2016 à 16:24, Kevin Wolf a écrit : > >Hm, which of the paths in cmd_read_cd() does this hit? Is it the one > >that directly calls ide_atapi_cmd_ok() without doing anything? > > This is in ide_atapi_cmd, at line: > if (cmd->handler && !(cmd->flags & NONDATA)) { > handler is cmd_read_cd and flags doesn't contain NONDATA and atapi_byte_count_limit is 0 and atapi_dma is false, so command is aborted. > Adding NONDATA flag prevents this command abort. > > > > >I think adding NONDATA is okay, but we may need to add explicit > >atapi_byte_count_limit() == 0 checks to those paths that do transfer > >some data. At least at first sight I'm not sure that > >ide_atapi_cmd_read() can handle this. > > > > ATAPI packet is: > ATAPI limit=0x0 packet: be 00 00 00 00 00 00 00 00 00 00 00 > Note that byte count limit is 0x0. > I also checked that s->packet_dma is false. > > cmd_read_cd calculates nb_sectors using buf[6], buf[7] and buf[8] => nb_sectors = 0. > There is a specific case in cmd_read_cd if nb_sectors == 0, which succeeds the command. > > So, we have four cases: > a) byte limit == 0 && nb_sectors == 0 -> used by NT4, currently is aborting the command in ide_atapi_cmd > b) byte limit == 0 && nb_sectors != 0 -> command is aborted in ide_atapi_cmd > c) byte limit != 0 && nb_sectors == 0 -> command succeeds in cmd_read_cd > d) byte limit != 0 && nb_sectors != 0 -> usual case, works fine > > Maybe we should add NONDATA flag for cmd_read_cd command, and add on top of cmd_read_cd > - if nb_sectors == 0, succeed command (for cases a and c) > - if byte limit == 0 && nb_sectors != 0, abort command (for case b) > - otherwise, process as usual (for case d) Yes, for the part about nb_sectors, this sounds about right. I see annother immediate ide_atapi_cmd_ok() in the switch for (transfer_request & 0xf8 == 0). I think this needs to be considered in the check as well. Kevin
On 08/21/2016 05:16 PM, Hervé Poussineau wrote: > Le 18/08/2016 à 16:24, Kevin Wolf a écrit : >> Hm, which of the paths in cmd_read_cd() does this hit? Is it the one >> that directly calls ide_atapi_cmd_ok() without doing anything? > > This is in ide_atapi_cmd, at line: > if (cmd->handler && !(cmd->flags & NONDATA)) { > handler is cmd_read_cd and flags doesn't contain NONDATA and > atapi_byte_count_limit is 0 and atapi_dma is false, so command is aborted. > Adding NONDATA flag prevents this command abort. > >> >> I think adding NONDATA is okay, but we may need to add explicit >> atapi_byte_count_limit() == 0 checks to those paths that do transfer >> some data. At least at first sight I'm not sure that >> ide_atapi_cmd_read() can handle this. >> > > ATAPI packet is: > ATAPI limit=0x0 packet: be 00 00 00 00 00 00 00 00 00 00 00 > Note that byte count limit is 0x0. > I also checked that s->packet_dma is false. > > cmd_read_cd calculates nb_sectors using buf[6], buf[7] and buf[8] => > nb_sectors = 0. > There is a specific case in cmd_read_cd if nb_sectors == 0, which > succeeds the command. > > So, we have four cases: > a) byte limit == 0 && nb_sectors == 0 -> used by NT4, currently is > aborting the command in ide_atapi_cmd > b) byte limit == 0 && nb_sectors != 0 -> command is aborted in > ide_atapi_cmd > c) byte limit != 0 && nb_sectors == 0 -> command succeeds in cmd_read_cd > d) byte limit != 0 && nb_sectors != 0 -> usual case, works fine > > Maybe we should add NONDATA flag for cmd_read_cd command, and add on top > of cmd_read_cd > - if nb_sectors == 0, succeed command (for cases a and c) > - if byte limit == 0 && nb_sectors != 0, abort command (for case b) > - otherwise, process as usual (for case d) > > Hervé What a mess. I guess there's really no way to -- at the command dispatch level -- tell whether or not having a BCL of 0 is a problem. It's literally up to the command handlers themselves. That's really unfortunate. So at this point, it's important to rename this flag. When I introduced it, I was under the impression that commands could either return data or not, and if they did, that the byte limit must be set. If there are commands which can do both, "NONDATA" gets a lot less meaningful. I might ask that we rename it BCL0 or something -- this command is allowed to process commands with a BCL of zero -- and then those commands will also be responsible for further checking if that's truly OK. If you respin, please cc stable for 2.7.1. Sorry for the long delay. --js
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 6189675..63312f2 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -1289,7 +1289,7 @@ static const struct AtapiCmd { [ 0xad ] = { cmd_read_dvd_structure, CHECK_READY }, [ 0xbb ] = { cmd_set_speed, NONDATA }, [ 0xbd ] = { cmd_mechanism_status, 0 }, - [ 0xbe ] = { cmd_read_cd, CHECK_READY }, + [ 0xbe ] = { cmd_read_cd, CHECK_READY | NONDATA }, /* [1] handler detects and reports not ready condition itself */ };
This fixes Windows NT4 startup when a cdrom is inserted. Fixes: 9ef2e93f9b1888c7d0deb4a105149138e6ad2e98 Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> --- hw/ide/atapi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)