Thanks Latha! Looks good. A few minor comments below.
On 9/13/19 9:48 AM, Brahadambal Srinivasan wrote:
> New drives that default to reporting descriptor format sense data,
> do not have support for this in iprutils and it makes it impossible
> to use the drives. This patch adds code to convert the sense data
> to the format that user expects.
>
> Signed-off-by: Brahadambal Srinivasan <la...@linux.vnet.ibm.com>
> Signed-off-by: Brian King <brk...@linux.vnet.ibm.com>
My signed off should be removed from the patch submission and I
will add it when committing the patch.
> Reviewed-by: Rick Lindsley <rickl...@linux.vnet.ibm.com>
> Reported-by: Timothy O'Callaghan <t...@us.ibm.com>
>
> ---
> iprlib.c | 31 +++++++++++++++++++++++++++++--
> iprlib.h | 11 +++++++++++
> 2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/iprlib.c b/iprlib.c
> index 516f069..032fe1a 100644
> --- a/iprlib.c
> +++ b/iprlib.c
> @@ -16,6 +16,8 @@
> #ifndef iprlib_h
> #include "iprlib.h"
> #endif
> +#include <stdio.h>
> +#include <stdlib.h>
Why are these needed? Can they be removed?
> @@ -5352,6 +5356,29 @@ static int _sg_ioctl(int fd, u8 cdb[IPR_CCB_CDB_LEN],
> break;
> }
>
> + memset(sense_data, 0, sizeof(struct sense_data_t));
> +
> + if (((sd.error_code & 0x7F) == 0x72) || ((sd.error_code & 0x7F) ==
> 0x73)) {
> + dfsdp = (struct df_sense_data_t *)&sd;
> + /* change error_codes 0x72 to 0x70 and 0x73 to 0x71 */
> + sense_data->error_code = dfsdp->error_code & 0xFD;
> +
> + /* Do not change the order of the next two assignments
> + * In the same u8, bit 4 of the fixed format corresponds
> + * to SDAT_OVLF and the last 4 bits to sense_key.
> + */
> + sense_data->sense_key = dfsdp->sense_key & 0x0F;
> + if (dfsdp->rfield & 0x80)
> + sense_data->sense_key |= 0x10;
> +
> + /* copy the other values */
> + sense_data->add_sense_code = dfsdp->add_sense_code;
> + sense_data->add_sense_code_qual = dfsdp->add_sense_code_qual;
> + sense_data->add_sense_len = dfsdp->add_sense_len;
I would suggest leaving the add_sense_len to zero here. There is no additional
sense data you are copying over and the format would be different anyway, so
you can't easily do that.
> + } else if (((sd.error_code & 0x7F) == 0x70) || ((sd.error_code & 0x7F)
> == 0x71)) {
> + memcpy((void *)sense_data, (void *)&sd, sizeof(struct
> sense_data_t));
Some trailing whitespace on the memcpy line. Also, you can probably remove the
void * casts
here. Were you getting compile warnings without them? Also, you can probably
just simplify
the check here to be something like:
else if (sd.error_code & 0x7F)
That way any vendor specific sense data doesn't get lost.
> + }
> +
> out:
> if (iovec_count) {
> for (i = 0, buf = (u8 *)data; i < iovec_count; i++) {
--
Brian King
Power Linux I/O
IBM Linux Technology Center
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
