lkml.org 
[lkml]   [2019]   [Jan]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: Regression: spi: core: avoid waking pump thread from spi_sync instead run teardown delayed
From
Date
Hi Mark!

Just got access to my computer back for the weekend ;)

> On 15.01.2019, at 22:25, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jan 15, 2019 at 09:58:55PM +0100, Martin Sperl wrote:
>
>> I may find some time over the weekend if no solution
>> has been found until then.
>
> Thanks for volunteering :)

I actually brought this about so I see is as part of my
responsibility trying to solve the issue as well...
>
>> The way I would envision it it would have a “state”
>> as a level (0=shutdown, 1=hw enabled, 2=in pump,
>> 3=in transfer, 4=in hw-mode,...) and a complete
>> to allow waking the shutdown thread (and by this
>> avoiding the busy wait loop we have now).
>> This would replace those idling, busy, and running flags.
>
> That's a good idea, yes - a single enum much more reflects what we can
> actually do in terms of transitions.

Does something like this looks acceptable?

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index ec210286567c..677fc5025033 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -288,6 +288,21 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
module_driver(__spi_driver, spi_register_driver, \
spi_unregister_driver)

+/* define SPI Controller states in the state machine */
+enum spi_controller_state {
+ SPI_CONTROLLER_STATE_SHUTDOWN = 0,
+ SPI_CONTROLLER_STATE_IDLE = 1,
+ SPI_CONTROLLER_STATE_IN_PROCESS = 2,
+ SPI_CONTROLLER_STATE_IN_TRANSFER = 3,
+};
+
+/* define SPI Controller mode */
+enum spi_controller_mode {
+ SPI_CONTROLLER_MODE_NORMAL = 0, /* normal spi mode */
+ SPI_CONTROLLER_MODE_MEM = 1, /* memory mode using mem_ops */
+ SPI_CONTROLLER_MODE_EXCLUSIVE = 2, /* could replace bus_lock_flag */
+};
+
/**
* struct spi_controller - interface to SPI master or slave controller
* @dev: device interface to this driver
@@ -332,14 +347,16 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
* @pump_idle_teardown: work structure for scheduling a teardown delayed
* @queue_lock: spinlock to syncronise access to message queue
* @queue: message queue
- * @idling: the device is entering idle state
* @cur_msg: the currently in-flight message
* @cur_msg_prepared: spi_prepare_message was called for the currently
* in-flight message
* @cur_msg_mapped: message has been mapped for DMA
+ * @controller_state: defines the current state of the controller
+ * state machine
+ * @controller_state_changed: wakeup of threads interrested in getting
+ * notified about a mode change (primarily pm)
+ * @controller_mode: defines the current mode of the controller
* @xfer_completion: used by core transfer_one_message()
- * @busy: message pump is busy
- * @running: message pump is running
* @rt: whether this queue is set to run as a realtime task
* @auto_runtime_pm: the core should ensure a runtime PM reference is held
* while the hardware is prepared, using the parent
@@ -524,9 +541,9 @@ struct spi_controller {
spinlock_t queue_lock;
struct list_head queue;
struct spi_message *cur_msg;
- bool idling;
- bool busy;
- bool running;
+ enum spi_controller_state controller_state;
+ struct completion controller_state_changed;
+ enum spi_controller_mode controller_mode;
bool rt;
bool auto_runtime_pm;
bool cur_msg_prepared;

SPI_CONTROLLER_MODE_EXCLUSIVE could replace the bus_lock_flag.
I am also not sure of the “convention” of memory mode (i.e
using mem_ops). Is it maybe implicitly spi_bus_locked - i.e
typically only a single device?

Essentially we would transition between these states linearly
(hopefully no jumps are needed).

Martin

\
 
 \ /
  Last update: 2019-01-18 18:11    [W:0.071 / U:3.656 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site