Re: [PATCH v2 09/11] firewire-sbp-target: Add sbp_target_agent.{c,h}

[Chris Boot]

On 18 Feb 2012, at 14:59, Stefan Richter <stefanr@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/