Re: [PATCH] scsi: core: Report error list information in debugfs

[John Garry <john.g.garry@oracle.com>]

On 22/08/2023 16:25, Bart Van Assche wrote:
> On 8/22/23 02:04, John Garry wrote:
>> If it's on the first list, then there is not much point in checking 
>> this list.
>> It might be even worth checking list_empty(&cmd->eh_entry) initially 
>> also to
>> save the bother.
>>
>> Having said all this, adding those checks will add lots of unpleasant 
>> indentation...
> 
> Since the above code is only used to export information through debugfs 
> I don't
> think that it should be optimized heavily? 

To me it's not really a matter of having the code optimized, but more 
about having clear and efficient code.

> Anyway, how about combining the
> (untested) patch below with the above patch?
> 
> diff --git a/drivers/scsi/scsi_debugfs.c b/drivers/scsi/scsi_debugfs.c
> index a9bc5f7ce745..f795848b316c 100644
> --- a/drivers/scsi/scsi_debugfs.c
> +++ b/drivers/scsi/scsi_debugfs.c
> @@ -45,15 +45,16 @@ void scsi_show_rq(struct seq_file *m, struct request 
> *rq)
>       list_for_each_entry(cmd2, &shost->eh_abort_list, eh_entry) {
>           if (cmd == cmd2) {
>               list_info = "on eh_abort_list";
> -            break;
> +            goto unlock;
>           }
>       }
>       list_for_each_entry(cmd2, &shost->eh_cmd_q, eh_entry) {
>           if (cmd == cmd2) {
>               list_info = "on eh_cmd_q";
> -            break;
> +            goto unlock;
>           }
>       }

I think that list_info could be set to NULL here, rather than when declared.

> +unlock:
>       spin_unlock_irq(shost->host_lock);
> 
>       __scsi_format_command(buf, sizeof(buf), cmd->cmnd, cmd->cmd_len);

Regardless of comment, above:
Reviewed-by: John Garry <john.g.garry@oracle.com>

thanks