Messages in this thread | | | Date | Wed, 24 Sep 2014 11:05:51 -0600 | From | Jason Gunthorpe <> | Subject | Re: [PATCH v1 11/12] tpm: Driver for TPM 2.0 CRB Interface |
| |
On Wed, Sep 24, 2014 at 12:06:01PM +0300, Jarkko Sakkinen wrote:
> + offset = cca->rsp_pa - priv->cca_pa; > + resp = (u8 *) ((unsigned long) cca + offset); > + memcpy(buf, resp, 6); > + expected = be32_to_cpu(*(__be32 *) (buf + 2));
be32_to_cpup?
> +static void crb_release(void *data) > +{ > + struct tpm_chip *chip = (struct tpm_chip *) data; > + tpm_remove_hardware(chip->dev); > +}
Please use a proper remove function on the device driver, not a devm function like this. 'tpm_remove_hardware' is the wrong name for a new API, it must be 'tpm_chip_unregister' (ie the undo of 'tpm_chip_register')
> +static int crb_acpi_add(struct acpi_device *device) > +{ > + struct tpm_chip *chip; > + struct acpi_tpm2 *buf; > + struct crb_priv *priv; > + struct device *dev = &device->dev; > + acpi_status status; > + u32 sm; > + int rc; > + > + chip = tpm_chip_alloc(dev, &tpm_crb); > + if (!chip) > + return -ENODEV;
Lets use ERRPTR here
> + chip->tpm2 = true; > + > + rc = tpm_chip_register(chip);
This is in the wrong place, it needs to be the last call in the probe function - the driver must be fully operational when register is called, that is one of the bugs the new interface must be fixing.
> + rc = tpm_do_selftest(chip); > + if (rc) { > + rc = -ENODEV; > + goto out_err; > + }
The common TPM command startup sequence should be in tpm_chip_register(), so move this into there.
> + rc = devm_add_action(dev, crb_release, chip); > + if (rc) > + goto out_err; > + > + return 0; > +out_err: > + tpm_remove_hardware(chip->dev); > + return rc; > +} > + > +static struct acpi_device_id crb_device_ids[] = {
const? Not sure
Jason
| |