Hi Lauro,
> >> > +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.
I agree. I'll send a patch for this.
> >> > +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'.
OK, I'll send a patch to protect those arguments.
We'll support full concurrency once LLCP is implemented.
Please note that this series of patches was already merged to wireless-next.
So, I'll base new patches on top of that.
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
