Re: [PATCH 01/14] EDAC/amd64: Move struct fam_type variables into struct amd64_pvt

Yazen Ghannam
2022-04-04T21:39:11Z

On Thu, Mar 31, 2022 at 05:45:39PM +0530, Chatradhi, Naveen Krishna wrote:

...

> > > case 0x15:
> > > if (pvt->model == 0x30) {
> > I just noticed that families 15h and 16h aren't matching against a model
> > range. I think we should look into that.
>
> The diff seems fine to me.  Yazen, do you have any inputs on this ?
>

Right, I think this patch is moving the code around correctly. But the code
seems incorrect. So I think it should be fixed if it is indeed incorrect.

> >
> > ...
> >
> > > @@ -4142,10 +4012,13 @@ static int probe_one_instance(unsigned int nid)
> > > pvt->mc_node_id = nid;
> > > pvt->F3 = F3;
> > > + pvt->ops = kzalloc(sizeof(*pvt->ops), GFP_KERNEL);
> > > + if (!pvt->ops)
> > > + goto err_out;
> > > +
> > This should goto err_enable so that pvt and above can be freed.
> > > ret = -ENODEV;
> > > - fam_type = per_family_init(pvt);
> > > - if (!fam_type)
> > > - goto err_enable;
> > > + if (per_family_init(pvt))
> > > + goto err_out;
> > >
> > Same here, but pvt->ops should also be freed at this point too.
>
> The hw_info_get() is not called till this point, calling goto err_enable
> would unnecessarily call hw_info_put()
>
> in the exit path.  Better to introduce goto err_pvt and goto err_pvt_ops
> like below Or can i split the function
>
> by moving the structure allocation part into a separate helper routine ?
>
> @@ -4101,15 +4101,15 @@ static int probe_one_instance(unsigned int nid)
>
>         pvt->ops = kzalloc(sizeof(*pvt->ops), GFP_KERNEL);
>         if (!pvt->ops)
> -               goto err_enable;
> +               goto err_pvt;
>
>         ret = -ENODEV;
>         if (per_family_init(pvt))
> -               goto err_enable;
> +               goto err_pvt_ops;
>
>         ret = hw_info_get(pvt);
>         if (ret < 0)
> -               goto err_enable;
> +               goto err_pvt_ops;
>
>         ret = 0;
>         if (!instance_has_memory(pvt)) {
> @@ -4151,6 +4151,11 @@ static int probe_one_instance(unsigned int nid)
>
>  err_enable:
>         hw_info_put(pvt);
> +
> +err_pvt_ops:
> +       kfree(pvt->ops);
> +
> +err_pvt:
>         kfree(pvt);
>

This seems okay to me.

Thanks,
Yazen