Re: [PATCH 3/9] platform/x86/intel/sdsi: Support different GUIDs

[Hans de Goede]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> Newer versions of Intel On Demand hardware may have an expanded list of
> registers to support new features. The register layout is identified by a
> unique GUID that's read during driver probe. Add support for handling
> different GUIDs and add support for current GUIDs [1].
> 
> [1] https://github.com/intel/intel-sdsi/blob/master/os-interface.rst
> 
> Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>

With Andy's remarks fixed this looks good to me:

Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>

Regards,

Hans



> ---
>  drivers/platform/x86/intel/sdsi.c | 47 +++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/sdsi.c b/drivers/platform/x86/intel/sdsi.c
> index bca05b4dd983..ab1f65919fc5 100644
> --- a/drivers/platform/x86/intel/sdsi.c
> +++ b/drivers/platform/x86/intel/sdsi.c
> @@ -27,10 +27,10 @@
>  #define ACCESS_TYPE_LOCAL		3
>  
>  #define SDSI_MIN_SIZE_DWORDS		276
> -#define SDSI_SIZE_CONTROL		8
>  #define SDSI_SIZE_MAILBOX		1024
> -#define SDSI_SIZE_REGS			72
> +#define SDSI_SIZE_REGS			80
>  #define SDSI_SIZE_CMD			sizeof(u64)
> +#define SDSI_SIZE_MAILBOX		1024
>  
>  /*
>   * Write messages are currently up to the size of the mailbox
> @@ -76,6 +76,9 @@
>  #define DT_TBIR				GENMASK(2, 0)
>  #define DT_OFFSET(v)			((v) & GENMASK(31, 3))
>  
> +#define SDSI_GUID_V1			0x006DD191
> +#define SDSI_GUID_V2			0xF210D9EF
> +
>  enum sdsi_command {
>  	SDSI_CMD_PROVISION_AKC		= 0x04,
>  	SDSI_CMD_PROVISION_CAP		= 0x08,
> @@ -100,6 +103,9 @@ struct sdsi_priv {
>  	void __iomem		*control_addr;
>  	void __iomem		*mbox_addr;
>  	void __iomem		*regs_addr;
> +	int			control_size;
> +	int			maibox_size;
> +	int			registers_size;
>  	u32			guid;
>  	u32			features;
>  };
> @@ -444,6 +450,18 @@ static ssize_t registers_read(struct file *filp, struct kobject *kobj,
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct sdsi_priv *priv = dev_get_drvdata(dev);
>  	void __iomem *addr = priv->regs_addr;
> +	int size =  priv->registers_size;
> +
> +	/*
> +	 * The check below is performed by the sysfs caller based on the static
> +	 * file size. But this may be greater than the actual size which is based
> +	 * on the GUID. So check here again based on actual size before reading.
> +	 */
> +	if (off >= size)
> +		return 0;
> +
> +	if (off + count > size)
> +		count = size - off;
>  
>  	memcpy_fromio(buf, addr + off, count);
>  
> @@ -496,6 +514,24 @@ static const struct attribute_group sdsi_group = {
>  };
>  __ATTRIBUTE_GROUPS(sdsi);
>  
> +static int sdsi_get_layout(struct sdsi_priv *priv, struct disc_table *table)
> +{
> +	switch (table->guid) {
> +	case SDSI_GUID_V1:
> +		priv->control_size = 8;
> +		priv->registers_size = 72;
> +		break;
> +	case SDSI_GUID_V2:
> +		priv->control_size = 16;
> +		priv->registers_size = 80;
> +		break;
> +	default:
> +		dev_err(priv->dev, "Unrecognized GUID 0x%x\n", table->guid);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *parent,
>  				   struct disc_table *disc_table, struct resource *disc_res)
>  {
> @@ -537,7 +573,7 @@ static int sdsi_map_mbox_registers(struct sdsi_priv *priv, struct pci_dev *paren
>  	if (IS_ERR(priv->control_addr))
>  		return PTR_ERR(priv->control_addr);
>  
> -	priv->mbox_addr = priv->control_addr + SDSI_SIZE_CONTROL;
> +	priv->mbox_addr = priv->control_addr + priv->control_size;
>  	priv->regs_addr = priv->mbox_addr + SDSI_SIZE_MAILBOX;
>  
>  	priv->features = readq(priv->regs_addr + SDSI_ENABLED_FEATURES_OFFSET);
> @@ -572,6 +608,11 @@ static int sdsi_probe(struct auxiliary_device *auxdev, const struct auxiliary_de
>  
>  	priv->guid = disc_table.guid;
>  
> +	/* Get guid based layout info */
> +	ret = sdsi_get_layout(priv, &disc_table);
> +	if (ret)
> +		return ret;
> +
>  	/* Map the SDSi mailbox registers */
>  	ret = sdsi_map_mbox_registers(priv, intel_cap_dev->pcidev, &disc_table, disc_res);
>  	if (ret)