On Tue, Mar 04, 2014 at 05:19:43PM -0700, Mark A. Greer wrote:
> 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.
That was my understanding as well, and I wanted to make sure that you
had that in mind too. It seems to be the case.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
