lkml.org 
[lkml]   [2012]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 07/11] firewire-sbp-target: add sbp_management_agent.{c,h}
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


\
 
 \ /
  Last update: 2012-02-16 11:31    [W:0.338 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site