Hi Werner,
thanks again for your patch, please find my comments below in the text.
2011/6/18 Werner Almesberger <[email protected]>
> The is_tx flag did not protect adequately against races when switching
> from receiving to transmitting. This patch removes the unnecessary
> spinlock and enforces proper synchronization along the interrupt
> processing chain (source, handler, and worker), before entering
> transmit mode.
>
> The mechanism for allowing transmits to be interrupted had two flaws:
> 1) it tried to return with the transmission potentially never
> completed, and
> 2) it attempted to leave BUSY_TX with STATE_RX_ON, which according to
> the data sheet is not valid.
>
> This patch force the transceiver into PLL_ON/TX_ON state on
> interruption and then has a common path for transiting to RX_ON and
> clearing is_tx.
>
> Note that the synchronization overhead introduced by this patch
> is about 100 us on a Ben NanoNote.
>
> Signed-off-by: Werner Almesberger <[email protected]>
>
> ---
> drivers/ieee802154/at86rf230.c | 40
> +++++++++++++++-------------------------
> 1 files changed, 15 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/ieee802154/at86rf230.c
> b/drivers/ieee802154/at86rf230.c
> index 184c442..d4dbd5b 100644
> --- a/drivers/ieee802154/at86rf230.c
> +++ b/drivers/ieee802154/at86rf230.c
> @@ -56,8 +56,7 @@ struct at86rf230_local {
>
> struct ieee802154_dev *dev;
>
> - spinlock_t lock;
> - unsigned is_tx:1; /* P: lock */
> + volatile unsigned is_tx:1; /* P: lock */
> };
>
>
> @@ -362,23 +361,24 @@ static int
> at86rf230_xmit(struct ieee802154_dev *dev, struct sk_buff *skb)
> {
> struct at86rf230_local *lp = dev->priv;
> - int rc;
> - unsigned long flags;
> + int rc, rc2;
>
> pr_debug("%s\n", __func__);
>
> might_sleep();
>
> - spin_lock_irqsave(&lp->lock, flags);
> BUG_ON(lp->is_tx);
> - lp->is_tx = 1;
> INIT_COMPLETION(lp->tx_complete);
> - spin_unlock_irqrestore(&lp->lock, flags);
>
> rc = at86rf230_state(dev, STATE_FORCE_TX_ON);
> if (rc)
> goto err;
>
> + synchronize_irq(lp->spi->irq);
> + flush_work(&lp->irqwork);
>
Could you please explain why we need these flush and sync?
> +
> + lp->is_tx = 1;
> +
rc = at86rf230_write_fbuf(lp, skb->data, skb->len);
> if (rc)
> goto err_rx;
> @@ -395,19 +395,16 @@ at86rf230_xmit(struct ieee802154_dev *dev, struct
> sk_buff *skb)
>
> rc = wait_for_completion_interruptible(&lp->tx_complete);
> if (rc < 0)
> - goto err_rx;
> -
> - rc = at86rf230_start(dev);
> -
> - return rc;
> + at86rf230_state(dev, STATE_FORCE_TX_ON);
>
> err_rx:
> - at86rf230_start(dev);
> + rc2 = at86rf230_start(dev);
>
As I understand, now we are unprotected from RX interrupts, but lp->is_tx is
still '1'. What will happen?
> + if (!rc)
> + rc = rc2;
> err:
> - pr_err("%s error: %d\n", __func__, rc);
> - spin_lock_irqsave(&lp->lock, flags);
> + if (rc)
> + pr_err("%s error: %d\n", __func__, rc);
>
For me such an error handling isn't too clear, we received an error status,
then we do something, then print message...
May be it will be better to keep current logic: if something fails - goto
error_label and finish function instead of continue execution.
> lp->is_tx = 0;
> - spin_unlock_irqrestore(&lp->lock, flags);
>
> return rc;
> }
> @@ -461,7 +458,6 @@ static void at86rf230_irqwork(struct work_struct *work)
> container_of(work, struct at86rf230_local, irqwork);
> u8 status = 0, val;
> int rc;
> - unsigned long flags;
>
> dev_dbg(&lp->spi->dev, "IRQ Worker\n");
>
> @@ -477,15 +473,10 @@ static void at86rf230_irqwork(struct work_struct
> *work)
>
> if (status & IRQ_TRX_END) {
> status &= ~IRQ_TRX_END;
> - spin_lock_irqsave(&lp->lock, flags);
> - if (lp->is_tx) {
> - lp->is_tx = 0;
> - spin_unlock_irqrestore(&lp->lock, flags);
> + if (lp->is_tx)
> complete(&lp->tx_complete);
> - } else {
> - spin_unlock_irqrestore(&lp->lock, flags);
> + else
> at86rf230_rx(lp);
> - }
> }
>
> } while (status != 0);
> @@ -667,7 +658,6 @@ static int __devinit at86rf230_probe(struct spi_device
> *spi)
>
> mutex_init(&lp->bmux);
> INIT_WORK(&lp->irqwork, at86rf230_irqwork);
> - spin_lock_init(&lp->lock);
> init_completion(&lp->tx_complete);
>
> spi_set_drvdata(spi, lp);
> --
> 1.7.0.4
>
>
>
> ------------------------------------------------------------------------------
> EditLive Enterprise is the world's most technically advanced content
> authoring tool. Experience the power of Track Changes, Inline Image
> Editing and ensure content is compliant with Accessibility Checking.
> http://p.sf.net/sfu/ephox-dev2dev
> _______________________________________________
> Linux-zigbee-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
>
------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev
_______________________________________________
Linux-zigbee-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
