Hello,
On Mon, 16 Oct 2017 19:34:56 +0200 SF Markus Elfring <elfr...@users.sourceforge.net> wrote: > From: Markus Elfring <elfr...@users.sourceforge.net> > 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 <elfr...@users.sourceforge.net> > --- > 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