Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[Michal SuchÃnek]

Hello,


On Mon, 16 Oct 2017 19:34:56 +0200
SF Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:

> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> Date: Mon, 16 Oct 2017 19:00:34 +0200
> 
> Two pointer checks could be repeated by the tpm_ibmvtpm_probe()
> function during error handling even if the relevant properties can be
> determined for the involved variables before by source code analysis.
> 
> * Return directly after a call of the function "kzalloc" failed
>   at the beginning.
> 
> * Adjust jump targets so that extra checks can be omitted at the end.
> 
> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/char/tpm/tpm_ibmvtpm.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
> b/drivers/char/tpm/tpm_ibmvtpm.c index a4b462a77b99..b8dda7546f64
> 100644 --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -610,7 +610,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, 
>  	ibmvtpm = kzalloc(sizeof(*ibmvtpm), GFP_KERNEL);
>  	if (!ibmvtpm)
> -		goto cleanup;
> +		return -ENOMEM;

Just no.

I have seen many fixes that do inverse of this after a piece of code
allocating some more resources was added before code that returns
straight away because it is the first allocation in a function.

>  
>  	ibmvtpm->dev = dev;
>  	ibmvtpm->vdev = vio_dev;
> @@ -619,7 +619,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, crq_q->crq_addr = (struct ibmvtpm_crq
> *)get_zeroed_page(GFP_KERNEL); if (!crq_q->crq_addr) {
>  		dev_err(dev, "Unable to allocate memory for
> crq_addr\n");
> -		goto cleanup;
> +		goto free_tpm;
>  	}
>  
>  	crq_q->num_entry = CRQ_RES_BUF_SIZE /
> sizeof(*crq_q->crq_addr); @@ -629,7 +629,7 @@ static int
> tpm_ibmvtpm_probe(struct vio_dev *vio_dev, 
>  	if (dma_mapping_error(dev, ibmvtpm->crq_dma_handle)) {
>  		dev_err(dev, "dma mapping failed\n");
> -		goto cleanup;
> +		goto free_page;
>  	}
>  
>  	rc = plpar_hcall_norets(H_REG_CRQ, vio_dev->unit_address,
> @@ -683,13 +683,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, reg_crq_cleanup:
>  	dma_unmap_single(dev, ibmvtpm->crq_dma_handle,
> CRQ_RES_BUF_SIZE, DMA_BIDIRECTIONAL);
> -cleanup:
> -	if (ibmvtpm) {
> -		if (crq_q->crq_addr)
> -			free_page((unsigned long)crq_q->crq_addr);
> -		kfree(ibmvtpm);
> -	}
> -

I think a single cleanup section is better than many labels that just
avoid a single null check.

As long as you can tell easily which resources were already allocated
and need to be freed it is saner to keep only one cleanup section.

If the code doing the allocation is changed in the future the single
cleanup can stay whereas multiple labels have to be rewritten again.

Also just changing this just for the sake of code style does not seem
worth it whatever style you prefer.

Thanks

Michal