Checksum is specific for a chip spcification and it varies
(in size and type) between different hardware. It should be
handled in the driver then.
Moreover, shdlc spec doesn't mention crc as a part of the frame.
Update pn544_hci driver as well.
Signed-off-by: Waldemar Rymarkiewicz <[email protected]>
---
drivers/nfc/pn544_hci.c | 19 ++++++++++++++++++-
net/nfc/hci/shdlc.c | 27 ++-------------------------
2 files changed, 20 insertions(+), 26 deletions(-)
diff --git a/drivers/nfc/pn544_hci.c b/drivers/nfc/pn544_hci.c
index 9458d53..4e0716c 100644
--- a/drivers/nfc/pn544_hci.c
+++ b/drivers/nfc/pn544_hci.c
@@ -128,6 +128,7 @@ static struct nfc_hci_gate pn544_gates[] = {
/* Largest headroom needed for outgoing custom commands */
#define PN544_CMDS_HEADROOM 2
+#define PN544_CMDS_TAILROOM 2
struct pn544_hci_info {
struct i2c_client *i2c_dev;
@@ -576,6 +577,20 @@ static int pn544_hci_ready(struct nfc_shdlc *shdlc)
return 0;
}
+static void pn544_hci_add_len_crc(struct sk_buff *skb)
+{
+ u16 crc;
+ int len;
+
+ len = skb->len + 2;
+ *skb_push(skb, 1) = len;
+
+ crc = crc_ccitt(0xffff, skb->data, skb->len);
+ crc = ~crc;
+ *skb_put(skb, 1) = crc & 0xff;
+ *skb_put(skb, 1) = crc >> 8;
+}
+
static int pn544_hci_xmit(struct nfc_shdlc *shdlc, struct sk_buff *skb)
{
struct pn544_hci_info *info = nfc_shdlc_get_clientdata(shdlc);
@@ -584,6 +599,8 @@ static int pn544_hci_xmit(struct nfc_shdlc *shdlc, struct sk_buff *skb)
if (info->hard_fault != 0)
return info->hard_fault;
+ pn544_hci_add_len_crc(skb);
+
return pn544_hci_i2c_write(client, skb->data, skb->len);
}
@@ -874,7 +891,7 @@ static int __devinit pn544_hci_probe(struct i2c_client *client,
info->shdlc = nfc_shdlc_allocate(&pn544_shdlc_ops,
&init_data, protocols,
- PN544_CMDS_HEADROOM, 0,
+ PN544_CMDS_HEADROOM, PN544_CMDS_TAILROOM,
PN544_HCI_LLC_MAX_PAYLOAD,
dev_name(&client->dev));
if (!info->shdlc) {
diff --git a/net/nfc/hci/shdlc.c b/net/nfc/hci/shdlc.c
index d4fe5e9..8a5f034 100644
--- a/net/nfc/hci/shdlc.c
+++ b/net/nfc/hci/shdlc.c
@@ -22,7 +22,6 @@
#include <linux/sched.h>
#include <linux/export.h>
#include <linux/wait.h>
-#include <linux/crc-ccitt.h>
#include <linux/slab.h>
#include <linux/skbuff.h>
@@ -30,7 +29,6 @@
#include <net/nfc/shdlc.h>
#define SHDLC_LLC_HEAD_ROOM 2
-#define SHDLC_LLC_TAIL_ROOM 2
#define SHDLC_MAX_WINDOW 4
#define SHDLC_SREJ_SUPPORT false
@@ -94,28 +92,13 @@ static struct sk_buff *nfc_shdlc_alloc_skb(struct nfc_shdlc *shdlc,
struct sk_buff *skb;
skb = alloc_skb(shdlc->client_headroom + SHDLC_LLC_HEAD_ROOM +
- shdlc->client_tailroom + SHDLC_LLC_TAIL_ROOM +
- payload_len, GFP_KERNEL);
+ shdlc->client_tailroom + payload_len, GFP_KERNEL);
if (skb)
skb_reserve(skb, shdlc->client_headroom + SHDLC_LLC_HEAD_ROOM);
return skb;
}
-static void nfc_shdlc_add_len_crc(struct sk_buff *skb)
-{
- u16 crc;
- int len;
-
- len = skb->len + 2;
- *skb_push(skb, 1) = len;
-
- crc = crc_ccitt(0xffff, skb->data, skb->len);
- crc = ~crc;
- *skb_put(skb, 1) = crc & 0xff;
- *skb_put(skb, 1) = crc >> 8;
-}
-
/* immediately sends an S frame. */
static int nfc_shdlc_send_s_frame(struct nfc_shdlc *shdlc,
enum sframe_type sframe_type, int nr)
@@ -131,8 +114,6 @@ static int nfc_shdlc_send_s_frame(struct nfc_shdlc *shdlc,
*skb_push(skb, 1) = SHDLC_CONTROL_HEAD_S | (sframe_type << 3) | nr;
- nfc_shdlc_add_len_crc(skb);
-
r = shdlc->ops->xmit(shdlc, skb);
kfree_skb(skb);
@@ -151,8 +132,6 @@ static int nfc_shdlc_send_u_frame(struct nfc_shdlc *shdlc,
*skb_push(skb, 1) = SHDLC_CONTROL_HEAD_U | uframe_modifier;
- nfc_shdlc_add_len_crc(skb);
-
r = shdlc->ops->xmit(shdlc, skb);
kfree_skb(skb);
@@ -510,8 +489,6 @@ static void nfc_shdlc_handle_send_queue(struct nfc_shdlc *shdlc)
shdlc->nr);
/* SHDLC_DUMP_SKB("shdlc frame written", skb); */
- nfc_shdlc_add_len_crc(skb);
-
r = shdlc->ops->xmit(shdlc, skb);
if (r < 0) {
shdlc->hard_fault = r;
@@ -881,7 +858,7 @@ struct nfc_shdlc *nfc_shdlc_allocate(struct nfc_shdlc_ops *ops,
shdlc->hdev = nfc_hci_allocate_device(&shdlc_ops, init_data, protocols,
tx_headroom + SHDLC_LLC_HEAD_ROOM,
- tx_tailroom + SHDLC_LLC_TAIL_ROOM,
+ tx_tailroom,
max_link_payload);
if (shdlc->hdev == NULL)
goto err_allocdev;
--
1.7.10
On 09/06/2012 06:02 PM, Eric Lapuyade wrote:
> Hi Waldemar,
>
> On 09/06/2012 12:22 PM, Waldemar Rymarkiewicz wrote:
>> /* Largest headroom needed for outgoing custom commands */
>> #define PN544_CMDS_HEADROOM 2
>> +#define PN544_CMDS_TAILROOM 2
As a side effect of my comment on the second patch, this should be
defined like this:
#define PN544_CMDS_HEADROOM 2
+#define PN544_FRAME_HEADROOM 1
+#define PN544_FRAME_TAILROOM 2
>> static int pn544_hci_xmit(struct nfc_shdlc *shdlc, struct sk_buff *skb)
>> {
>> struct pn544_hci_info *info = nfc_shdlc_get_clientdata(shdlc);
>> @@ -584,6 +599,8 @@ static int pn544_hci_xmit(struct nfc_shdlc
*shdlc, struct sk_buff *skb)
>> if (info->hard_fault != 0)
>> return info->hard_fault;
>>
>> + pn544_hci_add_len_crc(skb);
>> +
>> return pn544_hci_i2c_write(client, skb->data, skb->len);
Here, we would have:
r = pn544_hci_i2c_write(client, skb->data, skb->len);
pn544_hci_remove_len_crc(skb);
return r;
>> info->shdlc = nfc_shdlc_allocate(&pn544_shdlc_ops,
>> &init_data, protocols,
>> - PN544_CMDS_HEADROOM, 0,
>> + PN544_CMDS_HEADROOM, PN544_CMDS_TAILROOM,
>> PN544_HCI_LLC_MAX_PAYLOAD,
>> dev_name(&client->dev));
And this should be:
info->shdlc = nfc_shdlc_allocate(&pn544_shdlc_ops,
&init_data, protocols,
PN544_CMDS_HEADROOM +
PN544_FRAME_HEADROOM,
PN544_FRAME_TAILROOM,
PN544_HCI_LLC_MAX_PAYLOAD,
dev_name(&client->dev));
note that PN544_CMDS_HEADROOM is really only for skb allocated by HCI
(or NFC Core), not by those allocated by shdlc, but we aggregate it all
in a single parameter for simplicity.
Eric
Hi Eric,
>> BTW I see the headroom for pn544 is 2 but it make use of 1 byte (len).
>> Is that correct?
>
> That question made me go back to have a closer look. With shdlc
> currently adding the len and crc, there would be no reason for the
> driver to request any headroom. If you look at the comment above
> PN544_CMDS_HEADROOM, it says "Largest headroom needed for outgoing
> custom commands". The headroom it requests it not for len, it is used by
> the driver to insert special bytes when handling a
> pn544_hci_data_exchange for a reader F gate, which is a special case.
That makes sense. I guess in 1/2 patch PN544_CMDS_HEADROOM should be 3
() now. Will fix that.
> I didn't remember that when I wrote the previous answer, and this
> defeats my second suggestion. I suggest we go with my first suggestion:
>
> - Specify that the driver xmit MUST NOT modify skb. It shall remove
> anything it inserts or appends before returning from xmit.
It sounds sane.
Thanks,
/Waldek
On 09/06/2012 12:22 PM, Waldemar Rymarkiewicz wrote:
> Purge data added by lower layers (len and crc) in the head and tail of the frame
> during initial send. Now, the frame is correct to be resent.
>
> Signed-off-by: Waldemar Rymarkiewicz <[email protected]>
> ---
> net/nfc/hci/shdlc.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/nfc/hci/shdlc.c b/net/nfc/hci/shdlc.c
> index 8a5f034..30e353e 100644
> --- a/net/nfc/hci/shdlc.c
> +++ b/net/nfc/hci/shdlc.c
> @@ -241,8 +241,9 @@ static void nfc_shdlc_requeue_ack_pending(struct nfc_shdlc *shdlc)
> pr_debug("ns reset to %d\n", shdlc->dnr);
>
> while ((skb = skb_dequeue_tail(&shdlc->ack_pending_q))) {
> - skb_pull(skb, 2); /* remove len+control */
> - skb_trim(skb, skb->len - 2); /* remove crc */
> + /* remove client head + shdlc control field */
> + skb_pull(skb, shdlc->client_headroom + 1);
> + skb_trim(skb, skb->len - shdlc->client_tailroom);
> skb_queue_head(&shdlc->send_q, skb);
> }
> shdlc->ns = shdlc->dnr;
>
Hi Waldemar,
This patch would work for PN544, but it makes the assumption that the
driver will always insert/append exactly client_headroom/client_tailroom
bytes when xmit is called. This is not specified nor enforced so it may
be a little dangerous.
To correct that, we can :
- either specify that the driver xmit MUST NOT modify skb. We then let
it remove those len and crc before returning from xmit. This is the most
logical since only the driver really knows what it's doing.
- or specify that the driver head/tailroom request MUST BE the exact
number of bytes added to the skb by xmit.
Samuel, what do you think?
Eric
Hi Eric,
> This patch would work for PN544, but it makes the assumption that the
> driver will always insert/append exactly client_headroom/client_tailroom
> bytes when xmit is called. This is not specified nor enforced so it may
> be a little dangerous.
Do you mean that the driver can request client_head/tailroom which is a
maximum it can use, but it does not mean that all space will be used in
in each frame e.g. due to optional fields? That's the only situation I
can imagine now.
BTW I see the headroom for pn544 is 2 but it make use of 1 byte (len).
Is that correct?
Thanks,
/Waldek
Hi Waldek,
On 09/07/2012 08:38 AM, Rymarkiewicz Waldemar wrote:
> Hi Eric,
>
>> This patch would work for PN544, but it makes the assumption that the
>> driver will always insert/append exactly client_headroom/client_tailroom
>> bytes when xmit is called. This is not specified nor enforced so it may
>> be a little dangerous.
>
> Do you mean that the driver can request client_head/tailroom which is a
> maximum it can use, but it does not mean that all space will be used in
> in each frame e.g. due to optional fields? That's the only situation I
> can imagine now.
Yes, this is exactly what I mean.
> BTW I see the headroom for pn544 is 2 but it make use of 1 byte (len).
> Is that correct?
That question made me go back to have a closer look. With shdlc
currently adding the len and crc, there would be no reason for the
driver to request any headroom. If you look at the comment above
PN544_CMDS_HEADROOM, it says "Largest headroom needed for outgoing
custom commands". The headroom it requests it not for len, it is used by
the driver to insert special bytes when handling a
pn544_hci_data_exchange for a reader F gate, which is a special case.
I didn't remember that when I wrote the previous answer, and this
defeats my second suggestion. I suggest we go with my first suggestion:
- Specify that the driver xmit MUST NOT modify skb. It shall remove
anything it inserts or appends before returning from xmit.
Eric
Purge data added by lower layers (len and crc) in the head and tail of the frame
during initial send. Now, the frame is correct to be resent.
Signed-off-by: Waldemar Rymarkiewicz <[email protected]>
---
net/nfc/hci/shdlc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/nfc/hci/shdlc.c b/net/nfc/hci/shdlc.c
index 8a5f034..30e353e 100644
--- a/net/nfc/hci/shdlc.c
+++ b/net/nfc/hci/shdlc.c
@@ -241,8 +241,9 @@ static void nfc_shdlc_requeue_ack_pending(struct nfc_shdlc *shdlc)
pr_debug("ns reset to %d\n", shdlc->dnr);
while ((skb = skb_dequeue_tail(&shdlc->ack_pending_q))) {
- skb_pull(skb, 2); /* remove len+control */
- skb_trim(skb, skb->len - 2); /* remove crc */
+ /* remove client head + shdlc control field */
+ skb_pull(skb, shdlc->client_headroom + 1);
+ skb_trim(skb, skb->len - shdlc->client_tailroom);
skb_queue_head(&shdlc->send_q, skb);
}
shdlc->ns = shdlc->dnr;
--
1.7.10
Hi Waldemar,
On 09/06/2012 12:22 PM, Waldemar Rymarkiewicz wrote:
> Checksum is specific for a chip spcification and it varies
> (in size and type) between different hardware. It should be
> handled in the driver then.
>
> Moreover, shdlc spec doesn't mention crc as a part of the frame.
>
> Update pn544_hci driver as well.
>
> Signed-off-by: Waldemar Rymarkiewicz <[email protected]>
> ---
> drivers/nfc/pn544_hci.c | 19 ++++++++++++++++++-
> net/nfc/hci/shdlc.c | 27 ++-------------------------
> 2 files changed, 20 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/nfc/pn544_hci.c b/drivers/nfc/pn544_hci.c
> index 9458d53..4e0716c 100644
> --- a/drivers/nfc/pn544_hci.c
> +++ b/drivers/nfc/pn544_hci.c
> @@ -128,6 +128,7 @@ static struct nfc_hci_gate pn544_gates[] = {
>
> /* Largest headroom needed for outgoing custom commands */
> #define PN544_CMDS_HEADROOM 2
> +#define PN544_CMDS_TAILROOM 2
>
> struct pn544_hci_info {
> struct i2c_client *i2c_dev;
> @@ -576,6 +577,20 @@ static int pn544_hci_ready(struct nfc_shdlc *shdlc)
> return 0;
> }
>
> +static void pn544_hci_add_len_crc(struct sk_buff *skb)
> +{
> + u16 crc;
> + int len;
> +
> + len = skb->len + 2;
> + *skb_push(skb, 1) = len;
> +
> + crc = crc_ccitt(0xffff, skb->data, skb->len);
> + crc = ~crc;
> + *skb_put(skb, 1) = crc & 0xff;
> + *skb_put(skb, 1) = crc >> 8;
> +}
> +
> static int pn544_hci_xmit(struct nfc_shdlc *shdlc, struct sk_buff *skb)
> {
> struct pn544_hci_info *info = nfc_shdlc_get_clientdata(shdlc);
> @@ -584,6 +599,8 @@ static int pn544_hci_xmit(struct nfc_shdlc *shdlc, struct sk_buff *skb)
> if (info->hard_fault != 0)
> return info->hard_fault;
>
> + pn544_hci_add_len_crc(skb);
> +
> return pn544_hci_i2c_write(client, skb->data, skb->len);
> }
>
> @@ -874,7 +891,7 @@ static int __devinit pn544_hci_probe(struct i2c_client *client,
>
> info->shdlc = nfc_shdlc_allocate(&pn544_shdlc_ops,
> &init_data, protocols,
> - PN544_CMDS_HEADROOM, 0,
> + PN544_CMDS_HEADROOM, PN544_CMDS_TAILROOM,
> PN544_HCI_LLC_MAX_PAYLOAD,
> dev_name(&client->dev));
> if (!info->shdlc) {
> diff --git a/net/nfc/hci/shdlc.c b/net/nfc/hci/shdlc.c
> index d4fe5e9..8a5f034 100644
> --- a/net/nfc/hci/shdlc.c
> +++ b/net/nfc/hci/shdlc.c
> @@ -22,7 +22,6 @@
> #include <linux/sched.h>
> #include <linux/export.h>
> #include <linux/wait.h>
> -#include <linux/crc-ccitt.h>
> #include <linux/slab.h>
> #include <linux/skbuff.h>
>
> @@ -30,7 +29,6 @@
> #include <net/nfc/shdlc.h>
>
> #define SHDLC_LLC_HEAD_ROOM 2
> -#define SHDLC_LLC_TAIL_ROOM 2
>
> #define SHDLC_MAX_WINDOW 4
> #define SHDLC_SREJ_SUPPORT false
> @@ -94,28 +92,13 @@ static struct sk_buff *nfc_shdlc_alloc_skb(struct nfc_shdlc *shdlc,
> struct sk_buff *skb;
>
> skb = alloc_skb(shdlc->client_headroom + SHDLC_LLC_HEAD_ROOM +
> - shdlc->client_tailroom + SHDLC_LLC_TAIL_ROOM +
> - payload_len, GFP_KERNEL);
> + shdlc->client_tailroom + payload_len, GFP_KERNEL);
> if (skb)
> skb_reserve(skb, shdlc->client_headroom + SHDLC_LLC_HEAD_ROOM);
>
> return skb;
> }
>
> -static void nfc_shdlc_add_len_crc(struct sk_buff *skb)
> -{
> - u16 crc;
> - int len;
> -
> - len = skb->len + 2;
> - *skb_push(skb, 1) = len;
> -
> - crc = crc_ccitt(0xffff, skb->data, skb->len);
> - crc = ~crc;
> - *skb_put(skb, 1) = crc & 0xff;
> - *skb_put(skb, 1) = crc >> 8;
> -}
> -
> /* immediately sends an S frame. */
> static int nfc_shdlc_send_s_frame(struct nfc_shdlc *shdlc,
> enum sframe_type sframe_type, int nr)
> @@ -131,8 +114,6 @@ static int nfc_shdlc_send_s_frame(struct nfc_shdlc *shdlc,
>
> *skb_push(skb, 1) = SHDLC_CONTROL_HEAD_S | (sframe_type << 3) | nr;
>
> - nfc_shdlc_add_len_crc(skb);
> -
> r = shdlc->ops->xmit(shdlc, skb);
>
> kfree_skb(skb);
> @@ -151,8 +132,6 @@ static int nfc_shdlc_send_u_frame(struct nfc_shdlc *shdlc,
>
> *skb_push(skb, 1) = SHDLC_CONTROL_HEAD_U | uframe_modifier;
>
> - nfc_shdlc_add_len_crc(skb);
> -
> r = shdlc->ops->xmit(shdlc, skb);
>
> kfree_skb(skb);
> @@ -510,8 +489,6 @@ static void nfc_shdlc_handle_send_queue(struct nfc_shdlc *shdlc)
> shdlc->nr);
> /* SHDLC_DUMP_SKB("shdlc frame written", skb); */
>
> - nfc_shdlc_add_len_crc(skb);
> -
> r = shdlc->ops->xmit(shdlc, skb);
> if (r < 0) {
> shdlc->hard_fault = r;
> @@ -881,7 +858,7 @@ struct nfc_shdlc *nfc_shdlc_allocate(struct nfc_shdlc_ops *ops,
>
> shdlc->hdev = nfc_hci_allocate_device(&shdlc_ops, init_data, protocols,
> tx_headroom + SHDLC_LLC_HEAD_ROOM,
> - tx_tailroom + SHDLC_LLC_TAIL_ROOM,
> + tx_tailroom,
> max_link_payload);
> if (shdlc->hdev == NULL)
> goto err_allocdev;
>
You are certainly right that the CRC belongs to the driver.
Acked-by: Eric Lapuyade <[email protected]>
Hi Eric,
> As a side effect of my comment on the second patch, this should be
> defined like this:
>
> #define PN544_CMDS_HEADROOM 2
> +#define PN544_FRAME_HEADROOM 1
> +#define PN544_FRAME_TAILROOM 2
>
>>> static int pn544_hci_xmit(struct nfc_shdlc *shdlc, struct sk_buff *skb)
>>> {
>>> struct pn544_hci_info *info = nfc_shdlc_get_clientdata(shdlc);
>>> @@ -584,6 +599,8 @@ static int pn544_hci_xmit(struct nfc_shdlc
> *shdlc, struct sk_buff *skb)
>>> if (info->hard_fault != 0)
>>> return info->hard_fault;
>>>
>>> + pn544_hci_add_len_crc(skb);
>>> +
>>> return pn544_hci_i2c_write(client, skb->data, skb->len);
>
> Here, we would have:
>
> r = pn544_hci_i2c_write(client, skb->data, skb->len);
> pn544_hci_remove_len_crc(skb);
> return r;
>
>>> info->shdlc = nfc_shdlc_allocate(&pn544_shdlc_ops,
>>> &init_data, protocols,
>>> - PN544_CMDS_HEADROOM, 0,
>>> + PN544_CMDS_HEADROOM, PN544_CMDS_TAILROOM,
>>> PN544_HCI_LLC_MAX_PAYLOAD,
>>> dev_name(&client->dev));
>
> And this should be:
>
> info->shdlc = nfc_shdlc_allocate(&pn544_shdlc_ops,
> &init_data, protocols,
> PN544_CMDS_HEADROOM +
> PN544_FRAME_HEADROOM,
> PN544_FRAME_TAILROOM,
> PN544_HCI_LLC_MAX_PAYLOAD,
> dev_name(&client->dev));
>
> note that PN544_CMDS_HEADROOM is really only for skb allocated by HCI
> (or NFC Core), not by those allocated by shdlc, but we aggregate it all
> in a single parameter for simplicity.
Now, I see the point. Will update the patch.
Thanks,
/Waldek