Re: [PATCH v2 07/11] firewire-sbp-target: add sbp_management_agent.{c,h}

[Chris Boot <bootc@bootc.net>]

On 15/02/2012 19:48, Stefan Richter wrote:
> On Feb 15 Chris Boot wrote:
>> --- /dev/null
>> +++ b/drivers/target/sbp/sbp_management_agent.c
> [...]
>> +static void sbp_mgt_agent_rw(struct fw_card *card,
>> +	struct fw_request *request, int tcode, int destination, int source,
>> +	int generation, unsigned long long offset, void *data, size_t length,
>> +	void *callback_data)
>> +{
>> +	struct sbp_management_agent *agent = callback_data;
>> +	struct sbp2_pointer *ptr = data;
>> +
>> +	if (!agent->tport->enable) {
>> +		fw_send_response(card, request, RCODE_ADDRESS_ERROR);
>> +		return;
>> +	}
>> +
>> +	if ((offset != agent->handler.offset) || (length != 8)) {
>> +		fw_send_response(card, request, RCODE_ADDRESS_ERROR);
>> +		return;
>> +	}
>> +
>> +	if (tcode == TCODE_WRITE_BLOCK_REQUEST) {
>> +		struct sbp_management_request *req;
>> +		int ret;
>> +
>> +		smp_wmb();
>> +		if (atomic_cmpxchg(&agent->state,
>> +					MANAGEMENT_AGENT_STATE_IDLE,
>> +					MANAGEMENT_AGENT_STATE_BUSY) !=
>> +				MANAGEMENT_AGENT_STATE_IDLE) {
>> +			pr_notice("ignoring management request while busy\n");
>> +
>> +			fw_send_response(card, request, RCODE_CONFLICT_ERROR);
>> +			return;
>> +		}
>
> There is a rule of thumb which says:  If you add a memory barrier anywhere
> in your code, also add a comment saying which accesses this barrier is
> meant to bring into order.
>
> So after the write barrier is apparently the agent->state access.  What
> access is before the barrier?
>
> And how does the read side look like?
>
> These questions are mostly rhetoric.  It is quite likely that this code is
> better off with a plain and simple mutex serialization.

Well as I mentioned in my previous messages atomics and locking are 
pretty new ideas to me. Having only really done threading in PERL 
before, the world of doing it in the kernel is rather different! :-) The 
memory barriers are there after I looked in Documentation/atomic_ops.txt 
and clearly misunderstood the entire thing.

Reading about mutexes though I see I can't use them from interrupt 
context, but doesn't the FireWire address handler execute in interrupt 
context? I have to check the state of the management agent in the 
address handler to properly reject requests from the initiator when the 
agent is busy. I guess a spinlock is called for in this situation, 
possibly using spin_trylock() in the address handler to avoid blocking?

> [...]
>> +void sbp_management_agent_unregister(struct sbp_management_agent *agent)
>> +{
>> +	if (atomic_read(&agent->state) != MANAGEMENT_AGENT_STATE_IDLE)
>> +		flush_work_sync(&agent->work);
>> +
>> +	fw_core_remove_address_handler(&agent->handler);
>> +	kfree(agent);
>> +}
>
> I still have yet to test-apply all your patches, look at the sum of the
> code and understand what the execution contexts and critical sections
> are.  So I really should not yet ask the next, uninformed question.
>
> Looking at this function, I wonder:  Can the agent->state change after you
> read it, and what would happen then?

The agent state could change between the read and before the address 
handler is unregistered, this would require an initiator to keep sending 
management requests long after the FW unit descriptor is removed though.

I guess to solve this in a bulletproof manner I need a kref in struct 
sbp_management_agent and a spinlock(?), and a new state to say the agent 
is going away. That way if I happen to receive requests while the agent 
is being removed it can reply with address errors before the address 
handler is fully removed. I'd also need something similar in the target 
agent.

-- 
Chris Boot
bootc@bootc.net