lkml.org 
[lkml]   [2012]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 09/11] firewire-sbp-target: Add sbp_target_agent.{c,h}
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/


\
 
 \ /
  Last update: 2012-04-14 13:35    [W:0.049 / U:0.188 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site