Re: [PATCH] firewire-sbp2: Initialise sbp2_orb->rcode for management ORBs

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

On Feb 18 Chris Boot wrote:
> When sending ORBs the struct sbp2_orb->rcode field should be initialised
> to -1 otherwise complete_transaction() assumes the request is successful
> (RCODE_COMPLETE is 0). When sending managament ORBs, such as LOGIN or
> LOGOUT, this was not done and so the initiator would wait for the
> request to time out before trying again.
> 
> Without this, LOGINs are only retried when the management ORB times out,
> rather than the initiator noticing an error occurred and retrying soon
> after. For targets that advertise more than one LUN per unit, and can
> only accept one management request at a time, this means LUNs are only
> logged in one per timeout period.
> 
> Signed-off-by: Chris Boot <bootc@bootc.net>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
>  drivers/firewire/sbp2.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
> index b12c6ba..7776c18 100644
> --- a/drivers/firewire/sbp2.c
> +++ b/drivers/firewire/sbp2.c
> @@ -572,6 +572,9 @@ static int sbp2_send_management_orb(struct sbp2_logical_unit *lu, int node_id,
>  	if (dma_mapping_error(device->card->device, orb->response_bus))
>  		goto fail_mapping_response;
>  
> +	/* Initialize rcode to something not RCODE_COMPLETE. */
> +	orb->base.rcode = -1;
> +
>  	orb->request.response.high = 0;
>  	orb->request.response.low  = cpu_to_be32(orb->response_bus);
>  

I left this hanging in my inbox for too long, sorry...

While I agree that the current initialization of orb->base.rcode with 0 is
wrong, I don't think your change alone is sufficient:

Consider the case that a login request to LU 0 causes the target to pull
out the hardware behind that LU out of a powered-down state --- which may
take a very long time --- and login requests to LU 1 would be aborted by
the target with resp_conflict_error on any Management_Agent write
request.  Of course a reasonably clever target would accept login before
full power-up, but you never now.

We retry login 5 times in 0.2 seconds intervals, and this 1 s in total may
not be enough.

--------
And a few notes to myself, to be done on top of the above:

a) Turn the magic value -1 into a defined constant.  Use that constant
in the two ORB initializers and in the SBP-2 status write handler.

b) The reconnect failure handling seems a bit simplistic.  I changed it
from Kristian's original retry loop to a shortcut to re-login in commit
ce896d95cc7886ae05859c5b409a7b2f3b606ec1.  While re-login instead of
reconnect during management-agent-busy situations works too, retrying
the reconnect at least during reconnect-hold period may be more robust in
such situations.

So the generation check should be replaced by checks for 1394 transaction
completion with RCODE_CONFLICT_ERROR or RCODE_BUSY, and maybe for some
other types of 1394 transaction failure or SBP-2 transaction failure.

c) Perhaps retry logout in sbp2_remove() if we clearly detect a
management-agent-busy situation.

d) Reduce log spam of the sort of "management write failed" or "failed to
reconnect" in situations where we know that we deal with such failures
correctly.
-- 
Stefan Richter
-=====-===-- --== --=--
http://arcgraph.de/sr/