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@xxxxxxx> wrote:
>>> Am 16.09.2015 um 03:18 schrieb Peter Hurley:
>>>> On Tue, Sep 15, 2015 at 8:37 PM, Tilman Schmidt <tilman@xxxxxxx> wrote:
>>>>> Am 16.09.2015 um 01:08 schrieb Peter Hurley:
>>>>>> On Tue, Sep 15, 2015 at 10:22 AM, Jiri Slaby <jslaby@xxxxxxx> wrote:
>>>>>>
>>>>>> From: Tilman Schmidt <tilman@xxxxxxx>
>>>>>>
>>>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html