Hi,
On 01/04/2017 02:30 PM, Harry Morris wrote:
> Hi Alex,
>
> Sorry for such a late reply, I wanted to see if I could implement the async spi behaviour completely before responding and it's been a pretty big change. (and holidays) :)
>
> On 15/12/2016 20:06, Alexander Aring wrote:
>> Hi,
>>
>> On 12/15/2016 02:52 PM, Harry Morris wrote:
>>> Hi Alex,
>>>
>>> On 12/12/2016 14:27, Alexander Aring wrote:
>>>> All these spi stuff is low-level spi functionality, why not simple use
>>>> the spi helper functions. E.g. "spi_write_then_read" and I don't mean
>>>> only just this code, I mean every code here.
>>>>
>>>> Low-level is _maybe_ needed if you using async api.
>>> As far as I can see, I'm unable to use the helper functions because I need (effectively) manual control of the spi chip select. The way the ca8210 API works I don't know how many bytes I'm reading in total until I read the "command length" byte. So I need to read two bytes, keep the chip select asserted while I process the length and then read the rest of the data. This is the same for the tx path since the ca8210 operates in full duplex mode, so when transmitting I may also be receiving a command that is longer than what I am transmitting so there is a risk of me de-asserting the chip select too early using a helper. Maybe I'm not completely understanding how the helpers work though.
>>>
>> Sorry to say that, but ... this is impossible with the spi subsystem.
>>
>> I mean it's possible, but if you doing that there exists spi controller
>> drivers/hardware stuff which doesn't support it. The cs_change property
>> doesn't work on all hardware.
>>
>> People told me to avoid it, otherwise I could easy implement receive
>> handling at at86rf230 driver to not drop cs after framebuffer read.
>> Deasserting cs after framebuffer read will drop the framebuffer
>> protection there and new frames can overwrite the framebuffer...
>>
>> We currently always read the whole framebuffer instead first fetching
>> the length and then read the length only.
>>
>> Is something somehow possible in your case? You should be careful using
>> cs_change attribute it doesn't work on all hardware.
> This is possible in my case, it just reduces the bandwidth slightly. I have implemented this behaviour for the next patch set.
>
that's good! Thanks :-)
>>>>> + msdulen = data_ind[22]; /* msdu_length */
>>>>> + if (msdulen > 127) {
>>>> use ieee802154_is_valid_psdu_len.
>>>>
>>>> Also this is psdu_len, in case of monitors -> monitors will filter then
>>>> minimum ACK frame length only.
>>> So the reason for this is that this *is* actually msdu length and not psdu length. What's being received is an MCPS-DATA.indication, so the chip has already performed the is_valid_psdu_len check in its MAC. Truthfully this check is probably completely unnecessary, I just added it in case something gets messed up over the SPI but I've never seen it happen. Maybe it should be removed altogether, I just thought it's a simple check to perform that could easily highlight a big problem.
>>>
>> ah, ok. If you know what the firmware filters and what not, then
>> everything is fine. I did the expierence e.g. at at86rf230 it's simple
>> not documented what they filter for length at firmware side...
>>
>> but you know the firmware so everything fine.
>>
>>>>> + dev_err(
>>>>> + &priv->spi->dev,
>>>>> + "received erroneously large msdu length!\n"
>>>>> + );
>>>>> + kfree_skb(skb);
>>>>> + return -EMSGSIZE;
>>>>> + }
>>>>> + dev_dbg(&priv->spi->dev, "skb buffer length = %d\n", msdulen);
>>>>> +
>>>>> + /* Populate hdr *
>>>> Doing header parsing below here will not work with monitors, monitors
>>>> can also receive stuff which are smaller than 29 - I don't know what's
>>>> already filtered on firmware side.
>>> Again the hardware MAC will have already filtered out any frames that are not valid data frames, so the header of the data indication will always be a fixed 29 bytes.
>> You are sure about this? If you receiving ack frames there will no
>> addressing stuff and you will parse the frame here at positions without
>> length checking if it's such frame.
>>
>> I can understand if you saying "ack" frames will never hit the driver,
>> but begin to parse frames here... I don't know exactly the "why"?
>>
>> 802.15.4 frame parsing is very complex and we should some one code for
>> SoftMAC to do that.
> So the reason for this is just the program flow. This point in the code will only be reached after a data frame is being indicated. The command id of the API command is checked to make sure it's an MCPS-DATA.indication and the ca8210 command API has a fixed header for all the data indication parameters. So if we are checking the "frame" here we can be certain that it is always an MCPS-DATA.indication and we know that the header is always a fixed 29 bytes before the payload.
>
"This point in the code will only be reached after a data frame"
This is currently not what it's made for... We currently support data
frames only and the reason why it works for you.
It smells really again like you having HardMAC transceiver here and want
to put it into the SoftMAC stack.
Everything will be reparsed then in SoftMAC layer...
Anyway, it's okay for me, but you should be prepared if other SoftMAC
transceivers receives Beacon/CMD, frames and it works for them... it
will not work for you then.
>>>> I see the driver doesn't implement any promiscuousmode functionality,
>>>> you need to care about that if you will add support for that later.
>>> When it comes to promiscuous mode, the psdu is just copied into the msdu of the data indication. So again there will be a fixed 29 byte header on the front, just the contents is meaningless (zeroed). Again this is because the specification doesn't say how promiscuous frames are to be passed out of the MAC.
>>>>> + hdr.sec.level = data_ind[29 + msdulen];
>>>>> + dev_dbg(&priv->spi->dev, "security level: %#03x\n", hdr.sec.level);
>>>>> + if (hdr.sec.level > 0) {
>>>>> + hdr.sec.key_id_mode = data_ind[30 + msdulen];
>>>>> + memcpy(&hdr.sec.extended_src, &data_ind[31 + msdulen], 8);
>>>>> + hdr.sec.key_id = data_ind[39 + msdulen];
>>>>> + }
>>>>> + hdr.source.mode = data_ind[0];
>>>>> + dev_dbg(&priv->spi->dev, "srcAddrMode: %#03x\n", hdr.source.mode);
>>>>> + hdr.source.pan_id = *(u16 *)&data_ind[1];
>>>>> + dev_dbg(&priv->spi->dev, "srcPanId: %#06x\n", hdr.source.pan_id);
>>>>> + memcpy(&hdr.source.extended_addr, &data_ind[3], 8);
>>>>> + hdr.dest.mode = data_ind[11];
>>>>> + dev_dbg(&priv->spi->dev, "dstAddrMode: %#03x\n", hdr.dest.mode);
>>>>> + hdr.dest.pan_id = *(u16 *)&data_ind[12];
>>>>> + dev_dbg(&priv->spi->dev, "dstPanId: %#06x\n", hdr.dest.pan_id);
>>>>> + memcpy(&hdr.dest.extended_addr, &data_ind[14], 8);
>>>>> +
>>>>> + /* Fill in FC implicitly */
>>>>> + hdr.fc.type = 1; /* Data frame */
>>>>> + if (hdr.sec.level)
>>>>> + hdr.fc.security_enabled = 1;
>>>>> + else
>>>>> + hdr.fc.security_enabled = 0;
>>>>> + if (data_ind[1] != data_ind[12] || data_ind[2] != data_ind[13])
>>>>> + hdr.fc.intra_pan = 1;
>>>>> + else
>>>>> + hdr.fc.intra_pan = 0;
>>>>> + hdr.fc.dest_addr_mode = hdr.dest.mode;
>>>>> + hdr.fc.source_addr_mode = hdr.source.mode;
>>>>> +
>>>>> + /* Add hdr to front of buffer */
>>>>> + hlen = ieee802154_hdr_push(skb, &hdr);
>>>>> +
>>>>> + if (hlen < 0) {
>>>>> + dev_crit(&priv->spi->dev, "failed to push mac hdr onto skb!\n");
>>>>> + kfree_skb(skb);
>>>>> + return hlen;
>>>>> + }
>>>>> +
>>>>> + skb_reset_mac_header(skb);
>>>>> + skb->mac_len = hlen;
>>>>> +
>>>>> + /* Add <msdulen> bytes of space to the back of the buffer */
>>>>> + /* Copy msdu to skb */
>>>>> + memcpy(skb_put(skb, msdulen), &data_ind[29], msdulen);
>>>>> +
>>>>> + ieee802154_rx_irqsafe(hw, skb, data_ind[23]/*LQI*/);
>>>>> + /* TODO: Set protocol & checksum? */
>>>>> + /* TODO: update statistics */
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * ca8210_net_rx() - Acts upon received SAP commands relevant to the network
>>>>> + * driver
>>>>> + * @hw: ieee802154_hw that command was received by
>>>>> + * @command: Octet array of received command
>>>>> + * @len: length of the received command
>>>>> + *
>>>>> + * Called by the spi driver whenever a SAP command is received, this function
>>>>> + * will ascertain whether the command is of interest to the network driver and
>>>>> + * take necessary action.
>>>>> + *
>>>>> + * Return: 0 or linux error code
>>>>> + */
>>>>> +static int ca8210_net_rx(struct ieee802154_hw *hw, u8 *command, size_t len)
>>>>> +{
>>>>> + struct ca8210_priv *priv = hw->priv;
>>>>> + unsigned long flags;
>>>>> + u8 status;
>>>>> +
>>>>> + dev_dbg(&priv->spi->dev, "ca8210_net_rx(), CmdID = %d\n", command[0]);
>>>>> +
>>>>> + if (command[0] == SPI_MCPS_DATA_INDICATION) {
>>>>> + /* Received data */
>>>>> + spin_lock_irqsave(&priv->lock, flags);
>>>>> + if (command[26] == priv->last_dsn) {
>>>>> + dev_dbg(
>>>>> + &priv->spi->dev,
>>>>> + "DSN %d resend received, ignoring...\n",
>>>>> + command[26]
>>>>> + );
>>>>> + spin_unlock_irqrestore(&priv->lock, flags);
>>>>> + return 0;
>>>>> + }
>>>>> + priv->last_dsn = command[26];
>>>>> + spin_unlock_irqrestore(&priv->lock, flags);
>>>>> + return ca8210_skb_rx(hw, len - 2, command + 2);
>>>>> + } else if (command[0] == SPI_MCPS_DATA_CONFIRM) {
>>>>> + status = command[3];
>>>>> + if (priv->async_tx_pending) {
>>>>> + return ca8210_async_xmit_complete(
>>>>> + hw,
>>>>> + command[2],
>>>>> + status
>>>>> + );
>>>>> + } else if (priv->sync_tx_pending) {
>>>>> + priv->sync_tx_pending = false;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * ca8210_skb_tx() - Transmits a given socket buffer using the ca8210
>>>>> + * @skb: Socket buffer to transmit
>>>>> + * @msduhandle: Data identifier to pass to the 802.15.4 MAC
>>>>> + * @priv: Pointer to private data section of target ca8210
>>>>> + *
>>>>> + * Return: 0 or linux error code
>>>>> + */
>>>>> +static int ca8210_skb_tx(
>>>>> + struct sk_buff *skb,
>>>>> + u8 msduhandle,
>>>>> + struct ca8210_priv *priv
>>>>> +)
>>>>> +{
>>>>> + int status;
>>>>> + struct ieee802154_hdr header = { 0 };
>>>>> + struct secspec secspec;
>>>>> + unsigned int mac_len;
>>>>> +
>>>>> + dev_dbg(&priv->spi->dev, "ca8210_skb_tx() called\n");
>>>>> +
>>>>> + /* Get addressing info from skb - ieee802154 layer creates a full
>>>>> + * packet
>>>>> + */
>>>>> + mac_len = ieee802154_hdr_peek_addrs(skb, &header);
>>>>> +
>>>> you need at least check if error occurred. Currently we support at least
>>>> 3 bytes FC + SEQ is there at driver level...
>>> Sorry but I'm not sure I understand what you mean, do I need to check for errors in the skb I'm being told to transmit? Over-air error checking is performed by the ca8210 with the use of a frame counter so maybe this doesn't apply to this situation?
>> I want to say here, it's possible that this transmit function get's at
>> least 3 bytes from userspace. Parsing addresses will fail here, because
>> the header is too small -> it will transmit random data.
> Ah okay I understand now, I'm assuming this is for transmitting acknowledgements. I'll be throwing them away since ACKs are handled by the hardware.
>
Exactly, some Transceivers also need to check on Frame Control Field to
prepare register settings for ack handling e.g. mrf24j40.
>>>>> +/**
>>>>> + * ca8210_xmit_sync() - Synchronously transmits a given socket buffer using the
>>>>> + * ca8210
>>>>> + * @hw: ieee802154_hw of ca8210 to transmit from
>>>>> + * @skb: Socket buffer to transmit
>>>>> + *
>>>>> + * Transmits the buffer and does not return until a confirmation of the exchange
>>>>> + * is received from the ca8210.
>>>>> + *
>>>>> + * Return: 0 or linux error code
>>>>> + */
>>>>> +static int ca8210_xmit_sync(struct ieee802154_hw *hw, struct sk_buff *skb)
>>>>> +{
>>>>> + struct ca8210_priv *priv = hw->priv;
>>>>> + int status;
>>>>> +
>>>>> + dev_dbg(&priv->spi->dev, "calling ca8210_xmit_sync()\n");
>>>>> +
>>>>> + status = ca8210_skb_tx(skb, priv->nextmsduhandle++, priv);
>>>>> + if (status)
>>>>> + return status;
>>>>> +
>>>>> + priv->sync_tx_pending = true;
>>>>> +
>>>>> + while (priv->sync_tx_pending)
>>>>> + msleep(1);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * ca8210_xmit_async() - Asynchronously transmits a given socket buffer using
>>>>> + * the ca8210
>>>>> + * @hw: ieee802154_hw of ca8210 to transmit from
>>>>> + * @skb: Socket buffer to transmit
>>>>> + *
>>>>> + * Hands the transmission of the buffer off to a workqueue and returns
>>>>> + * immediately.
>>>>> + *
>>>>> + * Return: 0 or linux error code
>>>>> + */
>>>>> +static int ca8210_xmit_async(struct ieee802154_hw *hw, struct sk_buff *skb)
>>>>> +{
>>>>> + struct ca8210_priv *priv = hw->priv;
>>>>> +
>>>>> + dev_dbg(&priv->spi->dev, "calling ca8210_xmit_async()\n");
>>>>> +
>>>>> + priv->tx_skb = skb;
>>>>> + queue_work(priv->async_tx_workqueue, &priv->async_tx_work);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>> Okay, both sync and async are here implemented for that the callbacks
>>>> are never made for.
>>>>
>>>> The above sync function looks like an synced operation around a async
>>>> operation, because you wait until it's finished.
>>>>
>>>> More bad, you use busy-waiting there, look for "wait_for_completion" in
>>>> linux kernel.
>>>>
>>>> Nevertheless, the sync operation will not be used when async is defined,
>>>> so sync operation is useless here.
>>>>
>>>> THe idea of async is that the Linux kernel doesn't do _anything_ until a
>>>> tx complete irq reports "tx is done" and wake the tx queue again. This
>>>> will not happen if you queue it to a workqueue again... which makes the
>>>> whole stuff synced again.
>>>>
>>>> If we one day have MLME-OPS it needs a synced xmit functionality ->
>>>> which will use xmit_async then - because we can and need transmit stuff
>>>> synced. But NOT for dataplane -> dataframes, these frames should be
>>>> synced completely undepended from linux kernel. The kernel doesn't wait
>>>> until it's done -> hardware will report it when it's done.
>>>>
>>>> The issue will begin when we have synced xmit above async
>>>> functionality... Then we have a workqueue above a workqueue which is bad.
>>> Okay so I've probably misunderstood how the sync and async functions should be implemented, sorry about that. I am still a bit confused though.
>>> Regarding async, you say I shouldn't use a workqueue, but the context is atomic right? So if I follow my transmission path directly it will call spi_sync which can sleep. Does this mean that I *have* to use spi_async() instead? That would be a shame since it will make transmission a lot more complicated because I have to perform multiple spi exchanges to read the correct length.
>>>
>> Yes, async is the right way to do it.
>>
>> What the callback of netdevice xmit_do is "send the frame out" queue
>> this into a workqueue will delay it and drop the context of netdevice
>> xmit_do function...
>>
>> You should use async in xmit_do and irq handling.
> I am using async now and I think it's all working fine, I'm still using a workqueue for the irq handling. This is because I ran into a deadlock problem calling the spi_async directly in the irq handler. I have a mutex to protect the spi buffers and sometimes the following would happen: The SPI mutex is locked while a transfer is in progress then an IRQ is received causing the interrupt handler to wait forever for the mutex to be released because it can't sleep. Is it acceptable for the IRQ to still use a workqueue or would it be better to remove it? If so I can try to change the locking or remove the need for it.
>
Good that async works for you now!
Why using locks here? :-) Look at at86rf230 driver, it has no locks at
all... just irq_enable/irq_disable - This is mostly because High-Level
triggered irq's and clearing the irq-line.
Use resources on Heap, I tried to use a global resource at first to
avoid heap allocation... but at the end it doesn't matter and you will
only get more and more issues to handle it right. I think benefits are
more to have a small disabled irq time.
To your question about if it's acceptable:
Change it to threaded irq's which can sleep -> not using own generates
workqueues... It's acceptable but not the best solution. :-)
- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
