Re: [RFC PATCH 4/6] isci: hardware / topology event handling

[Christoph Hellwig <hch@infradead.org>]

On Fri, Mar 25, 2011 at 03:34:39PM -0700, Dan Williams wrote:
> Ok, so that's why I went all the way to recommending a switch to
> delayed_workqueue.  We can bring down the core and ensure all events
> have seen a cancellation.  None of these containing objects are
> dynamically destroyed outside of driver exit, nor would they care
> about the switch from timer callback to process context.  That final
> flush is the only elusive bit that is covered by flush_workqueue.

This seems like a really nasy hack to me.  Right now there are 9
callers of isci_timer_create, of those

  5 operate on struct scic_sds_controller
  2 operate on struct scic_sds_phy 
  1 operates on struct scic_sds_port
  1 operates on struct isci_request

of these at least the request has completely different life time rules
than the scic_sds_controller, so both the existing code and the
flush_workqueue variant would be incorrect.

I'd suggest that as a first step you remove the dynamic allocation of
the timers with a structure embedded into the containing structure
and replace isci_timer_list_destroy with a calls to
del_timer_sync when the containing structure is freed.

After that you can trivial remove the wrappers and just opencode the
Linux timer calls, which should lead to much simpler and easier to
understand code.

If the process context of delayed work items provices a significant
benefit to your execution model you can convert the timers to delayed
work items now, and remove the irqsafe locking.  Given that the
isci code still does some non-trivial work from it's interrupt
handlers and tasklets I'm not sure it's going to buy you much, though.

It might be worth to try replacing the tasklets with threaded interrupts
to simplify the locking model, but it will need carefully benchmarking
to evaluate the performance impact.