From 8b8d719161c386668161daf29b2632f094e326c8 Mon Sep 17 00:00:00 2001 From: Virupax Sadashivpetimath Date: Thu, 10 Nov 2011 12:43:24 +0530 Subject: spi/pl022: make the chip deselect handling thread safe There is a possibility that the pump_message and giveback run in parallel on a SMP system. Both the pump_message and giveback threads work on the same SPI message queue. Results will be in correct if the pump_message gets to work on the queue first. when the pump_message works with the queue, it reads the head of the queue and removes it from the queue. pump_message activates the chip select depending on this message read. This leads to giveback working on the modified queue or a emptied queue. If the queue is empty or if the next message on the queue (which is not the actual next message, as the pump message has removed the next message from the queue) is not for the same SPI device as that Of the previous one, giveback will de-activate the chip select activated by pump_message(), which is wrong. To solve this problem pump_message is made not to run and access the queue until the giveback is done handling the queue. I.e. by making the cur_msg NULL after the giveback has read the queue. Also a state variable has been introduced to keep track of when the CS for next message is already activated and avoid to double-activate it. Reviewed-by: Viresh Kumar Signed-off-by: Virupax Sadashivpetimath Signed-off-by: Linus Walleij --- drivers/spi/spi-pl022.c | 72 +++++++++++++++++++++++++------------------------ 1 file changed, 37 insertions(+), 35 deletions(-) (limited to 'drivers/spi') diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index 13988a3024b..1cff8ad470d 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -340,6 +340,10 @@ struct vendor_data { * @cur_msg: Pointer to current spi_message being processed * @cur_transfer: Pointer to current spi_transfer * @cur_chip: pointer to current clients chip(assigned from controller_state) + * @next_msg_cs_active: the next message in the queue has been examined + * and it was found that it uses the same chip select as the previous + * message, so we left it active after the previous transfer, and it's + * active already. * @tx: current position in TX buffer to be read * @tx_end: end position in TX buffer to be read * @rx: current position in RX buffer to be written @@ -373,6 +377,7 @@ struct pl022 { struct spi_message *cur_msg; struct spi_transfer *cur_transfer; struct chip_data *cur_chip; + bool next_msg_cs_active; void *tx; void *tx_end; void *rx; @@ -445,23 +450,9 @@ static void giveback(struct pl022 *pl022) struct spi_transfer *last_transfer; unsigned long flags; struct spi_message *msg; - void (*curr_cs_control) (u32 command); + pl022->next_msg_cs_active = false; - /* - * This local reference to the chip select function - * is needed because we set curr_chip to NULL - * as a step toward termininating the message. - */ - curr_cs_control = pl022->cur_chip->cs_control; - spin_lock_irqsave(&pl022->queue_lock, flags); - msg = pl022->cur_msg; - pl022->cur_msg = NULL; - pl022->cur_transfer = NULL; - pl022->cur_chip = NULL; - queue_work(pl022->workqueue, &pl022->pump_messages); - spin_unlock_irqrestore(&pl022->queue_lock, flags); - - last_transfer = list_entry(msg->transfers.prev, + last_transfer = list_entry(pl022->cur_msg->transfers.prev, struct spi_transfer, transfer_list); @@ -473,18 +464,13 @@ static void giveback(struct pl022 *pl022) */ udelay(last_transfer->delay_usecs); - /* - * Drop chip select UNLESS cs_change is true or we are returning - * a message with an error, or next message is for another chip - */ - if (!last_transfer->cs_change) - curr_cs_control(SSP_CHIP_DESELECT); - else { + if (!last_transfer->cs_change) { struct spi_message *next_msg; - /* Holding of cs was hinted, but we need to make sure - * the next message is for the same chip. Don't waste - * time with the following tests unless this was hinted. + /* + * cs_change was not set. We can keep the chip select + * enabled if there is message in the queue and it is + * for the same spi device. * * We cannot postpone this until pump_messages, because * after calling msg->complete (below) the driver that @@ -501,14 +487,26 @@ static void giveback(struct pl022 *pl022) struct spi_message, queue); spin_unlock_irqrestore(&pl022->queue_lock, flags); - /* see if the next and current messages point - * to the same chip + /* + * see if the next and current messages point + * to the same spi device. */ - if (next_msg && next_msg->spi != msg->spi) + if (next_msg && next_msg->spi != pl022->cur_msg->spi) next_msg = NULL; - if (!next_msg || msg->state == STATE_ERROR) - curr_cs_control(SSP_CHIP_DESELECT); + if (!next_msg || pl022->cur_msg->state == STATE_ERROR) + pl022->cur_chip->cs_control(SSP_CHIP_DESELECT); + else + pl022->next_msg_cs_active = true; } + + spin_lock_irqsave(&pl022->queue_lock, flags); + msg = pl022->cur_msg; + pl022->cur_msg = NULL; + pl022->cur_transfer = NULL; + pl022->cur_chip = NULL; + queue_work(pl022->workqueue, &pl022->pump_messages); + spin_unlock_irqrestore(&pl022->queue_lock, flags); + msg->state = NULL; if (msg->complete) msg->complete(msg->context); @@ -1350,7 +1348,7 @@ static void pump_transfers(unsigned long data) */ udelay(previous->delay_usecs); - /* Drop chip select only if cs_change is requested */ + /* Reselect chip select only if cs_change was requested */ if (previous->cs_change) pl022->cur_chip->cs_control(SSP_CHIP_SELECT); } else { @@ -1389,8 +1387,10 @@ static void do_interrupt_dma_transfer(struct pl022 *pl022) */ u32 irqflags = ENABLE_ALL_INTERRUPTS & ~SSP_IMSC_MASK_RXIM; - /* Enable target chip */ - pl022->cur_chip->cs_control(SSP_CHIP_SELECT); + /* Enable target chip, if not already active */ + if (!pl022->next_msg_cs_active) + pl022->cur_chip->cs_control(SSP_CHIP_SELECT); + if (set_up_next_transfer(pl022, pl022->cur_transfer)) { /* Error path */ pl022->cur_msg->state = STATE_ERROR; @@ -1445,7 +1445,8 @@ static void do_polling_transfer(struct pl022 *pl022) } else { /* STATE_START */ message->state = STATE_RUNNING; - pl022->cur_chip->cs_control(SSP_CHIP_SELECT); + if (!pl022->next_msg_cs_active) + pl022->cur_chip->cs_control(SSP_CHIP_SELECT); } /* Configuration Changing Per Transfer */ @@ -1604,6 +1605,7 @@ static int start_queue(struct pl022 *pl022) pl022->cur_msg = NULL; pl022->cur_transfer = NULL; pl022->cur_chip = NULL; + pl022->next_msg_cs_active = false; spin_unlock_irqrestore(&pl022->queue_lock, flags); queue_work(pl022->workqueue, &pl022->pump_messages); -- cgit v1.2.3-70-g09d2