Re: [PATCH 38/41] hpsa: improve error messages for driver initiated commands

[Hannes Reinecke <hare@suse.de>]

On 01/15/2014 11:39 PM, Stephen M. Cameron wrote:
> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> 
> On encountering unexpected error conditions from driver initiated
> commands, print something useful like CDB and sense data rather than
> something useless like the kernel virtual address of the command buffer.
> 
> Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> ---
>  drivers/scsi/hpsa.c |   71 ++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 44 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 6a50a83..d469974 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -1847,17 +1847,37 @@ static void hpsa_scsi_do_simple_cmd_with_retry(struct ctlr_info *h,
>  	hpsa_pci_unmap(h->pdev, c, 1, data_direction);
>  }
>  
> -static void hpsa_scsi_interpret_error(struct CommandList *cp)
> +static void hpsa_print_cmd(struct ctlr_info *h, char *txt,
> +				struct CommandList *c)
>  {
> -	struct ErrorInfo *ei;
> +	const u8 *cdb = c->Request.CDB;
> +	const u8 *lun = c->Header.LUN.LunAddrBytes;
> +
> +	dev_warn(&h->pdev->dev, "%s: LUN:%02x%02x%02x%02x%02x%02x%02x%02x"
> +	" CDB:%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> +		txt, lun[0], lun[1], lun[2], lun[3],
> +		lun[4], lun[5], lun[6], lun[7],
> +		cdb[0], cdb[1], cdb[2], cdb[3],
> +		cdb[4], cdb[5], cdb[6], cdb[7],
> +		cdb[8], cdb[9], cdb[10], cdb[11],
> +		cdb[12], cdb[13], cdb[14], cdb[15]);
> +}
Do you _really_ have to do this?
We do have scsi_logging_level, which would provide exactly this
information.
Can't you rely on the SCSI stack for logging here and drop all
the redundancy in the driver?

Thing is, the hpsa driver is already quite chatty:

hpsa 0000:4f:00.0: Direct-Access     device c0b0t0l0 added.
hpsa 0000:4f:00.0: RAID              device c0b3t0l0 added.
scsi 0:0:0:0: Direct-Access     HP       LOGICAL VOLUME   7.24 PQ
: 0 ANSI: 5
sd 0:0:0:0: [sda] 286677120 512-byte logical blocks: (146 GB/136
GiB)
scsi 0:3:0:0: RAID              HP       P400             7.24 PQ: 0
ANSI: 0

So where is the point of the first two messages?
The enumeration is internal anyway, so it has a very quesionable
value to the user. And the devices are listed afterwards anyway.

Hence it _might_ be an idea to remove some of those messages ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html