Re: [PATCH RFC 1/3] tpm_crb: expand struct crb_control_area to struct crb_regs

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Sun, Oct 09, 2016 at 03:15:09AM +0300, Jarkko Sakkinen wrote:
> +	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address,
> +			   sizeof(struct crb_regs) -
> +			   offsetof(struct crb_regs, ctrl_req));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	/* The control area always overrlaps IO memory mapped from the ACPI
> +	 * object with CRB start only devices. Thus, this is perfectly safe.
> +	 */
> +	priv->regs = (void *)((unsigned long)ctrl -
> +		offsetof(struct crb_regs, ctrl_req));

Hum. No, this makes bad assumptions about the structure of iomapping.

The map itself needs to be done with the adjustment:

	ctrl = crb_map_res(dev, priv, &io_res, buf->control_address -
		offsetof(struct crb_regs, ctrl_req),
	   	sizeof(struct crb_regs));

.. and nothing actually proves that control_address follows anything
in the driver, so this seems like a terrifying blind assumption, but
everything about the iomap in this ACPI binding seems totally bonkers
so that is in good company I guess.

.. and the comment says this only holds for 'crb start only' devices,
but the code doesn't actually act differently based on what sort of
device we have..

Your commit message also seems to imply the new registers are only on
newer hardware, but nothing seems to check for that before acessing
them?  Confusing.

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot