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

[Johan Hovold <johan@kernel.org>]

On Wed, Sep 24, 2014 at 04:36:22PM +0300, Octavian Purdila wrote:
> On Wed, Sep 24, 2014 at 1:48 PM, Johan Hovold <johan@kernel.org> 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.

Just because you can reuse them doesn't mean it's a good idea. By
separating a generic disconnect solution from your custom slot
implementation we get something that is way easier to verify for
correctness and that could also be reused in other drivers.

> <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).

But the disconnect flag will also be set and makes it more obvious what
is going on.

[...]

> >> +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.

That's really not a big deal. See above.

Johan