2011/9/21 Elias, Ilan <ilane@xxxxxx>:
>> > +
>> > +static int nci_start_poll(struct nfc_dev *nfc_dev, __u32 protocols)
>> > +{
>> > + struct nci_dev *ndev = nfc_get_drvdata(nfc_dev);
>> > + int rc;
>> > +
>> > + nfc_dbg("entry");
>> > +
>> > + if (test_bit(NCI_DISCOVERY, &ndev->flags)) {
>> > + nfc_err("unable to start poll, since poll
>> is already active");
>> > + return -EBUSY;
>> > + }
>> > +
>> > + if (test_bit(NCI_POLL_ACTIVE, &ndev->flags)) {
>> > + nfc_dbg("target already active, first
>> deactivate...");
>> > +
>> > + rc = nci_request(ndev, nci_rf_deactivate_req, 0,
>> > +
>> msecs_to_jiffies(NCI_RF_DEACTIVATE_TIMEOUT));
>> > + if (rc)
>> > + return -EBUSY;
>> > + }
>>
>> I don't think it is a good idea to implicitly deactivate a target. I
>> think you should just return EBUSY.
> The problem here is related to the NCI definition.
>
> The NCI defines that if only 1 target is discovered (the common case),
> it's automatically activated by the controller (i.e. in state POLL_ACTIVE).
> On the other hand, if more than 1 target is discovered, the controller
> sends RF_DISCOVER_NTF for each target, and the host need to select
> one (this case is much less common and I didn't implemented it yet).
>
> So, we're talking about the first case: once we start polling, and
> discover only 1 target, it's automatically activated.
> Now, if we call start_poll again, we have to deactivate the target before
> starting the poll again (as dictated by the NCI spec).
> I think this is the simplest solution to this issue: implicitly deactivate
> the target before starting to poll.
I don't see any problem in implicitly deactivating a target that was
implicitly activated. But if the target was activated by the user
(nfc_activate_target), it should not be deactivated.
>
> Another alternative is to always deactivate the target after it was
> automatically activated, and when activate_target is called, we'll have to
> poll again and hope that the same target will be activated again...
> I think this solution is much less preferred, since it's much more
> complicated and contradictes the NCI idea.
I agree that is not a good solution.
>
> Please advise.
>
>>
>>
>>
>> > +static int nci_data_exchange(struct nfc_dev *nfc_dev,
>> __u32 target_idx,
>> > + struct sk_buff *skb,
>> > +
>> data_exchange_cb_t cb,
>> > + void *cb_context)
>> > +{
>> > + struct nci_dev *ndev = nfc_get_drvdata(nfc_dev);
>> > +
>> > + nfc_dbg("entry, target_idx %d, len %d", target_idx,
>> skb->len);
>> > +
>> > + if (!ndev->target_active_prot) {
>> > + nfc_err("unable to exchange data, no active
>> target");
>> > + return -EINVAL;
>> > + }
>> > +
>> > + /* store cb and context to be used on receiving data */
>> > + ndev->data_exchange_cb = cb;
>> > + ndev->data_exchange_cb_context = cb_context;
>>
>> This is wrong. The data exchange can be called again before completing
>> the previous call. So, you shouldn't store the cb and cb_context in
>> the device.
> You're correct, of course.
> I see in your pn533 implementation that you also store the arguments
> (in the field pn533.cmd_complete_arg), but you do protect them with the
> 'cmd_lock' semaphore.
> If already in progress of data exchange transaction, you return EBUSY.
> I can also protect the 'cb' and 'cb_context' with such a semaphore.
> In fact, this is an implementation of a queue with max length of 1
> data exchange transaction.
>
You are right. The pn533 driver protects 'cb' and 'cb_context' but it
doesn't support full concurrency. Probably we will have to improve
this when the LLCP is implemented.
For your patch, I think it is enough to just protect the 'cb' and 'cb_context'.
Thanks,
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
