Re: [PATCH v2] tag: Implement readout of tag UID via DBus interface
by Frieder Schrempf
Hi Fabian,
On 16.03.21 19:49, Gottstein, Fabian wrote:
> Hi Frieder,
>
> thanks for the patch.
thanks for your feedback.
>
> Could you please also consider the following situation:
> In the case of a NFC Tag Type 1, the identifier is delivered via the RID command (see NFC Digital Protocol). Thus, the Tag's nfcid property is updated in a later step.
> To inform the neard users, a property changed signal has to be emitted when nfcid has changed (in near_tag_set_nfcid). Also, a exists() handler for the new DBus property should be implemented.
I'm new to NFC and D-Bus, so I don't know much about what use-cases and
requirements there are.
Your request sounds reasonable and I think I have a rough understanding
of what is probably needed to implement this. Still to actually do this
I need to look at the specifications and the code more closely and I
don't know if/when I will find time to do this.
Also I don't have any hardware to test this with NFC type 1 tags.
>
> Another thing regarding building the response message:
> The following code snippet could simplify and improve the readability of the usage of the dbus message builder:
>
> dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE_AS_STRING, &entry);
> dbus_message_iter_append_fixed_array(&entry, DBUS_TYPE_BYTE, &uid, len);
> dbus_message_iter_close_container(iter, &entry);
>
Thanks for the improved code, I will use this instead.
Frieder
>
>
> -----Original Message-----
> From: Schrempf Frieder <frieder.schrempf(a)kontron.de>
> Sent: Dienstag, 16. März 2021 12:22
> To: Samuel Ortiz <sameo(a)linux.intel.com>; linux-nfc(a)lists.01.org
> Cc: Frieder Schrempf <frieder.schrempf(a)kontron.de>
> Subject: [linux-nfc] [PATCH v2] tag: Implement readout of tag UID via DBus interface
>
> Caution: This e-mail originated from outside of Philips, be careful for phishing.
>
>
> From: Frieder Schrempf <frieder.schrempf(a)kontron.de>
>
> NFC tags usually provide an unique identifier. Neard already checks if one of the two types of identifiers is available, reads them from tags and stores them in near_tag.nfcid or near_tag.iso15693_uid respectively.
>
> Though currently it is not possible for any client application to get this information via the D-Bus interface as no property for the UID is implemented.
>
> This adds a 'Uid' property to the D-Bus interface for tags, which exposes the UID of the tag as byte array. If nfcid is available this is returned as UID, otherwise if iso15693_uid is available this is returned. If no UID is available, no 'Uid' property is exposed.
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf(a)kontron.de>
> ---
> Changes in v2:
> * Add whitespaces after 'for' statements
> * Add more details to the commit message
> ---
> src/tag.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/src/tag.c b/src/tag.c
> index 9eba4ee..d530893 100644
> --- a/src/tag.c
> +++ b/src/tag.c
> @@ -53,6 +53,7 @@ struct near_tag {
> uint8_t nfcid_len;
>
> uint8_t iso15693_dsfid;
> + uint8_t iso15693_uid_len;
> uint8_t iso15693_uid[NFC_MAX_ISO15693_UID_LEN];
>
> size_t data_length;
> @@ -168,6 +169,29 @@ static const char *type_string(struct near_tag *tag)
> return type;
> }
>
> +static const uint8_t uid_array(struct near_tag *tag, uint8_t **uid) {
> + if (tag->nfcid_len) {
> + DBG("NFCID: ");
> + for (int i = 0; i < tag->nfcid_len; i++)
> + DBG("%x", tag->nfcid[i]);
> +
> + *uid = tag->nfcid;
> +
> + return tag->nfcid_len;
> + } else if (tag->iso15693_uid_len) {
> + DBG("ISO-UID: ");
> + for (int i = 0; i < tag->iso15693_uid_len; i++)
> + DBG("%x", tag->iso15693_uid[i]);
> +
> + *uid = tag->iso15693_uid;
> +
> + return tag->iso15693_uid_len;
> + }
> +
> + return 0;
> +}
> +
> static const char *protocol_string(struct near_tag *tag) {
> const char *protocol;
> @@ -219,6 +243,30 @@ static gboolean property_get_type(const GDBusPropertyTable *property,
> return TRUE;
> }
>
> +static gboolean property_get_uid(const GDBusPropertyTable *property,
> + DBusMessageIter *iter, void
> +*user_data) {
> + struct near_tag *tag = user_data;
> + DBusMessageIter entry;
> + uint8_t *uid;
> + uint8_t len;
> +
> + len = uid_array(tag, &uid);
> + if (!uid || !len)
> + return FALSE;
> +
> + dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
> + "{y}", &entry);
> +
> + for (int i = 0; i < len; i++)
> + dbus_message_iter_append_basic(&entry, DBUS_TYPE_BYTE,
> + (void *)&uid[i]);
> +
> + dbus_message_iter_close_container(iter, &entry);
> +
> + return TRUE;
> +}
> +
> static gboolean property_get_protocol(const GDBusPropertyTable *property,
> DBusMessageIter *iter, void *user_data) { @@ -526,6 +574,7 @@ static const GDBusPropertyTable tag_properties[] = {
> { "Protocol", "s", property_get_protocol },
> { "ReadOnly", "b", property_get_readonly },
> { "Adapter", "o", property_get_adapter },
> + { "Uid", "ay", property_get_uid },
>
> { }
> };
> @@ -671,8 +720,10 @@ static int tag_initialize(struct near_tag *tag,
> if (nfcid_len && nfcid_len <= NFC_MAX_NFCID1_LEN) {
> tag->nfcid_len = nfcid_len;
> memcpy(tag->nfcid, nfcid, nfcid_len);
> - } else if (iso15693_uid_len) {
> + } else if (iso15693_uid_len &&
> + iso15693_uid_len <= NFC_MAX_ISO15693_UID_LEN) {
> tag->iso15693_dsfid = iso15693_dsfid;
> + tag->iso15693_uid_len = iso15693_uid_len;
> memcpy(tag->iso15693_uid, iso15693_uid, iso15693_uid_len);
> }
>
> @@ -837,11 +888,11 @@ uint8_t *near_tag_get_iso15693_uid(uint32_t adapter_idx, uint32_t target_idx)
> if (!tag)
> goto fail;
>
> - iso15693_uid = g_try_malloc0(NFC_MAX_ISO15693_UID_LEN);
> + iso15693_uid = g_try_malloc0(tag->iso15693_uid_len);
> if (!iso15693_uid)
> goto fail;
>
> - memcpy(iso15693_uid, tag->iso15693_uid, NFC_MAX_ISO15693_UID_LEN);
> + memcpy(iso15693_uid, tag->iso15693_uid, tag->iso15693_uid_len);
>
> return iso15693_uid;
>
> --
> 2.25.1
> _______________________________________________
> Linux-nfc mailing list -- linux-nfc(a)lists.01.org To unsubscribe send an email to linux-nfc-leave(a)lists.01.org %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>
> ________________________________
> The information contained in this message may be confidential and legally protected under applicable law. The message is intended solely for the addressee(s). If you are not the intended recipient, you are hereby notified that any use, forwarding, dissemination, or reproduction of this message is strictly prohibited and may be unlawful. If you are not the intended recipient, please contact the sender by return e-mail and destroy all copies of the original message.
>
10 months, 1 week
[PATCH 1/2] MAINTAINERS: nfc: add Krzysztof Kozlowski as maintainer
by Krzysztof Kozlowski
The NFC subsystem is orphaned. I am happy to spend some cycles to
review the patches, send pull requests and in general keep the NFC
subsystem running.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)canonical.com>
---
I admit I don't have big experience in NFC part but this will be nice
opportunity to learn something new. I am already maintainer of few
other parts: memory controller drivers, Samsung ARM/ARM64 SoC and some
drviers. I have a kernel.org account and my GPG key is:
https://git.kernel.org/pub/scm/docs/kernel/pgpkeys.git/tree/keys/1B93437D...
Best regards,
Krzysztof
---
MAINTAINERS | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index cc81667e8bab..adc6cbe29f78 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12899,8 +12899,9 @@ F: include/uapi/linux/nexthop.h
F: net/ipv4/nexthop.c
NFC SUBSYSTEM
+M: Krzysztof Kozlowski <krzysztof.kozlowski(a)canonical.com>
L: netdev(a)vger.kernel.org
-S: Orphan
+S: Maintained
F: Documentation/devicetree/bindings/net/nfc/
F: drivers/nfc/
F: include/linux/platform_data/nfcmrvl.h
--
2.25.1
11 months, 2 weeks
[PATCH v2 1/2] nfc: mrvl: remove useless "continue" at end of loop
by Krzysztof Kozlowski
The "continue" statement at the end of a for loop does not have an
effect. Entire loop contents can be slightly simplified to increase
code readability. No functional change.
Suggested-by: Joe Perches <joe(a)perches.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)canonical.com>
---
Changes since v1:
1. Make it if-else-if as Joe suggested.
---
drivers/nfc/nfcmrvl/usb.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
index bcd563cb556c..6fec20abfd1e 100644
--- a/drivers/nfc/nfcmrvl/usb.c
+++ b/drivers/nfc/nfcmrvl/usb.c
@@ -319,13 +319,9 @@ static int nfcmrvl_probe(struct usb_interface *intf,
if (!drv_data->bulk_tx_ep &&
usb_endpoint_is_bulk_out(ep_desc)) {
drv_data->bulk_tx_ep = ep_desc;
- continue;
- }
-
- if (!drv_data->bulk_rx_ep &&
- usb_endpoint_is_bulk_in(ep_desc)) {
+ } else if (!drv_data->bulk_rx_ep &&
+ usb_endpoint_is_bulk_in(ep_desc)) {
drv_data->bulk_rx_ep = ep_desc;
- continue;
}
}
--
2.27.0
1 year
Re: [PATCH] nfc: mrvl: remove useless "continue" at end of loop
by Krzysztof Kozlowski
On 01/06/2021 18:30, Joe Perches wrote:
> On Tue, 2021-06-01 at 18:07 +0200, Krzysztof Kozlowski wrote:
>> The "continue" statement at the end of a for loop does not have an
>> effect.
> []
>> diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
> []
>> @@ -325,7 +325,6 @@ static int nfcmrvl_probe(struct usb_interface *intf,
>> if (!drv_data->bulk_rx_ep &&
>> usb_endpoint_is_bulk_in(ep_desc)) {
>> drv_data->bulk_rx_ep = ep_desc;
>> - continue;
>> }
>> }
>
> I think this code would be clearer with an if/else instead of
> multiple continues.
Makes sense. I'll send a v2.
Best regards,
Krzysztof
1 year
[PATCH] nfc: mrvl: remove useless "continue" at end of loop
by Krzysztof Kozlowski
The "continue" statement at the end of a for loop does not have an
effect.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)canonical.com>
---
drivers/nfc/nfcmrvl/usb.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/nfc/nfcmrvl/usb.c b/drivers/nfc/nfcmrvl/usb.c
index bcd563cb556c..433bdc37ba91 100644
--- a/drivers/nfc/nfcmrvl/usb.c
+++ b/drivers/nfc/nfcmrvl/usb.c
@@ -325,7 +325,6 @@ static int nfcmrvl_probe(struct usb_interface *intf,
if (!drv_data->bulk_rx_ep &&
usb_endpoint_is_bulk_in(ep_desc)) {
drv_data->bulk_rx_ep = ep_desc;
- continue;
}
}
--
2.27.0
1 year
Re: [PATCH] NFC: microread: Pass err variable to async_cb()
by Krzysztof Kozlowski
On 01/06/2021 15:00, Nigel Christian wrote:
> In the case MICROREAD_CB_TYPE_READER_ALL clang reports a dead
> code warning. The error code is being directly passed to
> async_cb(). Fix this by passing the err variable, which is also
> done in another path.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Nigel Christian <nigel.l.christian(a)gmail.com>
> ---
> drivers/nfc/microread/microread.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nfc/microread/microread.c b/drivers/nfc/microread/microread.c
> index 8d3988457c58..130b0f554016 100644
> --- a/drivers/nfc/microread/microread.c
> +++ b/drivers/nfc/microread/microread.c
> @@ -367,7 +367,7 @@ static void microread_im_transceive_cb(void *context, struct sk_buff *skb,
> err = -EPROTO;
Remove this line instead, please. The err is argument passed by value so
assigning it within a function is ugly.
> kfree_skb(skb);
> info->async_cb(info->async_cb_context, NULL,
> - -EPROTO);
> + err);
> return;
> }
>
>
Best regards,
Krzysztof
1 year
Re: [PATCH] NFC: nci: Remove redundant assignment to len
by Krzysztof Kozlowski
On 01/06/2021 11:49, Yang Li wrote:
> Variable 'len' is set to conn_info->max_pkt_payload_len but this
> value is never read as it is overwritten with a new value later on,
> hence it is a redundant assignment and can be removed.
>
> Clean up the following clang-analyzer warning:
>
> net/nfc/nci/hci.c:164:3: warning: Value stored to 'len' is never read
> [clang-analyzer-deadcode.DeadStores]
>
> Reported-by: Abaci Robot <abaci(a)linux.alibaba.com>
> Signed-off-by: Yang Li <yang.lee(a)linux.alibaba.com>
> ---
> net/nfc/nci/hci.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/net/nfc/nci/hci.c b/net/nfc/nci/hci.c
> index 9686514..d6732e5 100644
> --- a/net/nfc/nci/hci.c
> +++ b/net/nfc/nci/hci.c
> @@ -161,8 +161,6 @@ static int nci_hci_send_data(struct nci_dev *ndev, u8 pipe,
> *(u8 *)skb_push(skb, 1) = data_type;
>
> do {
> - len = conn_info->max_pkt_payload_len;
> -
> /* If last packet add NCI_HFP_NO_CHAINING */
> if (i + conn_info->max_pkt_payload_len -
> (skb->len + 1) >= data_len) {
>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)canonical.com>
Best regards,
Krzysztof
1 year