Re: [PATCH v6 1/4] mfd: add support for Diolan DLN-2 devices

[Octavian Purdila <octavian.purdila@xxxxxxxxx>]

On Wed, Oct 8, 2014 at 12:23 PM, Johan Hovold <johan@xxxxxxxxxx> wrote:
> On Thu, Sep 25, 2014 at 07:07:31PM +0300, Octavian Purdila wrote:
>
>> +static void dln2_rx_transfer(struct dln2_dev *dln2, struct urb *urb,
>> +                          u16 handle, u16 rx_slot)
>> +{
>> +     struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
>> +     struct dln2_rx_context *rxc;
>> +     struct device *dev = &dln2->interface->dev;
>> +
>> +     spin_lock(&rxs->lock);
>
> You must use spin_lock_irqsave here as you call it from the completion
> handler.

Why? AFAICS the completion handler gets called from the HCD irq handler:

[    2.503945] Call Trace:
[    2.503945]  <IRQ>  [<ffffffff81a73d14>] dump_stack+0x4e/0x7a
[    2.503945]  [<ffffffff81639fcb>] dln2_rx+0x1eb/0x2d0
[    2.503945]  [<ffffffff81742202>] __usb_hcd_giveback_urb+0x72/0x120
[    2.503945]  [<ffffffff817423cf>] usb_hcd_giveback_urb+0x3f/0x120
[    2.503945]  [<ffffffff81786ffa>] uhci_giveback_urb+0xaa/0x290
[    2.503945]  [<ffffffff811d36d7>] ? dma_pool_free+0xa7/0xd0
[    2.503945]  [<ffffffff81788fe3>] uhci_scan_schedule+0x493/0xb20
[    2.503945]  [<ffffffff81789c9e>] uhci_irq+0x9e/0x180
[    2.503945]  [<ffffffff81741546>] usb_hcd_irq+0x26/0x40
[    2.503945]  [<ffffffff8110e46e>] handle_irq_event_percpu+0x3e/0x1e0
[    2.503945]  [<ffffffff8110e64d>] handle_irq_event+0x3d/0x60
[    2.503945]  [<ffffffff811117f9>] handle_fasteoi_irq+0x99/0x160
[    2.503945]  [<ffffffff810492c2>] handle_irq+0x102/0x190
[    2.503945]  [<ffffffff810e493a>] ? atomic_notifier_call_chain+0x3a/0x50
[    2.503945]  [<ffffffff81a7fbcb>] do_IRQ+0x5b/0x100
[    2.503945]  [<ffffffff81a7dd6f>] common_interrupt+0x6f/0x6f
[    2.503945]  <EOI>  [<ffffffff810512ed>] ? default_idle+0x1d/0x100

>
>> +
>> +     rxc = &rxs->slots[rx_slot];
>> +
>> +     if (rxc->in_use && !rxc->urb) {
>> +             rxc->urb = urb;
>> +             complete(&rxc->done);
>> +             /* URB will be resubmitted in free_rx_slot */
>> +     } else {
>> +             dev_warn(dev, "bad/late response %d/%d\n", handle, rx_slot);
>> +             dln2_submit_urb(dln2, urb, GFP_ATOMIC);
>> +     }
>> +
>> +     spin_unlock(&rxs->lock);
>> +}
>
>> +static int _dln2_transfer(struct dln2_dev *dln2, u16 handle, u16 cmd,
>> +                       const void *obuf, unsigned obuf_len,
>> +                       void *ibuf, unsigned *ibuf_len)
>> +{
>> +     int ret = 0;
>> +     u16 result;
>> +     int rx_slot;
>> +     struct dln2_response *rsp;
>> +     struct dln2_rx_context *rxc;
>> +     struct device *dev;
>> +     const int timeout = DLN2_USB_TIMEOUT * HZ / 1000;
>> +     struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[handle];
>> +
>> +     spin_lock(&dln2->disconnect_lock);
>
> How did you test this version? You never initialise disconnect_lock so
> lockdep complains loudly when calling _dln2_transfer during probe.
>

Oops, missed that as lockdep was not enable in the lastest test setup.

>> +     if (!dln2->disconnect)
>> +             dln2->active_transfers++;
>> +     else
>> +             ret = -ENODEV;
>> +     spin_unlock(&dln2->disconnect_lock);
>> +
>> +     if (ret)
>> +             return ret;
>> +
>> +     rx_slot = alloc_rx_slot(dln2, handle);
>> +     if (rx_slot < 0) {
>> +             ret = rx_slot;
>> +             goto out_decr;
>> +     }
>> +
>> +     dev = &dln2->interface->dev;
>> +
>> +     ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
>> +     if (ret < 0) {
>> +             dev_err(dev, "USB write failed: %d\n", ret);
>> +             goto out_free_rx_slot;
>> +     }
>> +
>> +     rxc = &rxs->slots[rx_slot];
>> +
>> +     ret = wait_for_completion_interruptible_timeout(&rxc->done, timeout);
>> +     if (ret <= 0) {
>> +             if (!ret)
>> +                     ret = -ETIMEDOUT;
>> +             goto out_free_rx_slot;
>> +     }
>> +
>> +     if (!rxc->urb) {
>> +             ret = -ENODEV;
>> +             goto out_free_rx_slot;
>> +     }
>> +
>> +     /* if we got here we know that the response header has been checked */
>> +     rsp = rxc->urb->transfer_buffer;
>> +
>> +     if (rsp->hdr.size < sizeof(*rsp)) {
>> +             ret = -EPROTO;
>> +             goto out_free_rx_slot;
>> +     }
>> +
>> +     result = le16_to_cpu(rsp->result);
>> +     if (result) {
>> +             dev_dbg(dev, "%d received response with error %d\n",
>> +                     handle, result);
>> +             ret = -EREMOTEIO;
>> +             goto out_free_rx_slot;
>> +     }
>> +
>> +     if (!ibuf) {
>> +             ret = 0;
>> +             goto out_free_rx_slot;
>> +     }
>> +
>> +     if (*ibuf_len > rsp->hdr.size - sizeof(*rsp))
>> +             *ibuf_len = rsp->hdr.size - sizeof(*rsp);
>> +
>> +     memcpy(ibuf, rsp + 1, *ibuf_len);
>> +
>> +out_free_rx_slot:
>> +     free_rx_slot(dln2, handle, rx_slot);
>> +out_decr:
>> +     spin_lock(&dln2->disconnect_lock);
>> +     dln2->active_transfers--;
>> +     spin_unlock(&dln2->disconnect_lock);
>> +     if (dln2->disconnect)
>> +             wake_up(&dln2->disconnect_wq);
>> +
>> +     return ret;
>> +}
>
>> +static void dln2_disconnect(struct usb_interface *interface)
>> +{
>> +     struct dln2_dev *dln2 = usb_get_intfdata(interface);
>> +     int i, j;
>> +
>> +     /* don't allow starting new transfers */
>> +     spin_lock(&dln2->disconnect_lock);
>> +     dln2->disconnect = true;
>> +     spin_unlock(&dln2->disconnect_lock);
>> +
>> +     /* cancel in progress transfers */
>> +     for (i = 0; i < DLN2_HANDLES; i++) {
>> +             struct dln2_mod_rx_slots *rxs = &dln2->mod_rx_slots[i];
>> +             unsigned long flags;
>> +
>> +             spin_lock_irqsave(&rxs->lock, flags);
>
> Just stick to spin_lock in this function.
>

AFAICS disconnect is called from a kernel thread. Are there guarantees
that we can't get a call to the completion routine while we are
running it?
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html