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

[Tilman Schmidt]

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.

> and then set this line discipline, tty->receive_room will be 64K, not 4K.

That wouldn't affect the operation of ser_gigaset, 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?

Regards,
Tilman

-- 
Tilman Schmidt                              E-Mail: tilman@xxxxxxx
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
UngeÃffnet mindestens haltbar bis: (siehe RÃckseite)

Attachment: signature.asc
Description: OpenPGP digital signature