Re: [PATCH] cdc-wdm: fix "out-of-sync" due to missing notifications

[Oliver Neukum <oneukum@suse.com>]

On Tue, 2016-05-17 at 21:24 +0200, Bjørn Mork wrote:
> Oliver Neukum <oneukum@suse.com> writes:
> 
> > On Fri, 2016-05-13 at 18:59 +0200, Bjørn Mork wrote:
> >> Bjørn Mork <bjorn@mork.no> writes:
> >> 
> >> > The driver enforces a strict one-to-one relationship between the
> >> > received RESPONSE_AVAILABLE notifications and messages read from
> >> > the device. At the same time, it will cancel the interrupt URB
> >> > when there is no client holding the character device open.
> >> 
> >> Never mind.  Forget it.
> >> 
> >> This patch breaks other devices again.  The immediate and unconditional
> >> reading make them barf. I guess it can be worked around by delaying the
> >> flushing until at least one notification is received, but I obviously
> >> have to test this theory thoroughly on all devices I have.
> >
> > Hi,
> >
> > I think the best approach would be to keep the interrupt URB always
> > active. I didn't do this to conserve bandwidth, but if it makes devices
> > work, it certainly would be the best option.
> 
> Yes, I considered that.  But this implies purging the device message
> queue without telling userspace that we did so.  At least with the
> current driver design, which is based on a single limited size
> buffer. If the device queues a number of unsolictied messages between
> two userspace requests, then we really want all those unsolicted
> messages delivered to the userspace program on the second request.

You might argue that if user space wants the data it should open the
device.

> And I do think the original bandwidth (and power) conservative approach
> is worth keeping too.  There is no point in waking up these devices
> unless there actually is an interested userspace application.

They can sleep just fine. I did not imply that runtime PM should
be disabled.

> FWIW, my initial analysis of the problem with the patch was too quick
> imprecise. The problem is simply the -EPIPE status we inevitably will
> hit when the queue is empty, as I should have anticipated. It will be
> returned to userspace translated to -EIO.  I am currently testing a
> version taking care of that, and it seems to behave well so far. I'll
> submit it as soon as I am absoltely sure that it works on all WDM, QMI
> and MBIM devices I have.  Might take some time, since I am running out
> of mini-PCIe and m.2 adapters..

That looks a bit risky. Firstly, if you get -EPIPE after a notification
it is an error and must be reported as such, so you need an additional
state. And what do you do after -EPIPE? Do you clean up the stall
or not? And the fun really starts if you get a notification while
you clean the stall.
And are you sure all devices can cope with an unsolicited request?

	Regards
		Oliver