lkml.org 
[lkml]   [2012]   [Feb]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 09/11] firewire-sbp-target: Add sbp_target_agent.{c,h}
On Feb 16 Chris Boot wrote:
> On 15/02/2012 21:27, Stefan Richter wrote:
> > On Feb 15 Chris Boot wrote:
> >> --- /dev/null
> >> +++ b/drivers/target/sbp/sbp_target_agent.c
> > [...]
> >> +static int tgt_agent_rw_orb_pointer(struct fw_card *card,
> >> + int tcode, int generation, void *data,
> >> + struct sbp_target_agent *agent)
> >> +{
> >> + struct sbp2_pointer *ptr = data;
> >> + int ret;
> >> +
> >> + switch (tcode) {
> >> + case TCODE_WRITE_BLOCK_REQUEST:
> >> + smp_wmb();
> >> + atomic_cmpxchg(&agent->state,
> >> + AGENT_STATE_RESET, AGENT_STATE_SUSPENDED);
> >> + smp_wmb();
> >> + if (atomic_cmpxchg(&agent->state,
> >> + AGENT_STATE_SUSPENDED,
> >> + AGENT_STATE_ACTIVE)
> >> + != AGENT_STATE_SUSPENDED)
> >> + return RCODE_CONFLICT_ERROR;
> >> + smp_wmb();
> >
> > Why the double state change?
>
> Because the SBP spec differentiates between the RESET state, which
> happens after the agent initialises or is sent an explicit reset
> request, and when it's suspended between requests...

OK, right, there are the state transitions Reset-->Active and
Suspended-->Active. Though you implement the former as a swift
Reset-->Suspended-->Active. Which does indeed work, provided that there
is no other concurrent context which could transition from Suspended to
Anything-but-Active.

> > And as asked at the patch, which writes are the barriers meant to order,
> > and how does the corresponding read side look like? Or are these barriers
> > not actually needed after all?
>
> ...of course this is another time when my use of atomics and memory
> barriers is entirely wrong. I should most likely be using locking here.
>
> > [...]
> >> +void sbp_target_agent_unregister(struct sbp_target_agent *agent)
> >> +{
> >> + if (atomic_read(&agent->state) == AGENT_STATE_ACTIVE)
> >> + flush_work_sync(&agent->work);
> >> +
> >> + fw_core_remove_address_handler(&agent->handler);
> >> + kfree(agent);
> >> +}
> >
> > So, asking once more without having read the code in full yet: Are you
> > sure that agent->state is not going to change anymore after you tested it
> > here?
>
> Nope. At least in this case I can unregister the address handler before
> I check if I need to flush the work item.

Yep, first unregister the handler, then wait for the work to finish, then
free the data.

And as discussed off-list today, firewire-core should be improved to
guarantee you that the handler isn't still running anywhere when
fw_core_remove_address_handler() returns.
--
Stefan Richter
-=====-===-- --=- =--=-
http://arcgraph.de/sr/


\
 
 \ /
  Last update: 2012-02-18 16:03    [W:0.110 / U:1.020 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site