Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1345342 > unrolled thread

Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON

Started byDmitry Vyukov <dvyukov@google.com>
First post2016-02-28 17:20 +0100
Last post2016-03-01 18:20 +0100
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON Dmitry Vyukov <dvyukov@google.com> - 2016-02-28 17:20 +0100
    Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-03-01 06:10 +0100
      Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON Dmitry Vyukov <dvyukov@google.com> - 2016-03-01 10:10 +0100
        Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-03-01 18:20 +0100

#1345342 — Re: [PATCH] TTY: n_gsm, fix false positive WARN_ON

FromDmitry Vyukov <dvyukov@google.com>
Date2016-02-28 17:20 +0100
SubjectRe: [PATCH] TTY: n_gsm, fix false positive WARN_ON
Message-ID<r7ceB-57l-7@gated-at.bofh.it>
On Wed, Nov 25, 2015 at 11:32 AM, xinhui <xinhui@linux.vnet.ibm.com> wrote:
> hi, Jiri
>
> On 2015/11/25 17:56, Jiri Slaby wrote:
>>
>> Hi,
>>
>> On 11/25/2015, 07:32 AM, xinhui wrote:
>>>
>>>      This warning should blame on commit 5a640967 ("tty/n_gsm.c: fix a
>>> memory leak in gsmld_open()").
>>
>>
>> Oh, yes, I messed up the "Fixes" line then. It should write:
>> Fixes: 5a640967 ("tty/n_gsm.c: fix a memory leak in gsmld_open()")
>>
> that's Okay. :)
>
>>> I have one confusion. As there is field gsm->num to store the index of
>>> gsm_mux[]. so in gsm_cleanup_mux(), why we still use for-loop to find
>>> this mux?
>>>
>>> In error handle path, for example, the call trace in this patch, as we
>>> failed to activate it and the
>>> gsm->num is invalid(and the value is 0). we can just modify the codes
>>> like below:
>>>
>>> if(gsm_mux[gsm->num] == gsm)
>>> ....other work
>>> else
>>>      return;
>>>
>>> I think it would work, and the logic is correct. Or I just miss
>>> something important?
>>
>>
>> Yup, it looks like a cleanup. Could you prepare a separate patch for that?
>>
> yes, I will do that :)
>
>> Something like this:
>>          /* open failed before registering => nothing to do */
>>          if (gsm_mux[gsm->num] != gsm)
>>                return;
>>          spin_lock(&gsm_mux_lock);
>>          gsm_mux[gsm->num] = NULL;
>>          spin_unlock(&gsm_mux_lock);
>>
> looks pretty good, thanks.


This is still not merged and fires regularly for me. Can we please merge it?

[toc] | [next] | [standalone]

#1346366

FromGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Date2016-03-01 06:10 +0100
Message-ID<r7KJj-4CH-5@gated-at.bofh.it>
In reply to#1345342
On Sun, Feb 28, 2016 at 05:16:15PM +0100, Dmitry Vyukov wrote:
> On Wed, Nov 25, 2015 at 11:32 AM, xinhui <xinhui@linux.vnet.ibm.com> wrote:
> > hi, Jiri
> >
> > On 2015/11/25 17:56, Jiri Slaby wrote:
> >>
> >> Hi,
> >>
> >> On 11/25/2015, 07:32 AM, xinhui wrote:
> >>>
> >>>      This warning should blame on commit 5a640967 ("tty/n_gsm.c: fix a
> >>> memory leak in gsmld_open()").
> >>
> >>
> >> Oh, yes, I messed up the "Fixes" line then. It should write:
> >> Fixes: 5a640967 ("tty/n_gsm.c: fix a memory leak in gsmld_open()")
> >>
> > that's Okay. :)
> >
> >>> I have one confusion. As there is field gsm->num to store the index of
> >>> gsm_mux[]. so in gsm_cleanup_mux(), why we still use for-loop to find
> >>> this mux?
> >>>
> >>> In error handle path, for example, the call trace in this patch, as we
> >>> failed to activate it and the
> >>> gsm->num is invalid(and the value is 0). we can just modify the codes
> >>> like below:
> >>>
> >>> if(gsm_mux[gsm->num] == gsm)
> >>> ....other work
> >>> else
> >>>      return;
> >>>
> >>> I think it would work, and the logic is correct. Or I just miss
> >>> something important?
> >>
> >>
> >> Yup, it looks like a cleanup. Could you prepare a separate patch for that?
> >>
> > yes, I will do that :)
> >
> >> Something like this:
> >>          /* open failed before registering => nothing to do */
> >>          if (gsm_mux[gsm->num] != gsm)
> >>                return;
> >>          spin_lock(&gsm_mux_lock);
> >>          gsm_mux[gsm->num] = NULL;
> >>          spin_unlock(&gsm_mux_lock);
> >>
> > looks pretty good, thanks.
> 
> 
> This is still not merged and fires regularly for me. Can we please merge it?

merge what?  I don't see any patch here or in my queue for this :(

[toc] | [prev] | [next] | [standalone]

#1346466

FromDmitry Vyukov <dvyukov@google.com>
Date2016-03-01 10:10 +0100
Message-ID<r7OtA-75N-15@gated-at.bofh.it>
In reply to#1346366
On Tue, Mar 1, 2016 at 6:01 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Feb 28, 2016 at 05:16:15PM +0100, Dmitry Vyukov wrote:
>> On Wed, Nov 25, 2015 at 11:32 AM, xinhui <xinhui@linux.vnet.ibm.com> wrote:
>> > hi, Jiri
>> >
>> > On 2015/11/25 17:56, Jiri Slaby wrote:
>> >>
>> >> Hi,
>> >>
>> >> On 11/25/2015, 07:32 AM, xinhui wrote:
>> >>>
>> >>>      This warning should blame on commit 5a640967 ("tty/n_gsm.c: fix a
>> >>> memory leak in gsmld_open()").
>> >>
>> >>
>> >> Oh, yes, I messed up the "Fixes" line then. It should write:
>> >> Fixes: 5a640967 ("tty/n_gsm.c: fix a memory leak in gsmld_open()")
>> >>
>> > that's Okay. :)
>> >
>> >>> I have one confusion. As there is field gsm->num to store the index of
>> >>> gsm_mux[]. so in gsm_cleanup_mux(), why we still use for-loop to find
>> >>> this mux?
>> >>>
>> >>> In error handle path, for example, the call trace in this patch, as we
>> >>> failed to activate it and the
>> >>> gsm->num is invalid(and the value is 0). we can just modify the codes
>> >>> like below:
>> >>>
>> >>> if(gsm_mux[gsm->num] == gsm)
>> >>> ....other work
>> >>> else
>> >>>      return;
>> >>>
>> >>> I think it would work, and the logic is correct. Or I just miss
>> >>> something important?
>> >>
>> >>
>> >> Yup, it looks like a cleanup. Could you prepare a separate patch for that?
>> >>
>> > yes, I will do that :)
>> >
>> >> Something like this:
>> >>          /* open failed before registering => nothing to do */
>> >>          if (gsm_mux[gsm->num] != gsm)
>> >>                return;
>> >>          spin_lock(&gsm_mux_lock);
>> >>          gsm_mux[gsm->num] = NULL;
>> >>          spin_unlock(&gsm_mux_lock);
>> >>
>> > looks pretty good, thanks.
>>
>>
>> This is still not merged and fires regularly for me. Can we please merge it?
>
> merge what?  I don't see any patch here or in my queue for this :(


"[PATCH] TTY: n_gsm, fix false positive WARN_ON" from Jiri Slaby:
https://lkml.org/lkml/2015/11/24/600

FWIW I have it in my tree for 3 months. Warnings have gone. No issues noticed.

[toc] | [prev] | [next] | [standalone]

#1346780

FromGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Date2016-03-01 18:20 +0100
Message-ID<r7W7L-3DI-13@gated-at.bofh.it>
In reply to#1346466
On Tue, Mar 01, 2016 at 10:03:30AM +0100, Dmitry Vyukov wrote:
> On Tue, Mar 1, 2016 at 6:01 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sun, Feb 28, 2016 at 05:16:15PM +0100, Dmitry Vyukov wrote:
> >> On Wed, Nov 25, 2015 at 11:32 AM, xinhui <xinhui@linux.vnet.ibm.com> wrote:
> >> > hi, Jiri
> >> >
> >> > On 2015/11/25 17:56, Jiri Slaby wrote:
> >> >>
> >> >> Hi,
> >> >>
> >> >> On 11/25/2015, 07:32 AM, xinhui wrote:
> >> >>>
> >> >>>      This warning should blame on commit 5a640967 ("tty/n_gsm.c: fix a
> >> >>> memory leak in gsmld_open()").
> >> >>
> >> >>
> >> >> Oh, yes, I messed up the "Fixes" line then. It should write:
> >> >> Fixes: 5a640967 ("tty/n_gsm.c: fix a memory leak in gsmld_open()")
> >> >>
> >> > that's Okay. :)
> >> >
> >> >>> I have one confusion. As there is field gsm->num to store the index of
> >> >>> gsm_mux[]. so in gsm_cleanup_mux(), why we still use for-loop to find
> >> >>> this mux?
> >> >>>
> >> >>> In error handle path, for example, the call trace in this patch, as we
> >> >>> failed to activate it and the
> >> >>> gsm->num is invalid(and the value is 0). we can just modify the codes
> >> >>> like below:
> >> >>>
> >> >>> if(gsm_mux[gsm->num] == gsm)
> >> >>> ....other work
> >> >>> else
> >> >>>      return;
> >> >>>
> >> >>> I think it would work, and the logic is correct. Or I just miss
> >> >>> something important?
> >> >>
> >> >>
> >> >> Yup, it looks like a cleanup. Could you prepare a separate patch for that?
> >> >>
> >> > yes, I will do that :)
> >> >
> >> >> Something like this:
> >> >>          /* open failed before registering => nothing to do */
> >> >>          if (gsm_mux[gsm->num] != gsm)
> >> >>                return;
> >> >>          spin_lock(&gsm_mux_lock);
> >> >>          gsm_mux[gsm->num] = NULL;
> >> >>          spin_unlock(&gsm_mux_lock);
> >> >>
> >> > looks pretty good, thanks.
> >>
> >>
> >> This is still not merged and fires regularly for me. Can we please merge it?
> >
> > merge what?  I don't see any patch here or in my queue for this :(
> 
> 
> "[PATCH] TTY: n_gsm, fix false positive WARN_ON" from Jiri Slaby:
> https://lkml.org/lkml/2015/11/24/600
> 
> FWIW I have it in my tree for 3 months. Warnings have gone. No issues noticed.

I don't see it in my queue, nor in Linus's tree, so someone needs to
resend it if they want it merged into the kernel tree...

{hint}

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web