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

[Yazen Ghannam <yazen.ghannam@amd.com>]

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