lkml.org 
[lkml]   [2012]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] firewire-sbp2: Initialise sbp2_orb->rcode for management ORBs
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/


\
 
 \ /
  Last update: 2012-03-04 13:51    [W:0.059 / U:0.428 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site