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

[Dan Carpenter]

tpm_ibmvtpm_probe() is an example of poor names.  It has the generic
ones like "cleanup" which don't say *what* is cleaned and the come-from
ones like "init_irq_cleanup" which don't say anything useful at all:

   647          rc = request_irq(vio_dev->irq, ibmvtpm_interrupt, 0,
   648                           tpm_ibmvtpm_driver_name, ibmvtpm);
   649          if (rc) {
   650                  dev_err(dev, "Error %d register irq 0x%x\n", rc, vio_dev->irq);
   651                  goto init_irq_cleanup;
   652          }
   653  
   654          rc = vio_enable_interrupts(vio_dev);
   655          if (rc) {
   656                  dev_err(dev, "Error %d enabling interrupts\n", rc);
   657                  goto init_irq_cleanup;
   658          }

Sadly, we never do call free_irq().

> It can do it automatically, in most cases better, and automatically
> adapt it to code changes.

I've heard this before that if you only have one label that does
everything then it's "automatic" and "future proof".  It's not true.
The best error handling is methodical error handling:

1) In the reverse order from how things were allocated
2) That mirrors the allocations exactly
3) That frees one thing at a time
4) With a proper, useful, readable label name which says what the goto
   does
5) That doesn't free anything which hasn't been allocated

regards,
dan carpenter