Hi Johannes,
2011/6/3 Johannes Berg <johannes@xxxxxxxxxxxxxxxx>:
> On Thu, 2011-06-02 at 18:46 -0300, Lauro Ramos Venancio wrote:
>
>> + * @NFC_ATTR_TARGETS: array of targets (see enum nfc_target_attr)
>
>
>> +static int nfc_genl_msg_put_target(struct sk_buff *msg,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct nfc_target *target)
>> +{
>> + Â Â NLA_PUT_U32(msg, NFC_TARGET_ATTR_TARGET_INDEX, target->idx);
>> + Â Â NLA_PUT_U32(msg, NFC_TARGET_ATTR_SUPPORTED_PROTOCOLS,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â target->supported_protocols);
>> + Â Â NLA_PUT_U8(msg, NFC_TARGET_ATTR_SENS_RES, target->sens_res);
>> + Â Â NLA_PUT_U8(msg, NFC_TARGET_ATTR_SEL_RES, target->sel_res);
>> +
>> + Â Â return 0;
>> +
>> +nla_put_failure:
>> + Â Â return -EMSGSIZE;
>> +}
>> +
>> +int nfc_genl_targets_found(struct nfc_dev *dev, struct nfc_target *targets,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â int ntargets)
>> +{
>> + Â Â struct sk_buff *msg;
>> + Â Â void *hdr;
>> + Â Â struct nlattr *targets_attr;
>> + Â Â int i;
>> +
>> + Â Â pr_debug("%s\n", __func__);
>> +
>> + Â Â dev->genl_data.poll_req_pid = 0;
>> +
>> + Â Â msg = nlmsg_new(NLMSG_GOODSIZE, GFP_ATOMIC);
>> + Â Â if (!msg)
>> + Â Â Â Â Â Â return -ENOMEM;
>> +
>> + Â Â hdr = genlmsg_put(msg, 0, 0, &nfc_genl_family, 0,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â NFC_EVENT_TARGETS_FOUND);
>> + Â Â if (!hdr)
>> + Â Â Â Â Â Â goto free_msg;
>> +
>> + Â Â NLA_PUT_U32(msg, NFC_ATTR_DEVICE_INDEX, dev->idx);
>> +
>> + Â Â targets_attr = nla_nest_start(msg, NFC_ATTR_TARGETS);
>> +
>> + Â Â for (i = 0; i < ntargets; i++) {
>> + Â Â Â Â Â Â struct nlattr *target = nla_nest_start(msg, i);
>> +
>> + Â Â Â Â Â Â if (nfc_genl_msg_put_target(msg, &targets[i]))
>> + Â Â Â Â Â Â Â Â Â Â goto nla_put_failure;
>> +
>> + Â Â Â Â Â Â nla_nest_end(msg, target);
>> + Â Â }
>> +
>> + Â Â nla_nest_end(msg, targets_attr);
>> + Â Â genlmsg_end(msg, hdr);
>> +
>> + Â Â return genlmsg_multicast(msg, 0, nfc_genl_event_mcgrp.id, GFP_ATOMIC);
>
> This is almost certainly not a good idea.
>
> Eventually, you will want to add more information about a target. I know
> NFC is a pretty limited protocol, but I wouldn't want to make this
> harder in the future than it must be. The way you're doing it, you're
> currently limiting yourself to about 100 targets. Each new target
> attribute that you might add in the future will reduce that number
> significantly.
Actually, we don't expect more than a couple of targets because of NFC
short range.
I agree that this can be a problem if we start supporting vicinity cards.
> IMHO, the better way to structure this would be to create an event that
> contains no information, and then allow a dump of the targets (with a
> generation counter). That way, there are no such artificial limits due
> to message sizes.
I agree that this is a better solution. But I think we don't need a
generation counter because only a new polling operation (start_poll
call) can change the targets list (i.e. there is no passive polling).
>> +static int nfc_genl_dump_devices(struct sk_buff *skb,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct netlink_callback *cb)
>
> You should have a generation counter here so that applications getting a
> dump can know whether their dump was a complete and consistent snapshot.
> Otherwise, if devices are added or removed during the dump applications
> will not be able to know that their dump wasn't right.
>
We don't need a generation counter here because we have the events
NFC_EVENT_DEVICE_ADDED and NFC_EVENT_DEVICE_REMOVED. So, it is
possible to keep the device list consistency listening for these
events.
Lauro
--
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
