[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux1394-devel
Subject:    Re: [PATCH v2 09/11] firewire-sbp-target: Add sbp_target_agent.{c,h}
From:       Chris Boot <bootc () bootc ! net>
Date:       2012-02-18 15:05:54
Message-ID: 1BDF59F2-4EFE-4E6B-9958-FD056CA3120B () bootc ! net
[Download RAW message or body]

On 18 Feb 2012, at 14:59, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> 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.

I'll rewrite this bit to use a lock to protect the state struct member so it isn't so \
strange. That would also avoid the unusual double state change.

> > > 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.

Great, thanks for clearing that up.

Cheers,
Chris

-- 
Chris Boot
bootc@bootc.net


------------------------------------------------------------------------------
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing 
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
_______________________________________________
mailing list linux1394-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux1394-devel


[prev in list] [next in list] [prev in thread] [next in thread]