linux-kernel - Re: [PATCH] [SCSI]: print the msgbytes and statusbyte from scsi result

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKYAXd-thiuWagZPf8825=eKLQPyx3w+h5vzKXn_O_g7n9CpDg@mail.gmail.com>
Date:	Mon, 18 Mar 2013 20:57:38 +0900
From:	Namjae Jeon <linkinjeon@...il.com>
To:	James Bottomley <James.Bottomley@...senpartnership.com>
Cc:	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	Namjae Jeon <namjae.jeon@...sung.com>,
	Amit Sahrawat <a.sahrawat@...sung.com>
Subject: Re: [PATCH] [SCSI]: print the msgbytes and statusbyte from scsi result

2013/3/18, James Bottomley <James.Bottomley@...senpartnership.com>:
> On Sun, 2013-03-17 at 17:29 +0900, Namjae Jeon wrote:
>> From: Namjae Jeon <namjae.jeon@...sung.com>
>>
>> Introduce msgbyte and statusbyte in the prints as part of the
>> result which is returned by the lower layer driver in response to
>> SCSI command issued, in case of any error conditions.
>>
>> Purpose of adding these prints is to convey, during any I/O
>> error case, which condition exactly has happened in lower device and
>> from the prints we can directly deduce, what is the status of command
>> issued. This will help to quickly debug the scenario and also making
>> a test case to create new scenarios.
>>
>> Also change the printk to more appropriate pr_* macro.
>>
>> Signed-off-by: Namjae Jeon <namjae.jeon@...sung.com>
>> Signed-off-by: Amit Sahrawat <a.sahrawat@...sung.com>
>> ---
>>  drivers/scsi/constants.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
>> index 76e4c03..77bb1dc 100644
>> --- a/drivers/scsi/constants.c
>> +++ b/drivers/scsi/constants.c
>> @@ -1445,8 +1445,10 @@ void scsi_show_result(int result)
>>
>>  void scsi_show_result(int result)
>>  {
>> -	printk("Result: hostbyte=0x%02x driverbyte=0x%02x\n",
>> -	       host_byte(result), driver_byte(result));
>> +	pr_info("Result: hostbyte=0x%02x driverbyte=0x%02x"
>> +			"msgbyte=0x%02x statusbyte=0x%02x\n",
>> +	       host_byte(result), driver_byte(result), msg_byte(result),
>> +							status_byte(result));
Hi James.
>
> You didn't test this, did you? If you did, you'd have noticed the change
> from printk to pr_info gives you an unwanted "6" in the message.
Yes, we tested with "printk" version of the patch, but before sending
the patch, when we checked for issues using "checkpatch.pl" it showed
warning.
So, we thought that to be a cosmetic change and replaced the printk
with  pr_info. Sorry, if below your douting is clear, I will change
log level as current one.

>
> Also, what are you hoping to achieve? scsi_show_result() is only used by
> sd in a very few special command situations.  I can't believe the msg
> byte would be anything other than zero and the status byte check
> condition.
Regarding the introduction of additional information in prints.
We encountered an error with error logs like:
[ 131.673096] sd 0:0:0:0: [sda] Result: hostbyte=0x00 driverbyte=0x08
[ 131.679038] sd 0:0:0:0: [sda] Sense Key : 0xb [current]
[ 131.684801] sd 0:0:0:0: [sda] ASC=0x8 ASCQ=0x3
[ 131.689241] sd 0:0:0:0: [sda] CDB: cdb[0]=0x2a: 2a 00 00 cb 0c 00 00 00 f0 00

Looking at the logs it was clear it was due "ABORTED command" but we
wanted to check in the code if there was any retry in such case:
In ‘scsi_decide_disposition’ there are "3" main conditions

switch (host_byte(scmd->result)) -> this returned "DID_OK"
        if (msg_byte(scmd->result) != COMMAND_COMPLETE)
                return FAILED;

And the last was:
switch (status_byte(scmd->result)) {
…
    case TASK_ABORTED:
                goto maybe_retry;
    case CHECK_CONDITION:
…

So, if the status/host bytes were known - we could have directly
deduced from the code. Instead we needed to introduce prints and then
check the path.

Thanks.
>
> James
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/