At Thu, 30 Oct 2003 13:33:37 -0800,
Steve deRosier wrote:
>
> Takashi,
>
> Thanks for your response. I've addressed your issues below. Let's
> discuss this and if necessary I'll modify my fix.
>
> >>1. Move all checks of buffer overflow and such to the
> >>actual buffer write and read routines. This makes
> >>the buffer routines more robust and encaspulates buffer
> >>opperations better.
> >
> >
> > the check in the caller may be still needed for the multi-bytes write
> > (e.g. for switching the port). otherwise, the port-switch command
> > can be mixed and messed up when the device is accessed concurrently.
> >
>
> I thought about this, but decided it's not a large issue since it is
> mitigated by the natural operation of the driver.
> For normal opperation: As this is called with interupts off, the
> process can't be interupted. The function doesn't return until after
> we've drained the snd_rawmidi_transmit(), and if we do decide to insert
> a "f5..." msg, we do the whole msg before we can return. So, no issue
> durring normal opperation. So concurrent access doesn't apear to be a
> large issue.
yes, normally it's ok. the problem would be only the full-buffer
state.
> If buffer gets full: Yes, it is possible for an "f5..." msg to get
> interupted. BUT, I don't see this as a large problem. On return to
> normal opperation (ie. the buffer begins draining again), the proper
> port-switch command will be written in correctly when the port gets
> switched, or in the next three seconds, whichever comes first. Perhaps
> some data gets sent to the wrong port, but it autocorrects very quickly.
> While this condition technically is possible, I haven't seen it happen
> in practice, and even if it does it would correct itself quickly. For
> another mittigating factor, please see my answer to #2 below.
>
> If you're still concerned about this, I have an alternate fix that would
> keep a check, but not interfeare with opperation.
>
> >
> >>2. Always try to send data to the buffers. Don't interupt
> >>normal opperation of the algoritim because buffers are full.
> >>This was what was actually causing the data to be left in
> >>rawmidi and thus causing it to lock up. This is helped
> >>by moving the buffer size checks into the routines.
> >
> >
> > this is the questionary behavior.
> > the fact that the local buffer is full means that the transfer doesn't
> > work. so, it is quite correct behavior that rawmidi blocks the
> > further write (in blocking mode), i think. and, it should return
> > -EAGAIN in non-blocking mode.
> > or, at least, we can make the behavior selectable: abandon or block.
> >
>
> Well, the driver already was trying to opperate in the mode where it was
> abandoning bytes, it was just doing a bad job of it. All I did was fix
> it so it actually abandoned all bytes that didn't fit in the buffers.
>
> Before it was grabbing a byte from snd_rawmidi_transmit() and if no room
> was in the buffer it would break out of the while(1) loop. So,
> basically it would abandon the first byte on its execution, but leave
> some data there for later. This caused a problem with rawmidi's buffers
> seeming to back up and eventually hanging up the driver (recoverable
> only via reopening the device).
basically, the data should NOT be abandoned as long as it can be
held. the current code is not good from this perspective.
it should be something like:
while (snd_rawmidi_transmit_peek(substream, &byte, 1) == 1) {
if (! writable())
break;
write_byte(byte);
snd_rawmidi_transmit_ack(substream, 1);
}
> The program (call it pdr) we have using this device operates in blocking
> mode. So, if things worked properly, pdr would attempt to write the
> midi msg, and the snd_seq_event_output() just wouldn't return until it
> was able to send the data. That would be fine. BUT, if we physically
> disconnect the serial cable from the device, after awhile, the buffer in
> serial-u16550.c fills up, then one byte is retrieved from
> snd_rawmidi_transmit() (and then discarded by the driver) for every
> three bytes put into the rawmidi buffers, these buffers fill up and
> eventually cause the lockup of the driver, all while never reporting a
> problem back to pdr (snd_seq_event_output() seems not to block even
> though we're in blocking mode).
in this case, the operation is blocked because it's really blocked.
sure, it doesn't report errors because it's just the blocking
behavior. the driver doesn't detect the disconnection, and it simply
waits until the next transfer is available.
(snip)
> However, if we want different behavior, let me know how to do this. I
> was able to fix the problem here, without digging further into the
> rawmidi code or even further up the chain. Frankly I'm not educated
> enough on that code to safely mess with it. To handle a "blocking"
> mode, the driver would need some way to properly communicate it is full
> to rawmidi. This could be either some method to do this specfically, or
> just that rawmidi's buffer doesn't get emptied. But, at this time, that
> seems to cause a problem.
my opinion is that it's bad behavior to abandon the data IMMEDIATELY
when the local buffer is full. this situation can happen quite easily
when you send a large bulk data. the uart's local buffer will be full
soon, then the successding data will be simply lost.
it's true that in some special cases, we need to reset the device.
but, as far as i understand, the buffer-full is not the criteria of
the reset, since it may happen even with the normal operation.
maybe it'd be better to add a timeout for the sanity check. firstly
after the time out is detected, we can do some reset work on the
device, e.g. flush the buffer, etc.
(the communication over the ALSA sequencer is a different story,
though...)
well, until this is implemented, we can add the switch (e.g. a module
parameter) for your behavior (ignore-buffer-overflow), so that the
driver works at least for your purpose...
> >>3. When able to write, drain output as necessary instead of
> >>only writing one byte at a time. This was causing us to
> >>backup eventually anyway since we usually write more than
> >>one byte of data.
> >
> >
> > looking at the code, only MS124W_SA and GENERIC types don't do loop.
> > so, it might be intentional, although i don't see any reason against
> > the looping.
> >
>
> I talked w/ the person who said he added that if(...) in there in the
> first place and I was told it wasn't intentional. So I think that the
> loop should be fine and it makes it more consistant with the second
> clause. BTW, we're using the GENERIC type. Not having the loop
> actually caused a secondary problem.
ok, thanks for the confirmation.
> >
> > btw, please use the unified diff (-u) as the patch at the next time.
> > it's much more readable.
> >
> Sure, not a problem. BTW, there doesn't seem to be anywhere on the alsa
> web site (and no mention in alsa docs or readme) of what patch format to
> use. I researched it for quite awhile and then gave up and just used
> the form that GNU's GCC project says to use
> (http://www.gnu.org/software/gcc/contribute.html#patches). I'm a past
> contributer of GCC and Binutils, so I just fell back on what I'm used
> to. What is the Alsa project's offical format? -u? -up?
we follow the linux-kernel style. in general, -u would suffice, but
of course -up would be nicer in many cases.
ciao,
Takashi
-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive? Does it
help you create better code? SHARE THE LOVE, and help us help
YOU! Click Here: http://sourceforge.net/donate/
_______________________________________________
Alsa-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/alsa-devel
