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.
+ 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.
+ 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.
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?
+/**
+ * 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.
Would it maybe be better just to implement xmit_sync and not xmit_async?
Is that acceptable since it's to be deprecated? Using
wait_for_completion definitely seems like a good idea though, thanks for
pointing that out.
The reason why it looks async is because of the use of the management
entity primitives - the transmission path is sending an
MCPS-DATA.request command and then waiting for an MCPS-DATA.confirm to
come back from the chip, confirming successful transmission. Just
sending the request to the chip does not guarantee that the frame was or
will be transmitted.
Thanks for the feedback,
Regards,
Harry
--
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
