Re: [PATCH 3.12 16/33] isdn/gigaset: reset tty->receive_room when attaching ser_gigaset

[Peter Hurley <peter@hurleysoftware.com>]

On 09/18/2015 08:38 AM, Tilman Schmidt wrote:
> Am 17.09.2015 um 20:13 schrieb Peter Hurley:
>> On Wed, Sep 16, 2015 at 7:26 AM, Tilman Schmidt <tilman@imap.cc> wrote:
>>> Am 16.09.2015 um 03:18 schrieb Peter Hurley:
>>>> On Tue, Sep 15, 2015 at 8:37 PM, Tilman Schmidt <tilman@imap.cc> wrote:
>>>>> Am 16.09.2015 um 01:08 schrieb Peter Hurley:
>>>>>> On Tue, Sep 15, 2015 at 10:22 AM, Jiri Slaby <jslaby@suse.cz> wrote:
>>>>>>
>>>>>>     From: Tilman Schmidt <tilman@imap.cc>
>>>>>>
>>>>>>     3.12-stable review patch.  If anyone has any objections, please let
>>>>>>     me know.
>>>>>>
>>>>>>     ===============
>>>>>>
>>>>>>     [ Upstream commit fd98e9419d8d622a4de91f76b306af6aa627aa9c ]
>>>>>>
>>>>>>     Commit 79901317ce80 ("n_tty: Don't flush buffer when closing ldisc"),
>>>>>>     first merged in kernel release 3.10, caused the following regression
>>>>>>     in the Gigaset M101 driver:
>>>>>>
>>>>>>
>>>>>> Again, I'll just note my objection to this commit log.
>>>>>>
>>>>>> This driver was always broken because it never initialized
>>>>>> tty->receive_room,
>>>>>> but rather relied on common but not guaranteed circumstances to
>>>>>> function.
>>>>>>
>>>>>> The commit noted simply made the underlying bug more evident, but the
>>>>>> root cause was from the original merge commit of this driver.
>>>>>
>>>>> I must admit I still don't understand that objection. The meaning of the
>>>>> term "regression" is simply that something which previously worked
>>>>> stopped working. It doesn't imply any statement about the root cause.
>>>>>
>>>>> The ser-gigaset driver worked before the introduction of commit
>>>>> 79901317ce80. It didn't work anymore after the introduction of that
>>>>> commit. So it is correct, and does not contradict your statements above
>>>>> in any way, to state that commit introduced the described regression.
>>>>
>>>> By asserting that commit 79901317ce80 caused the regression, you're
>>>> claiming that this fix is unnecessary for kernel versions prior to 3.10
>>>
>>> Correct.
>>>
>>>> Are you certain that no other sequence of state leads to the same
>>>> condition (and thus requiring the same fix) in earlier kernel versions?
>>>
>>> Reasonably certain, yes, for three reasons:
>>> - There where no reports of that problem before 3.10.
>>
>>
>>
>>> - My own tests did never encounter that condition, and even after being
>>> made aware of it I was not able to come up with a test that would
>>> provoke it with a kernel version before 3.10.
>>
>> Do any of your tests switch to this line discipline from any other than N_TTY?
> 
> Of course not. That wouldn't make any sense.
> 
>> So for example, if you manually set N_PPP (as if by user error)
> 
> User error wouldn't suffice, as the LD would get reset to N_TTY when the
> serial device is closed. You would have to write a program that
> deliberately switched the LD first to N_PPP and then to N_GIGASET_M101
> without closing the device in between.

???

The tool you authored will do it from the command line

$ ldattach PPP /dev/ttyS1
$ ldattach GIGASET_M101 /dev/ttyS1

Note that nothing here closes the serial device 'in between', and
the tty core has switched directly from PPP to GIGASET_M101.
n_tty->receive_room is now 64K.

Please add switching from line disciplines other than N_TTY to your
regression testing.

>> and then set this line discipline, tty->receive_room will be 64K, not 4K.
> 
> That wouldn't affect the operation of ser_gigaset,

I've explained this before to you, but here it is again:

tty->receive_room announces the maximum amt of data the line discipline
can accept from tty core with each call to its receive_buf() method (for
line disciplines that don't provide flow control).

If the line discipline sets ->receive_room to 64K but can only handle
8K (as in the case of GIGASET_M101), then data loss should be the expected
result.


> so even if I had set
> up such a contrived test scenario it wouldn't have exposed any problem.
> Only setting tty->receive_room to 0 causes the problem, and N_TTY with
> commit 79901317ce80 is the only LD which does that.
> 
>>> - The requirement for line disciplines to set receive_room wasn't (and
>>> btw still isn't) documented anywhere, so it's unlikely anything actively
>>> relied on it.
>>
>> Nevertheless, that is the requirement, and what every other in-tree line
>> discipline does.
> 
> Your word for it. Still I don't understand the curious resistance to
> documenting it. If it is the requirement, why keep it secret?

Nothing sinister here :)

Feel free to submit documentation patches.

Regards,
Peter Hurley