On Wed, Feb 26, 2014 at 02:53:09PM -0700, Mark A. Greer wrote:
> On Fri, Feb 21, 2014 at 01:50:59AM +0100, Samuel Ortiz wrote:
>
> > > +static int trf7970a_in_send_cmd(struct nfc_digital_dev *ddev,
> > > + struct sk_buff *skb, u16 timeout,
> > > + nfc_digital_cmd_complete_t cb, void *arg)
> > > +{
> > > + struct trf7970a *trf = nfc_digital_get_drvdata(ddev);
> > > + char *prefix;
> > > + unsigned int len;
> > > + int ret;
> > > +
> > > + dev_dbg(trf->dev, "New request - state: %d, timeout: %d ms, len: %d\n",
> > > + trf->state, timeout, skb->len);
> > > +
> > > + if (skb->len > TRF7970A_TX_MAX)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(&trf->lock);
> > > +
> > > + if ((trf->state != TRF7970A_ST_IDLE) &&
> > > + (trf->state != TRF7970A_ST_IDLE_RX_BLOCKED)) {
> > > + dev_err(trf->dev, "%s - Bogus state: %d\n", __func__,
> > > + trf->state);
> > > + ret = -EIO;
> > > + goto out_err;
> > > + }
> > > +
> > > + trf->rx_skb = nfc_alloc_recv_skb(TRF7970A_RX_SKB_ALLOC_SIZE,
> > > + GFP_KERNEL);
> > > + if (!trf->rx_skb) {
> > > + dev_dbg(trf->dev, "Can't alloc rx_skb\n");
> > > + ret = -ENOMEM;
> > > + goto out_err;
> > > + }
> > > +
> > > + if (trf->state == TRF7970A_ST_IDLE_RX_BLOCKED) {
> > > + ret = trf7970a_cmd(trf, TRF7970A_CMD_ENABLE_RX);
> > > + if (ret)
> > > + goto out_err;
> > > +
> > > + trf->state = TRF7970A_ST_IDLE;
> > > + }
> > > +
> > > + ret = trf7970a_per_cmd_config(trf, skb);
> > > + if (ret)
> > > + goto out_err;
> > > +
> > > + trf->ddev = ddev;
> > > + trf->tx_skb = skb;
> > As you're going to carry this guy around and may need it from e.g. your
> > threaded interrupt handler, shouldn't you take a reference (skb_get) on it ?
> > I'm concerned by the fact that you could see your tx_skb disappear from
> > abort_cmd and get an IRQ before your state is set to IDLE.
> > Hmm, I guess that's protected by the mutex and so when you get an abort
> > from the digital stack you reset the state to IDLE and no one should try
> > to touch tx_skb after you release the mutex. Is that what you had in
> > mind ?
>
> It is but taking a reference is a good idea. I'll add that.
I've changed my mind on this (hope that's okay).
The driver is in control of freeing that skb and won't free it until
there's an abort or the processing is complete. I believe that handle
the race with an abort correctly.
Mark
--
--
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
