Re: [PATCH 09/11] firewire-sbp-target: Add sbp_target_agent.{c,h}

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

On Apr 14 Stefan Richter wrote:
> On Apr 11 Chris Boot wrote:
> > +static int tgt_agent_rw_agent_state(struct fw_card *card, int tcode, void *data,
> > +		struct sbp_target_agent *agent)
> > +{
> > +	__be32 state;
> > +
> > +	switch (tcode) {
> > +	case TCODE_READ_QUADLET_REQUEST:
> > +		pr_debug("tgt_agent AGENT_STATE READ\n");
> > +
> > +		spin_lock_bh(&agent->lock);
> > +		state = cpu_to_be32(agent->state);
> > +		spin_unlock_bh(&agent->lock);
> > +		memcpy(data, &state, sizeof(state));
> > +
> > +		return RCODE_COMPLETE;
> > +
> > +	case TCODE_WRITE_QUADLET_REQUEST:
> > +		/* ignored */
> > +		return RCODE_COMPLETE;
> > +
> > +	default:
> > +		return RCODE_TYPE_ERROR;
> > +	}
> > +}
> 
> agent->state is an int; reading an int is atomic.  Hence the access on
> its own does not benefit from lock acquisition.

Actually this is only true because all write accesses to agent->state are
merely assignments (not increments or the like).  And I have to admit that
I don't remember whether this is only how compilers work in practice or it
is actually required by the C language specification.

> The memcpy can be simplified to
> 
> 	*(__be32 *)data = cpu_to_be32(agent->state);
> 
> if data is always at least quadlet aligned.  Thy type cast is only to tell
> code checkers like sparse that we know what we are doing.

So unless there is such an atomicity guarantee, leave the locking as is
and prefer

	int state;

	spin_lock_bh(&agent->lock);
	state = agent->state;
	spin_unlock_bh(&agent->lock);

	*(__be32 *)data = cpu_to_be32(state);

> If data may be
> unaligned, you could use
> 
> 	put_unaligned_be32(agent->state, data);

OK, I read further.  This is part of your handler.address_callback.
data will point to quadlet aligned memory then.  It is no where written as
an API specification, but you may rest assured that firewire-core will
always align quadlet read or block read response buffers at least on
quadlet boundary.  You can safely cast data into an u32 or __be32 pointer.
-- 
Stefan Richter
-=====-===-- -=-- -===-
http://arcgraph.de/sr/