[PATCH bluetooth-next 1/4] mac802154: iface: fix hrtimer cancel on ifdown

[PATCH bluetooth-next 1/4] mac802154: iface: fix hrtimer cancel on ifdown

[Alexander Aring]

The interframe spacing timer is a per phy definition and is part of a
ieee802154_local structure. If we have possible multiple interfaces
ifdown one interface then the timer should not be cancled. First if the
last interface is down and the receive handling is stopped we should be
sure that the interframe spacing timer isn't run anymore.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 net/mac802154/iface.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index e3d77b0..d804337 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -302,15 +302,15 @@ static int mac802154_slave_close(struct net_device *dev)
 
 	ASSERT_RTNL();
 
-	hrtimer_cancel(&local->ifs_timer);
-
 	netif_stop_queue(dev);
 	local->open_count--;
 
 	clear_bit(SDATA_STATE_RUNNING, &sdata->state);
 
-	if (!local->open_count)
+	if (!local->open_count) {
+		hrtimer_cancel(&local->ifs_timer);
 		drv_stop(local);
+	}
 
 	return 0;
 }
-- 
2.4.1

[PATCH bluetooth-next 2/4] mac802154: iface: flush workqueue before stop

[Alexander Aring]

This patch flushs the workqueue which is currently used for xmit_sync
callback before calling stop driver-ops. Flush the queue will ensure all
pending tx frames are transmitted.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 net/mac802154/iface.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index d804337..692731d 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -308,6 +308,7 @@ static int mac802154_slave_close(struct net_device *dev)
 	clear_bit(SDATA_STATE_RUNNING, &sdata->state);
 
 	if (!local->open_count) {
+		flush_workqueue(local->workqueue);
 		hrtimer_cancel(&local->ifs_timer);
 		drv_stop(local);
 	}
-- 
2.4.1

Re: [PATCH bluetooth-next 2/4] mac802154: iface: flush workqueue before stop

[Marcel Holtmann]

Hi Alex,

> This patch flushs the workqueue which is currently used for xmit_sync
> callback before calling stop driver-ops. Flush the queue will ensure all
> pending tx frames are transmitted.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
> net/mac802154/iface.c | 1 +
> 1 file changed, 1 insertion(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[PATCH bluetooth-next 3/4] at86rf230: use level high as fallback default

[Alexander Aring]

This patch use high level interrupt type as fallback handling when no
irq type is given.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 1bf86d2..25e1ede 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -1633,7 +1633,7 @@ static int at86rf230_probe(struct spi_device *spi)
 
 	irq_type = irq_get_trigger_type(spi->irq);
 	if (!irq_type)
-		irq_type = IRQF_TRIGGER_RISING;
+		irq_type = IRQF_TRIGGER_HIGH;
 
 	rc = devm_request_irq(&spi->dev, spi->irq, at86rf230_isr,
 			      IRQF_SHARED | irq_type, dev_name(&spi->dev), lp);
-- 
2.4.1

Re: [PATCH bluetooth-next 3/4] at86rf230: use level high as fallback default

[Marcel Holtmann]

Hi Alex,

> This patch use high level interrupt type as fallback handling when no
> irq type is given.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
> drivers/net/ieee802154/at86rf230.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[PATCH bluetooth-next 4/4] at86rf230: add support for sleep state

[Alexander Aring]

This patch adds support for sleep state when between stop and start
period. In this period the transceiver isn't used by the subsystem, in
this time we disable the irq and going into the sleep state.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 25e1ede..6b31f47 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -47,6 +47,8 @@ struct at86rf2xx_chip_data {
 	u16 t_reset_to_off;
 	u16 t_off_to_aack;
 	u16 t_off_to_tx_on;
+	u16 t_off_to_sleep;
+	u16 t_sleep_to_off;
 	u16 t_frame;
 	u16 t_p_ack;
 	int rssi_base_val;
@@ -869,13 +871,33 @@ at86rf230_ed(struct ieee802154_hw *hw, u8 *level)
 static int
 at86rf230_start(struct ieee802154_hw *hw)
 {
+	struct at86rf230_local *lp = hw->priv;
+
+	if (gpio_is_valid(lp->slp_tr)) {
+		gpio_set_value(lp->slp_tr, 0);
+		usleep_range(lp->data->t_sleep_to_off,
+			     lp->data->t_sleep_to_off + 100);
+	}
+
+	enable_irq(lp->spi->irq);
+
 	return at86rf230_sync_state_change(hw->priv, STATE_RX_AACK_ON);
 }
 
 static void
 at86rf230_stop(struct ieee802154_hw *hw)
 {
+	struct at86rf230_local *lp = hw->priv;
+
 	at86rf230_sync_state_change(hw->priv, STATE_FORCE_TRX_OFF);
+
+	disable_irq(lp->spi->irq);
+
+	if (gpio_is_valid(lp->slp_tr)) {
+		gpio_set_value(lp->slp_tr, 1);
+		usleep_range(lp->data->t_off_to_sleep,
+			     lp->data->t_off_to_sleep + 10);
+	}
 }
 
 static int
@@ -1241,6 +1263,8 @@ static struct at86rf2xx_chip_data at86rf233_data = {
 	.t_reset_to_off = 26,
 	.t_off_to_aack = 80,
 	.t_off_to_tx_on = 80,
+	.t_off_to_sleep = 35,
+	.t_sleep_to_off = 210,
 	.t_frame = 4096,
 	.t_p_ack = 545,
 	.rssi_base_val = -91,
@@ -1254,6 +1278,8 @@ static struct at86rf2xx_chip_data at86rf231_data = {
 	.t_reset_to_off = 37,
 	.t_off_to_aack = 110,
 	.t_off_to_tx_on = 110,
+	.t_off_to_sleep = 35,
+	.t_sleep_to_off = 380,
 	.t_frame = 4096,
 	.t_p_ack = 545,
 	.rssi_base_val = -91,
@@ -1267,6 +1293,8 @@ static struct at86rf2xx_chip_data at86rf212_data = {
 	.t_reset_to_off = 26,
 	.t_off_to_aack = 200,
 	.t_off_to_tx_on = 200,
+	.t_off_to_sleep = 35,
+	.t_sleep_to_off = 380,
 	.t_frame = 4096,
 	.t_p_ack = 545,
 	.rssi_base_val = -100,
@@ -1640,6 +1668,16 @@ static int at86rf230_probe(struct spi_device *spi)
 	if (rc)
 		goto free_dev;
 
+	/* disable_irq by default and wait for starting hardware */
+	disable_irq(spi->irq);
+
+	/* going into sleep by default */
+	if (gpio_is_valid(slp_tr)) {
+		gpio_set_value(slp_tr, 1);
+		usleep_range(lp->data->t_off_to_sleep,
+			     lp->data->t_off_to_sleep + 10);
+	}
+
 	rc = ieee802154_register_hw(lp->hw);
 	if (rc)
 		goto free_dev;
-- 
2.4.1

Re: [PATCH bluetooth-next 4/4] at86rf230: add support for sleep state

[Marcel Holtmann]

Hi Alex,

> This patch adds support for sleep state when between stop and start
> period. In this period the transceiver isn't used by the subsystem, in
> this time we disable the irq and going into the sleep state.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
> drivers/net/ieee802154/at86rf230.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH bluetooth-next 1/4] mac802154: iface: fix hrtimer cancel on ifdown

[Marcel Holtmann]

Hi Alex,

> The interframe spacing timer is a per phy definition and is part of a
> ieee802154_local structure. If we have possible multiple interfaces
> ifdown one interface then the timer should not be cancled. First if the
> last interface is down and the receive handling is stopped we should be
> sure that the interframe spacing timer isn't run anymore.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
> net/mac802154/iface.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH bluetooth-next 1/4] mac802154: iface: fix hrtimer cancel on ifdown

[Lennert Buytenhek]

On Sat, Jun 13, 2015 at 10:15:51PM +0200, Alexander Aring wrote:

> The interframe spacing timer is a per phy definition and is part of a
> ieee802154_local structure. If we have possible multiple interfaces
> ifdown one interface then the timer should not be cancled. First if the
> last interface is down and the receive handling is stopped we should be
> sure that the interframe spacing timer isn't run anymore.

What is the ifs timer handling in the core code needed for in the
first place?

I understand that the 802.15.4 code keeps the interface queue stopped
as long as the PHY is transmitting the packet, and this makes sense,
given that probably none of the hardware involved contains a DMA ring
or anything like that, but I don't think that a (hr)timer is the way
to do it.

The right way to trigger transmit queue waking would be to do it when
receiving a transmit completion interrupt, or if the hardware doesn't
support that, by setting up a timer and possibly doing some polling in
the driver itself, to see if the packet has gone out on the wire yet
(which can be delayed for a multitude of reasons).

Or, just have the driver xmit() function check on entry whether the
previous packet has been transmitted yet and if not, stop the queue
and set a polling timer -- this way you won't get any transmit
completion interrupts or timer expirations if the transmit rate is
low, which would save a little bit of CPU time and CPU wakeups.

The current ifs timer handling logic does not, and by definition
cannot, take into account time needed for things such as clear channel
assessment, packet retransmissions, etc, and this isn't really fixable.
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in

Re: [PATCH bluetooth-next 1/4] mac802154: iface: fix hrtimer cancel on ifdown

[Alexander Aring]

On Sat, Jun 20, 2015 at 08:19:20AM +0300, Lennert Buytenhek wrote:
> On Sat, Jun 13, 2015 at 10:15:51PM +0200, Alexander Aring wrote:
> 
> > The interframe spacing timer is a per phy definition and is part of a
> > ieee802154_local structure. If we have possible multiple interfaces
> > ifdown one interface then the timer should not be cancled. First if the
> > last interface is down and the receive handling is stopped we should be
> > sure that the interframe spacing timer isn't run anymore.
> 
> What is the ifs timer handling in the core code needed for in the
> first place?
> 

What is the ifs timer handling:

Refer 5.1.1.3 Interframe spacing (IFS) in 802.15.4-2011.


In the core mode:

Because it's a typical use case for the mode (max_frame_retries = -1).
This is what already mentioned the "transmit only" mode.

When a transceiver is in this mode it doesn't do any CSMA-CA handling
and ARET handling. In case of at86rf2xx I get heavy transmit issues when
more than one frame is transmitted. Again, this is only when
max_frame_retries = -1.

I suppose the reason is because the transceiver have no time in
"receiving mode" to detect the SFD (preamble), we going directly into tx
state again. With introducing a delay for starting the next transmit,
the problem was gone for me.


Meanwhile I asked the atmel support how the transceiver handles the
interframe spacing time and they answered with:

---
In PLL_ON state (i.e. Basic Operating mode) software have to handle
the LIFS and SIFS intervals based on the transmit frame length. In
TX_ARET_ON state transceiver waits for the ACK and based on this
retransmission is handled automatically by the transceiver.
---

This makes totally sense because we can't handle LIFS/SIFS in the middle
of ARET mode, it's fully done by transceiver. I also don't get the above
issue while turning into ARET mode.

This confirmed my experience which I had in the PLL_ON mode
(max_frame_retries = -1).

The first solution was simple to add a udelay after each transmit to do
a delayed wake queue for the next transmit. Since I got the response
from atmel and (there are for my opinion more transceivers which doesn't
care about the ifs handling on transceiver), we introduce it into core
functionality with a timer (which is definitely better than an udelay).

> I understand that the 802.15.4 code keeps the interface queue stopped
> as long as the PHY is transmitting the packet, and this makes sense,
> given that probably none of the hardware involved contains a DMA ring
> or anything like that, but I don't think that a (hr)timer is the way
> to do it.

Then send patches for a better way. Currently hrtimer is better as a udelay.

> 
> The right way to trigger transmit queue waking would be to do it when
> receiving a transmit completion interrupt, or if the hardware doesn't

That's what we doing now, see [0]. The last argument indicates the ifs
handling and this need to be done by software in non tx_aret mode.

> support that, by setting up a timer and possibly doing some polling in
> the driver itself, to see if the packet has gone out on the wire yet
> (which can be delayed for a multitude of reasons).
> 

Don't know what you exactly means using polling here. The timer should
simple delay the next transmission, by using a timer which do a delayed
wake of "all" interface queues which belongs to a phy.

> Or, just have the driver xmit() function check on entry whether the
> previous packet has been transmitted yet and if not, stop the queue
> and set a polling timer -- this way you won't get any transmit

The xmit callback _cannot_ called twice until a frame has not been transmitted.
For each xmit we stop all queues for the phy and if complete then we
awake all queues again. The main reason for doing it is because the
transceivers has one frame buffer only and I never saw any other
transceiver with more than more framebuffer.

> completion interrupts or timer expirations if the transmit rate is
> low, which would save a little bit of CPU time and CPU wakeups.
> 
> The current ifs timer handling logic does not, and by definition
> cannot, take into account time needed for things such as clear channel
> assessment, packet retransmissions, etc, and this isn't really fixable.

Yes, this is why I the IFS handling is done by transceiver when
max_frame_retries >= 0. In case of max_frame_retries = -1 this need to
be handled by software.

- Alex

[0] http://lxr.free-electrons.com/source/drivers/net/ieee802154/at86rf230.c#L717
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in