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

[Octavian Purdila]

On Wed, Sep 24, 2014 at 1:48 PM, Johan Hovold <johan@xxxxxxxxxx> wrote:
> On Fri, Sep 19, 2014 at 11:22:42PM +0300, Octavian Purdila wrote:

<snip>

>> + * dln2_dev.mod_rx_slots and then the echo header field to index the
>> + * slots field and find the receive context for a particular
>> + * request.
>> + */
>> +struct dln2_mod_rx_slots {
>> +     /* RX slots bitmap */
>> +     unsigned long bmap;
>> +
>> +     /* used to wait for a free RX slot */
>> +     wait_queue_head_t wq;
>> +
>> +     /* used to wait for an RX operation to complete */
>> +     struct dln2_rx_context slots[DLN2_MAX_RX_SLOTS];
>> +
>> +     /* device has been disconnected */
>> +     bool disconnected;
>
> This belongs in the dln2_dev struct.
>
> I think you're overcomplicating the disconnect handling by intertwining
> it with your slots.
>
> Add a lock, an active-transfer counter, a disconnected flag, and a wait
> queue to struct dln2_dev.
>

I agree that disconnected is better suited in dln2_dev.

However, I don't think that we need the active-transfer counter and a
new wait queue. We can simply use the existing waiting queues and the
implicit alloc_rx_slot()/free_rx_slot() calls to see if we are still
waiting for I/O.

<snip>

>> +
>> +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;
>> +     unsigned long flags;
>> +     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];
>> +
>
> Check the disconnected flag before incrementing the transfer count
> (while holding the device lock) here. Then decrement counter before
> returning and wake up an waiters if disconnected below.
>
> That is sufficient to implement wait-until-io-has-completed. Anything
> else you do below and in the helper functions is only to speed things
> up at disconnect (and can be done without locking the device).
>
>> +     rx_slot = alloc_rx_slot(rxs);
>> +     if (rx_slot < 0)
>> +             return rx_slot;
>> +
>> +     dev = &dln2->interface->dev;
>> +
>> +     ret = dln2_send_wait(dln2, handle, cmd, rx_slot, obuf, obuf_len);
>> +     if (ret < 0) {
>> +             free_rx_slot(dln2, rxs, rx_slot);
>
> goto out_free_rx_slot
>
>> +             dev_err(dev, "USB write failed: %d", ret);
>> +             return ret;
>> +     }
>> +
>> +     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;
>> +     }
>> +
>> +     spin_lock_irqsave(&rxs->lock, flags);
>> +
>> +     if (!rxc->urb) {
>
> Just check the disconnected flag directly here. Locking not needed (see
> below).
>

I think we need the check here for the case when we cancel the
completion and no response has been received yet. In that case rx->urb
will be NULL (even if we remove the rx->urb = NULL statement in
dln2_stop).

>> +             ret = -ECONNRESET;
>
> -ENODEV

OK.

>
>> +             spin_unlock_irqrestore(&rxs->lock, flags);
>> +             goto out_free_rx_slot;
>> +     }
>> +
>> +     /* if we got here we know that the response header has been checked */
>> +     rsp = rxc->urb->transfer_buffer;
>> +
>> +     spin_unlock_irqrestore(&rxs->lock, flags);
>> +
>> +
>> +static void dln2_stop(struct dln2_dev *dln2)
>> +{
>> +     int i, j;
>> +
>> +     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);
>> +
>> +             /* fail further alloc_rx_slot calls */
>> +             rxs->disconnected = true;
>> +
>> +             /* cancel all response waiters */
>> +             for (j = 0; j < DLN2_MAX_RX_SLOTS; j++) {
>> +                     struct dln2_rx_context *rxc = &rxs->slots[j];
>> +
>> +                     if (rxc->connected) {
>> +                             rxc->urb = NULL;
>
> Don't overload the meaning of urb. Check the disconnected flag were
> needed.
>
> Also what would prevent a racing completion handler from setting
> rxc->urb again when you release the lock below?
>

That should not be an issue, in that case _dln2_transfer will complete
successfully. But you are right, we don't need to set rxc->urb to NULL
here.

>> +                             complete(&rxc->done);
>> +                     }
>> +             }
>> +             spin_unlock_irqrestore(&rxs->lock, flags);
>> +
>> +             /* wait for callers to release all rx_slots */
>> +             wait_event(rxs->wq, dln2_all_rx_slots_free(rxs));
>> +     }
>> +}
>> +
>> +static void dln2_disconnect(struct usb_interface *interface)
>> +{
>> +     struct dln2_dev *dln2 = usb_get_intfdata(interface);
>> +
>
> First set the disconnected flag directly here to prevent any new
> transfers from starting.
>
>> +     dln2_stop(dln2);
>
> Then do all the completions (to speed things up somewhat).
>
> Then wait for the transfer counter to reach 0.
>
>> +     mfd_remove_devices(&interface->dev);
>> +     dln2_free(dln2);
>> +}
>> +

As I mentioned above, I don't think we need the transfer counter, we
can rely on the slots bitmap. Yes, a counter is simpler but it also
requires adding a new waiting queue and a new lock.

<snip>

>> +     ret = dln2_hw_init(dln2);
>> +     if (ret < 0) {
>> +             dev_err(dev, "failed to initialize hardware\n");
>> +             goto out_cleanup;
>> +     }
>> +
>> +     ret = mfd_add_devices(dev, -1, dln2_devs, ARRAY_SIZE(dln2_devs),
>
> Use
>         busnum << 8 | devnum
>
> as id for now (as mentioned earlier).
>

Right, forgot about this, sorry.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/