Martin,
On 3/1/17 12:21, Martin K. Petersen wrote:
>>>>>> "Damien" == Damien Le Moal <damien.lemoal@xxxxxxx> writes:
>
> Damien,
>
> Damien> The problem remains that the mpt3sas driver needs fixing. As you
> Damien> suggest, we can do that in sd, or directly in mpt3sas. I tried
> Damien> to do a clean fix in sd, but always end up consuming a lot of
> Damien> aspirin because of all the potential corner cases to deal
> Damien> with.
>
> What? You expected this to be easy? :)
>
> FWIW, I'm perfectly happy with the desire to shuffle the ZBC-specifics
> over to sd_zbc.c.
>
> Damien> The file sd.h has the inline function
> Damien> scsi_medium_access_command() defined. We could move that to
> Damien> include/scsi/scsi.h (or scsi_proto.h) and use it in place of
> Damien> blk_rq_accesses_medium() in the mpt3sas driver to not force
> Damien> unaligned resid corrections for zone commands. Would that be
> Damien> acceptable ?
>
> I'd still rather keep it in sd. If you don't have sufficient supplies of
> aspirin for the sd_completed_bytes() approach then I'm OK with a simple
> tweak to sd_done(). We need something we can reasonably Cc: to stable
> for 4.10.
>
> You are welcome to key off of scsi_medium_access_command() if you like
> but I don't think it's necessary. Just having a default for the req_op
> switch should suffice.
>
> I believe that was your original sd approach. If you post it again I'll
> have another look.
Just posted that. Please review.
>
> And then I'll put the bigger completion rework back on my todo list.
Still working on that, with the hope of being able to cleanup sd_done()
more nicely and hopefully integrate the resid fix in sd_completed_bytes().
Regarding this, I have a question:
In sd_done(), the sshdr.sense_key switch-case is entered after passing
this "if" statement:
if (driver_byte(result) != DRIVER_SENSE &&
(!sense_valid || sense_deferred))
goto out;
That is, the sshdr.sense_key switch-case is entered if
driver_byte(result) == DRIVER_SENSE ||
(sense_valid && !sense_deferred))
is true.
My question is: Why the "driver_byte(result) == DRIVER_SENSE" ?
wouldn't just checking for (sense_valid && !sense_deferred)) enough ?
After all, sshdr is initialized with scsi_command_normalize_sense() for
any non-0 value of result.
So we may be uselessly looking at sshdr with sense_valid being false for
those cases where driver_byte(result) == DRIVER_SENSE. But I am not sure
if such case is actually possible (bogus HW ?). If is is not, we should
check that more carefully, shouldn't we ?
Best regards.
--
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@xxxxxxx
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com
