Hi Gustavo,
> > > > -static void nci_rf_activate_ntf_packet(struct nci_dev *ndev,
> > > > - struct sk_buff *skb)
> > > > +static void nci_rf_intf_activated_ntf_packet(struct
> nci_dev *ndev,
> > > > + struct
> sk_buff *skb)
> > > > {
> > > > - struct nci_rf_activate_ntf ntf;
> > > > + struct nci_rf_intf_activated_ntf ntf;
> > > > __u8 *data = skb->data;
> > > > - int rc = -1;
> > > > + int err = 0;
> > > >
> > > > clear_bit(NCI_DISCOVERY, &ndev->flags);
> > > > set_bit(NCI_POLL_ACTIVE, &ndev->flags);
> > > >
> > > > - ntf.target_handle = *data++;
> > > > + ntf.rf_discovery_id = *data++;
> > > > + ntf.rf_interface_type = *data++;
> > > > ntf.rf_protocol = *data++;
> > > > - ntf.rf_tech_and_mode = *data++;
> > > > + ntf.activation_rf_tech_and_mode = *data++;
> > > > ntf.rf_tech_specific_params_len = *data++;
> > > >
> > > > - nfc_dbg("target_handle %d, rf_protocol 0x%x,
> > > rf_tech_and_mode 0x%x, rf_tech_specific_params_len %d",
> > > > - ntf.target_handle,
> > > > - ntf.rf_protocol,
> > > > - ntf.rf_tech_and_mode,
> > > > + nfc_dbg("rf_discovery_id %d", ntf.rf_discovery_id);
> > > > + nfc_dbg("rf_interface_type 0x%x",
> ntf.rf_interface_type);
> > > > + nfc_dbg("rf_protocol 0x%x", ntf.rf_protocol);
> > > > + nfc_dbg("activation_rf_tech_and_mode 0x%x",
> > > > + ntf.activation_rf_tech_and_mode);
> > > > + nfc_dbg("rf_tech_specific_params_len %d",
> > > > ntf.rf_tech_specific_params_len);
> > > >
> > > > - switch (ntf.rf_tech_and_mode) {
> > > > - case NCI_NFC_A_PASSIVE_POLL_MODE:
> > > > - rc =
> nci_rf_activate_nfca_passive_poll(ndev, &ntf,
> > > > - data);
> > > > - break;
> > > > + if (ntf.rf_tech_specific_params_len > 0) {
> > > > + switch (ntf.activation_rf_tech_and_mode) {
> > > > + case NCI_NFC_A_PASSIVE_POLL_MODE:
> > > > + data =
> > > nci_extract_rf_params_nfca_passive_poll(ndev,
> > > > + &ntf, data);
> > > > + break;
> > > > +
> > > > + default:
> > > > + nfc_err("unsupported
> > > activation_rf_tech_and_mode 0x%x",
> > > > +
> ntf.activation_rf_tech_and_mode);
> > > > + return;
> > > > + }
> > > > + }
> > > >
> > > > - default:
> > > > - nfc_err("unsupported rf_tech_and_mode 0x%x",
> > > > - ntf.rf_tech_and_mode);
> > > > - return;
> > > > + ntf.data_exchange_rf_tech_and_mode = *data++;
> > > > + ntf.data_exchange_tx_bit_rate = *data++;
> > > > + ntf.data_exchange_rx_bit_rate = *data++;
> > > > + ntf.activation_params_len = *data++;
> > > > +
> > > > + nfc_dbg("data_exchange_rf_tech_and_mode 0x%x",
> > > > + ntf.data_exchange_rf_tech_and_mode);
> > > > + nfc_dbg("data_exchange_tx_bit_rate 0x%x",
> > > > + ntf.data_exchange_tx_bit_rate);
> > > > + nfc_dbg("data_exchange_rx_bit_rate 0x%x",
> > > > + ntf.data_exchange_rx_bit_rate);
> > > > + nfc_dbg("activation_params_len %d",
> > > > + ntf.activation_params_len);
> > > > +
> > > > + if (ntf.activation_params_len > 0) {
> > >
> > > Seems to me that ntf.activation_params_len > 0 can be a check
> > > in the beginning
> > > of this function.
> > Actually, each param is read in order (as you can see the
> pointer data is advanced on each param read).
> > The order is important, since it's clearer and some
> parameters are depend on the previous ones (e.g. we must read
> activation_rf_tech_and_mode before we read the activation params).
> > So, activation_params_len is read when we arrive to its
> location in the data stream.
> > In any case, I don't see the benefit in checking the
> condition in the beginning of the function (since I still
> need to read the other params).
>
> Sure, I didn't realize this in my quick look to this code.
> But this remind me
> another thing. Why are you reading your data like this, each
> member at a time?
> Can't you do something like this:
>
> struct ntf *ntf = skb->data;
>
> Then if you need to copy them to another places, or do memcpy
> or pick the ones
> you want by assign them to other variables.
Wherever I can read them into a pointer like you suggest, I do it (as you can see with all other packets).
In this case, the packet is complicated, since it includes lots of different sub-structs depending on the previous fields.
Also, some fields have variable length (so I need to keep track of the current position all the time).
In addition, some fields are bigger than 1 byte and need to be converted from little-endian to local CPU.
The reason for the complication lies in the nature of NFC, where we're expected to support multiple technologies and protocols (represented in 1 single packet).
For example, we can receive activation from tech A tags or B, F etc...each of them have different representation.
For simplicity of the code and the parsing, we represent all the information of a activation ntf in a single struct.
Of course, this struct does not represent the true protocol format (as explained above).
>
> Gustavo
>
Thanks & BR,
Ilan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
