Re: [PATCH] drivers/net/ieee802154/adf7242: Driver for ADF7242 MAC IEEE802154

[Michael Hennerich <michael.hennerich@analog.com>]

On 12/08/2015 06:11 PM, Alexander Aring wrote:
> Hi,
>
> On Tue, Dec 08, 2015 at 09:43:20AM +0100, Michael Hennerich wrote:
>> On 12/07/2015 02:53 PM, Alexander Aring wrote:
>>> Hi,
>>>
>>> On Mon, Dec 07, 2015 at 01:47:06PM +0100, Michael Hennerich wrote:
>>> ...
>>>>>> +static struct ieee802154_ops adf7242_ops = {
>>>>>> +    .owner = THIS_MODULE,
>>>>>> +    .xmit_sync = adf7242_xmit,
>>>>>> +    .ed = adf7242_ed,
>>>>>> +    .set_channel = adf7242_channel,
>>>>>> +    .set_hw_addr_filt = adf7242_set_hw_addr_filt,
>>>>>> +    .start = adf7242_start,
>>>>>> +    .stop = adf7242_stop,
>>>>>> +    .set_csma_params = adf7242_set_csma_params,
>>>>>> +    .set_frame_retries = adf7242_set_frame_retries,
>>>>>> +    .set_txpower = adf7242_set_txpower,
>>>>>> +    .set_promiscuous_mode = adf7242_set_promiscuous_mode,
>>>>>> +    .set_cca_ed_level = adf7242_set_cca_ed_level,
>>>>>
>>>>> Nice to see so many callbacks implemented. The only things I see missing
>>>>> is xmit_async, set_lbt and set_cca_mode. I would not make it a
>>>>> requirements to get these hooked up before merging this patch but we
>>>>> should consider it as todo items.
>>>>
>>>> The part only supports CCA mode Energy above threshold.
>>>> Not sure what this LBT mode does on the AT86RFxxx driver.
>>>
>>> This is for sub 1Ghz regulations in some country (Japan/Europe) area,
>>> there CSMA/CA accoridng 802.15.4 isn't allowed at sub 1-Ghz, that's why
>>> they introduced LBT.
>>>
>>> That reminds me to work on a regulator db, again. :-)
>>>
>>> Nevertheless it should not related to 2.4 Ghz global ISM band, so far I
>>> know.
>>>
>>>> The ADF7242 only supports CSMA-CA and not some other listen before talk
>>>> flavour. The only oher option is to turn CSMA-CA completly off.
>>>
>>> Another thing for ToDo list, add support for turning CSMA-CA handling
>>> complete off, many transceiver has such option.
>>>
>>> There exists ways currently to turn off CSMA handling only by choosing
>>> the right backoff exponents, 802.15.4 writes:
>>>
>>> Note that if macMinBE is set to zero, collision avoidance will be
>>> disabled during the first iteration of this algorithm.
>>
>> First iteration only? And I think that is for slotted operation.
>> I wouldn't overload MinBe with an option to disable CSMA-CA.
>> Maybe add a new command?
>>
>>
>
> "CSMA" != "CSMA-CA"
>
> The calculation is for backoff periods (aUnitBackoffPeriod) is:
>
> 2^(MINBE + 1) - 1
>
> doesn't matter if slotted/unslotted. See page 23 at [0].
>
> By disable CSMA-CA I usually assume that the CCA status will not
> performed. Disable CSMA -> CCA status will be performed.
>
> I agree to add an own command to disable "CSMA-CA", to disable "CSMA" we
> have minBe.
>

Hi Alex,

ok - I see.



>>>
>>> Okay then another ToDo for wpan-tools would be to make a nice printout
>>> that CSMA is disabled if "macMinBE is set to zero".
>>>
>>>> I'm also not sure if we need to supprot the async mode, while the sync mode
>>>> is working. For me the async mode looks like it tries to workaround some HW
>>>> access issues.
>>>>
>>>
>>> We came to the conclusion that "sync" callback is a workaround that
>>> people can use spi_sync. :-)
>>
>> Hmmm - I don't quite understand.
>>
>> The difference is that xmit_sync blocks until the packet is transmitted,
>> while async returns immediately.
>
> Exact, and "blocking" is not what the netdev api offers by callback
> ".ndo_start_xmit".

I know.


>
>> spi_sync can be only used from context that can sleep. While spi_async
>> runs it's own queue and provides a completion callback.
>>
>> These radios can only do one thing at the time. And in both cases there are
>> queues that are being stopped while the radio driver is busy doing
>> something.
>>
>
> Yes, most transceivers has only one framebuffer. Btw: AT86RFxxx has
> really only one for rx/tx.

Yeah - the ADF7242 also has two buffers. But still 802.15.4 is 
TDD/Simplex. So either RX or TX. And in fact while TX with ACKreq, the 
RX buffer is used to store the ACK, and while RX data in the TX buffer 
might be overwritten by the ACK that is being transmitted.


>
>> So the only thing is the IF down/up issue?
>>
>
> I am not sure, I thought about that the whole day and I need to admit: I
> think both ways have some races _currently_.
>
> What I am suppose is that we can easier deal which such races when we
> use xmit_async without having a workqueue in the middle of
> "ndo_start_xmit" and "driver xmit" callback.


I don't see the difference with using spi_async here.
Instead of your own workqueue. You defer the work to the SPI master 
queue (kthread)...
In both cases all messages are sequential. And I don't see any timing 
benefit. It actually makes things more complex.
Think about the case during xmit where you have to check status -> write 
some registers -> check some other status -> write some more 
regsiters/data -> ...

For this to work you have to chain multiple async messages.
And rely on the goodwill of the SPI master kthread to pump them out.



>
> Furthermore, with MLME-ops we need to send frames out which are triggered
> by a workqueue as well, mac80211 MLME-ops do the same.
>
> But there exist a difference by starting a xmit workqueue from "ndo_start_xmit"
> or "netlink command".
>
>
> ----
>
> Btw: while thinking the whole day I detected a "deadlock" (maybe
> somebody can confirm it).
>
> At [1], we have ndo_stop callback which is called under RTNL lock. This
> will call ieee802154_stop_device, which also flushed the xmit workqueue
> (That means it will wait until the workqueue is complete scheduled).
> Now the "worker" callback of the xmit workqueue also hold the RTNL lock,
> see [2]. If this is called when "stop callback" [1] is waiting for the
> workqueue it will wait forever, because the xmit workqueue will wait on
> RTNL lock.


That looks racy.
In the xmit_async path there is no such lock.
I don't know why sync versus async would need it
- remove it altogether with the netif_running check.


>
> That the workqueue held the rtnl lock while transmit was one of my
> earlier changes, where somebody had some issues when doing ifdown. Maybe
> this change do "something" which fixed it but the real issue was an
> another. Now, when stop doing "flush_workqueue" and all netdev queues
> (which belongs to a phy) are stopped, then there can't be a 802.15.4
> driver "stop" callback.
>
> Also if we wait for the workqueue and the netdev queue was stopped
> before, it will be awake again because the driver will finish it.
>
>
> I have some headache now and I think we need to discuss again about the
> correct handling of "transmit" and look if everything works fine then
> without races, but it isn't easy. What I currently think is that
> "xmit_async" goes to the right direction, that we don't need to deal
> with the loosing context of "ndo_start_xmit" when call driver xmit
> callback.
>
>> The ADF7242 would actually return an error if the packet wasn’t sent
>> or ACKed by the receiver. It's still looks like this information
>> isn’t being used anywhere.
>>
>
> Yes, MLME-ops needs this option, we don't have currently any functionality to
> tell the "why transmit failed" to upper-layers (mac802154).
>
> Such stats should be available by "iwpan", like "iw station dump" in wireless.
> Then you can do some statistic counts. (Maybe the first use case to see
> that it works.)
>
>> Not a big deal - but in addition received ACKs (that would potentially
>> indicate that the packet was successfully delivered) cause warnings and are
>> completely dropped as well.
>>
>
> - Alex
>
> [0] ISBN 978-0-7381-6684-1 STDPD97126 (I hope they handle revisions of
> 				       802.15.4 standard with this
> 				       number).
>      or simple use this link (found by google):
>      http://ecee.colorado.edu/~liue/teaching/comm_standards/2015S_zigbee/802.15.4-2011.pdf
>      it's pdf page 43.
> [1] http://lxr.free-electrons.com/source/net/mac802154/iface.c#L333
> [2] http://lxr.free-electrons.com/source/net/mac802154/tx.c#L41
>


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif