From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755359Ab2DNLdn (ORCPT ); Sat, 14 Apr 2012 07:33:43 -0400 Received: from einhorn.in-berlin.de ([192.109.42.8]:32996 "EHLO einhorn.in-berlin.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754835Ab2DNLdm (ORCPT ); Sat, 14 Apr 2012 07:33:42 -0400 X-Envelope-From: stefanr@s5r6.in-berlin.de Date: Sat, 14 Apr 2012 13:33:22 +0200 From: Stefan Richter To: Chris Boot Cc: linux1394-devel@lists.sourceforge.net, target-devel@vger.kernel.org, linux-kernel@vger.kernel.org, agrover@redhat.com, clemens@ladisch.de, nab@linux-iscsi.org Subject: Re: [PATCH 09/11] firewire-sbp-target: Add sbp_target_agent.{c,h} Message-ID: <20120414133322.6f601eb9@stein> In-Reply-To: <20120414124909.0615aa2a@stein> References: <1329317248-94128-1-git-send-email-bootc@bootc.net> <1334154043-69076-1-git-send-email-bootc@bootc.net> <1334154043-69076-10-git-send-email-bootc@bootc.net> <20120414124909.0615aa2a@stein> X-Mailer: Claws Mail 3.8.0 (GTK+ 2.24.8; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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/