| Message ID | 1353475147-28308-1-git-send-email-sachin.kamat@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote: > kfree on a NULL pointer is a no-op. > > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> > --- > drivers/tty/vt/consolemap.c | 6 ++---- > 1 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c > index 2aaa0c2..248381b 100644 > --- a/drivers/tty/vt/consolemap.c > +++ b/drivers/tty/vt/consolemap.c > @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) > kfree(p->inverse_translations[i]); > p->inverse_translations[i] = NULL; > } > - if (p->inverse_trans_unicode) { > - kfree(p->inverse_trans_unicode); > - p->inverse_trans_unicode = NULL; > - } > + kfree(p->inverse_trans_unicode); > + p->inverse_trans_unicode = NULL; kfree with NULL is a no-op, but the line after that just caused a kernel crash if it was NULL, so I can't accept this type of thing. Please be more careful. What's with the patches@linaro.org email address? What is that for? thanks, greg k-h
On 21 November 2012 20:16, Greg KH <gregkh@linuxfoundation.org> wrote: > On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote: >> kfree on a NULL pointer is a no-op. >> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> >> --- >> drivers/tty/vt/consolemap.c | 6 ++---- >> 1 files changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c >> index 2aaa0c2..248381b 100644 >> --- a/drivers/tty/vt/consolemap.c >> +++ b/drivers/tty/vt/consolemap.c >> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) >> kfree(p->inverse_translations[i]); >> p->inverse_translations[i] = NULL; >> } >> - if (p->inverse_trans_unicode) { >> - kfree(p->inverse_trans_unicode); >> - p->inverse_trans_unicode = NULL; >> - } >> + kfree(p->inverse_trans_unicode); >> + p->inverse_trans_unicode = NULL; > > kfree with NULL is a no-op, but the line after that just caused a kernel > crash if it was NULL, so I can't accept this type of thing. > > Please be more careful. My mistake. Apologies for the same. Do we need to assign the pointer to NULL after freeing? > > What's with the patches@linaro.org email address? What is that for? That is a logging mechanism (done by patchwork) for all patches sent by Linaro engineers. > > thanks, > > greg k-h
On Wed, Nov 21, 2012 at 08:21:40PM +0530, Sachin Kamat wrote: > On 21 November 2012 20:16, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote: > >> kfree on a NULL pointer is a no-op. > >> > >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> > >> --- > >> drivers/tty/vt/consolemap.c | 6 ++---- > >> 1 files changed, 2 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c > >> index 2aaa0c2..248381b 100644 > >> --- a/drivers/tty/vt/consolemap.c > >> +++ b/drivers/tty/vt/consolemap.c > >> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) > >> kfree(p->inverse_translations[i]); > >> p->inverse_translations[i] = NULL; > >> } > >> - if (p->inverse_trans_unicode) { > >> - kfree(p->inverse_trans_unicode); > >> - p->inverse_trans_unicode = NULL; > >> - } > >> + kfree(p->inverse_trans_unicode); > >> + p->inverse_trans_unicode = NULL; > > > > kfree with NULL is a no-op, but the line after that just caused a kernel > > crash if it was NULL, so I can't accept this type of thing. > > > > Please be more careful. > > My mistake. Apologies for the same. > > Do we need to assign the pointer to NULL after freeing? It depends if it is checked later on, is it? > > What's with the patches@linaro.org email address? What is that for? > That is a logging mechanism (done by patchwork) for all patches sent > by Linaro engineers. So you can track it internally? Why? greg k-h
On 21 November 2012 20:46, Greg KH <gregkh@linuxfoundation.org> wrote: > On Wed, Nov 21, 2012 at 08:21:40PM +0530, Sachin Kamat wrote: >> On 21 November 2012 20:16, Greg KH <gregkh@linuxfoundation.org> wrote: >> > On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote: >> >> kfree on a NULL pointer is a no-op. >> >> >> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> >> >> --- >> >> drivers/tty/vt/consolemap.c | 6 ++---- >> >> 1 files changed, 2 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c >> >> index 2aaa0c2..248381b 100644 >> >> --- a/drivers/tty/vt/consolemap.c >> >> +++ b/drivers/tty/vt/consolemap.c >> >> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) >> >> kfree(p->inverse_translations[i]); >> >> p->inverse_translations[i] = NULL; >> >> } >> >> - if (p->inverse_trans_unicode) { >> >> - kfree(p->inverse_trans_unicode); >> >> - p->inverse_trans_unicode = NULL; >> >> - } >> >> + kfree(p->inverse_trans_unicode); >> >> + p->inverse_trans_unicode = NULL; >> > >> > kfree with NULL is a no-op, but the line after that just caused a kernel >> > crash if it was NULL, so I can't accept this type of thing. >> > >> > Please be more careful. >> >> My mistake. Apologies for the same. >> >> Do we need to assign the pointer to NULL after freeing? > > It depends if it is checked later on, is it? I will re-send this patch after deleting the assignment? > >> > What's with the patches@linaro.org email address? What is that for? >> That is a logging mechanism (done by patchwork) for all patches sent >> by Linaro engineers. > > So you can track it internally? Why? Yes. For tracking the status. > > greg k-h
On Wed, Nov 21, 2012 at 08:59:08PM +0530, Sachin Kamat wrote: > On 21 November 2012 20:46, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Nov 21, 2012 at 08:21:40PM +0530, Sachin Kamat wrote: > >> On 21 November 2012 20:16, Greg KH <gregkh@linuxfoundation.org> wrote: > >> > On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote: > >> >> kfree on a NULL pointer is a no-op. > >> >> > >> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> > >> >> --- > >> >> drivers/tty/vt/consolemap.c | 6 ++---- > >> >> 1 files changed, 2 insertions(+), 4 deletions(-) > >> >> > >> >> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c > >> >> index 2aaa0c2..248381b 100644 > >> >> --- a/drivers/tty/vt/consolemap.c > >> >> +++ b/drivers/tty/vt/consolemap.c > >> >> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) > >> >> kfree(p->inverse_translations[i]); > >> >> p->inverse_translations[i] = NULL; > >> >> } > >> >> - if (p->inverse_trans_unicode) { > >> >> - kfree(p->inverse_trans_unicode); > >> >> - p->inverse_trans_unicode = NULL; > >> >> - } > >> >> + kfree(p->inverse_trans_unicode); > >> >> + p->inverse_trans_unicode = NULL; > >> > > >> > kfree with NULL is a no-op, but the line after that just caused a kernel > >> > crash if it was NULL, so I can't accept this type of thing. > >> > > >> > Please be more careful. > >> > >> My mistake. Apologies for the same. > >> > >> Do we need to assign the pointer to NULL after freeing? > > > > It depends if it is checked later on, is it? > > I will re-send this patch after deleting the assignment? You tell me, please determine if it really is correct or not, before sending a patch. You did test this, right? greg k-h
On 11/21/2012 03:46 PM, Greg KH wrote: > On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote: >> --- a/drivers/tty/vt/consolemap.c >> +++ b/drivers/tty/vt/consolemap.c >> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) >> kfree(p->inverse_translations[i]); >> p->inverse_translations[i] = NULL; >> } >> - if (p->inverse_trans_unicode) { >> - kfree(p->inverse_trans_unicode); >> - p->inverse_trans_unicode = NULL; >> - } >> + kfree(p->inverse_trans_unicode); >> + p->inverse_trans_unicode = NULL; > > kfree with NULL is a no-op, but the line after that just caused a kernel > crash if it was NULL, so I can't accept this type of thing. Greg, I'm not sure -- what do you mean here? The change actually looks fine to me... We do not dereference p->inverse_trans_unicode there. thanks,
On Wed, Nov 21, 2012 at 11:43:34PM +0100, Jiri Slaby wrote: > On 11/21/2012 03:46 PM, Greg KH wrote: > > On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote: > >> --- a/drivers/tty/vt/consolemap.c > >> +++ b/drivers/tty/vt/consolemap.c > >> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) > >> kfree(p->inverse_translations[i]); > >> p->inverse_translations[i] = NULL; > >> } > >> - if (p->inverse_trans_unicode) { > >> - kfree(p->inverse_trans_unicode); > >> - p->inverse_trans_unicode = NULL; > >> - } > >> + kfree(p->inverse_trans_unicode); > >> + p->inverse_trans_unicode = NULL; > > > > kfree with NULL is a no-op, but the line after that just caused a kernel > > crash if it was NULL, so I can't accept this type of thing. > > Greg, I'm not sure -- what do you mean here? The change actually looks > fine to me... We do not dereference p->inverse_trans_unicode there. If we never dereference it, why is it being set to NULL? greg k-h
On 11/22/2012 12:00 AM, Greg KH wrote: > On Wed, Nov 21, 2012 at 11:43:34PM +0100, Jiri Slaby wrote: >> On 11/21/2012 03:46 PM, Greg KH wrote: >>> On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote: >>>> --- a/drivers/tty/vt/consolemap.c >>>> +++ b/drivers/tty/vt/consolemap.c >>>> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) >>>> kfree(p->inverse_translations[i]); >>>> p->inverse_translations[i] = NULL; >>>> } >>>> - if (p->inverse_trans_unicode) { >>>> - kfree(p->inverse_trans_unicode); >>>> - p->inverse_trans_unicode = NULL; >>>> - } >>>> + kfree(p->inverse_trans_unicode); >>>> + p->inverse_trans_unicode = NULL; >>> >>> kfree with NULL is a no-op, but the line after that just caused a kernel >>> crash if it was NULL, so I can't accept this type of thing. >> >> Greg, I'm not sure -- what do you mean here? The change actually looks >> fine to me... We do not dereference p->inverse_trans_unicode there. > > If we never dereference it, why is it being set to NULL? I'm not disputing the assignment. Let it there. But I do not see any issues with the patch. What problem do you see exactly?
On Thu, Nov 22, 2012 at 12:17:00AM +0100, Jiri Slaby wrote: > On 11/22/2012 12:00 AM, Greg KH wrote: > > On Wed, Nov 21, 2012 at 11:43:34PM +0100, Jiri Slaby wrote: > >> On 11/21/2012 03:46 PM, Greg KH wrote: > >>> On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote: > >>>> --- a/drivers/tty/vt/consolemap.c > >>>> +++ b/drivers/tty/vt/consolemap.c > >>>> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) > >>>> kfree(p->inverse_translations[i]); > >>>> p->inverse_translations[i] = NULL; > >>>> } > >>>> - if (p->inverse_trans_unicode) { > >>>> - kfree(p->inverse_trans_unicode); > >>>> - p->inverse_trans_unicode = NULL; > >>>> - } > >>>> + kfree(p->inverse_trans_unicode); > >>>> + p->inverse_trans_unicode = NULL; > >>> > >>> kfree with NULL is a no-op, but the line after that just caused a kernel > >>> crash if it was NULL, so I can't accept this type of thing. > >> > >> Greg, I'm not sure -- what do you mean here? The change actually looks > >> fine to me... We do not dereference p->inverse_trans_unicode there. > > > > If we never dereference it, why is it being set to NULL? > > I'm not disputing the assignment. Let it there. But I do not see any > issues with the patch. What problem do you see exactly? {sigh} Nothing. I don't know what I was thinking, my apologies Sachin, there's nothing wrong with this patch. I blame my fried brain, after reviewing all of those __dev* removal patches :) Sachin, can you resend these patches so I can apply them? thanks, greg k-h
On 22 November 2012 06:08, Greg KH <gregkh@linuxfoundation.org> wrote: > On Thu, Nov 22, 2012 at 12:17:00AM +0100, Jiri Slaby wrote: >> On 11/22/2012 12:00 AM, Greg KH wrote: >> > On Wed, Nov 21, 2012 at 11:43:34PM +0100, Jiri Slaby wrote: >> >> On 11/21/2012 03:46 PM, Greg KH wrote: >> >>> On Wed, Nov 21, 2012 at 10:49:07AM +0530, Sachin Kamat wrote: >> >>>> --- a/drivers/tty/vt/consolemap.c >> >>>> +++ b/drivers/tty/vt/consolemap.c >> >>>> @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) >> >>>> kfree(p->inverse_translations[i]); >> >>>> p->inverse_translations[i] = NULL; >> >>>> } >> >>>> - if (p->inverse_trans_unicode) { >> >>>> - kfree(p->inverse_trans_unicode); >> >>>> - p->inverse_trans_unicode = NULL; >> >>>> - } >> >>>> + kfree(p->inverse_trans_unicode); >> >>>> + p->inverse_trans_unicode = NULL; >> >>> >> >>> kfree with NULL is a no-op, but the line after that just caused a kernel >> >>> crash if it was NULL, so I can't accept this type of thing. >> >> >> >> Greg, I'm not sure -- what do you mean here? The change actually looks >> >> fine to me... We do not dereference p->inverse_trans_unicode there. >> > >> > If we never dereference it, why is it being set to NULL? >> >> I'm not disputing the assignment. Let it there. But I do not see any >> issues with the patch. What problem do you see exactly? > > {sigh} Nothing. I don't know what I was thinking, my apologies > Sachin, there's nothing wrong with this patch. > > I blame my fried brain, after reviewing all of those __dev* removal > patches :) > > Sachin, can you resend these patches so I can apply them? No problem Greg. I will send them again. > > thanks, > > greg k-h
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c index 2aaa0c2..248381b 100644 --- a/drivers/tty/vt/consolemap.c +++ b/drivers/tty/vt/consolemap.c @@ -410,10 +410,8 @@ static void con_release_unimap(struct uni_pagedir *p) kfree(p->inverse_translations[i]); p->inverse_translations[i] = NULL; } - if (p->inverse_trans_unicode) { - kfree(p->inverse_trans_unicode); - p->inverse_trans_unicode = NULL; - } + kfree(p->inverse_trans_unicode); + p->inverse_trans_unicode = NULL; } /* Caller must hold the console lock */
kfree on a NULL pointer is a no-op. Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> --- drivers/tty/vt/consolemap.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)