RE: Drivers: hv: vmbus: One function call less in create_gpadl_header() after error detection

[Michael Kelley]

From: Markus Elfring <Markus.Elfring@xxxxxx> Sent: Wednesday, January 10, 2024 9:08 AM
> 
> > It occurred to me overnight that the existing error handling
> > in create_gpadl_header() is unnecessarily complicated.  Here's
> > an approach that I think would fix what you have flagged, and
> > would reduce complexity instead of increasing it.  Thoughts?
> 
> I find this development view interesting.
> 
> 
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index 56f7e06c673e..44b1d5c8dfed 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -336,7 +336,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  			  sizeof(struct gpa_range) + pfncount * sizeof(u64);
> >  		msgheader =  kzalloc(msgsize, GFP_KERNEL);
> >  		if (!msgheader)
> > -			goto nomem;
> > +			return -ENOMEM;
> >
> >  		INIT_LIST_HEAD(&msgheader->submsglist);
> >  		msgheader->msgsize = msgsize;
> > @@ -386,8 +386,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  					list_del(&pos->msglistentry);
> >  					kfree(pos);
> >  				}
> > -
> > -				goto nomem;
> > +				kfree(msgheader);
> > +				return -ENOMEM;
> >  			}
> >
> >  			msgbody->msgsize = msgsize;
> > @@ -416,8 +416,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  			  sizeof(struct vmbus_channel_gpadl_header) +
> >  			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
> >  		msgheader = kzalloc(msgsize, GFP_KERNEL);
> > -		if (msgheader == NULL)
> > -			goto nomem;
> > +		if (!msgheader)
> > +			return -ENOMEM;
> >
> >  		INIT_LIST_HEAD(&msgheader->submsglist);
> >  		msgheader->msgsize = msgsize;
> > @@ -437,10 +437,6 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  	}
> >
> >  	return 0;
> > -nomem:
> > -	kfree(msgheader);
> > -	kfree(msgbody);
> > -	return -ENOMEM;
> >  }
> >
> >  /*
> >
> 
> Should up to two memory areas still be released after a data processing
> failure?

The function create_gpadl_header() is responsible only for allocating
the memory and filling in various fields.  It doesn't do any processing of
the data and doesn't generate any errors other than memory allocation
failures.  If create_gpadl_header() succeeds, it returns a pointer to the
allocated memory via the msginfo parameter, and the caller becomes
responsible for free'ing the memory.

The only caller is __vmbus_establish_gpadl(), which *does* free the
memory after communicating with Hyper-V.   Processing errors may
occur when communicating with Hyper-V, but in a quick review,
__vmbus_establish_gpadl() seems to handle those errors and to
correctly free the memory.

Michael