On Mon, 13 Nov 2023, Harshit Mogalapalli wrote:
> On 13/11/23 7:01 pm, Ilpo Järvinen wrote:
> > On Sat, 11 Nov 2023, Harshit Mogalapalli wrote:
> > > On 10/11/23 8:14 pm, Ilpo Järvinen wrote:
> > > > On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:
> > > >
> > >
> > > Thanks for the review.
> > >
> > > > This changelog needs to be rewritten, it contains multiple errors. I
> > > > suppose even this patch could be split into two but I'll not be too
> > > > picky
> > > > here if you insist on fixing them in the same patch.
> > > >
> > >
> > > Any thoughts on how to split this into two patches ?
> > >
> > > I thought of fixing memory leak in separate patch, but that would add more
> > > code which should be removed when we move kobject_put() to the end.
> >
> Thanks for the suggestions.
>
> > I meant that in the first patch you fix add the missing kfree(). Then in
> > the second one, you correct kobject_put() + play with goto labels. There's
> > no extra code that needs to be added and then removed AFAICT.
> >
>
> This is the problem I am trying to explain:
>
> Let us say we do memory leak fixing in first patch:
>
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 351d782f3e96..7f29a746210e 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -612,6 +612,7 @@ static int hp_add_other_attributes(int attr_type)
> default:
> pr_err("Error: Unknown attr_type: %d\n", attr_type);
> ret = -EINVAL;
> + kfree(attr_name_kobj);
> goto err_other_attr_init;
> }
>
> @@ -637,8 +638,10 @@ static int hp_add_other_attributes(int attr_type)
> ret = -EINVAL;
> }
>
> - if (ret)
> + if (ret) {
> + kfree(attr_name_kobj); ///// [1]
This relates to the 2nd problem (missing kobject_put()) and will be
covered by the other patch. Don't try to solve this in the first patch
at all!
There are two indepedent problems:
- Before kobject_init_and_add(), kfree() is missing
- After kobject_init_and_add(), kobject_put() is missing
Solve each in own patch and don't mix the solutions.
I know both patches are needed to solve _both_ problems so it's fine to
have "still broken" intermediate state as long as you didn't make things
worse in the first patch which you didn't.
> goto err_other_attr_init;
> + }
>
> mutex_unlock(&bioscfg_drv.mutex);
> return 0;
>
> Now when we want to move kobject_put() to goto label in next patch,
> we should remove the kfree [1], as kobject_put() already does a kfree().
>
> If that sounds okay, I will split this into two patches, (first one, only
> fixing memory leak; and second one fixing missing kobject_put() calls on error
> paths)
--
i.
