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/
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
