linux-kernel - Re: [PATCH v3 13/13] bus/fsl-mc: Add a new version for dprc_get_obj

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Jul 2020 16:14:22 +0300
From:   Laurentiu Tudor <laurentiu.tudor@....com>
To:     Diana Craciun <diana.craciun@....com>,
        linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org
Cc:     stuyoder@...il.com, leoyang.li@....com,
        linux-arm-kernel@...ts.infradead.org, bharatb.linux@...il.com,
        Diana Craciun <diana.craciun@....nxp.com>
Subject: Re: [PATCH v3 13/13] bus/fsl-mc: Add a new version for
 dprc_get_obj_region command



On 7/6/2020 3:42 PM, Diana Craciun wrote:
> From: Diana Craciun <diana.craciun@....nxp.com>
> 
> The region size reported by the firmware for mc and software
> portals was less than allocated by the hardware. This may be
> problematic when mmapping the region in user space because the
> region size is less than page size. However the size as reserved
> by the hardware is 64K.

Should we also mention in the commit msg that this shows up when
compiling the kernel with 64K page size support, or it's obvious enough?

> Signed-off-by: Diana Craciun <diana.craciun@....nxp.com>
> ---
>  drivers/bus/fsl-mc/dprc.c           | 38 ++++++++++++++++++-----------
>  drivers/bus/fsl-mc/fsl-mc-private.h |  3 +++
>  2 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c
> index 3f08752c2c19..ba292c56fe19 100644
> --- a/drivers/bus/fsl-mc/dprc.c
> +++ b/drivers/bus/fsl-mc/dprc.c
> @@ -536,20 +536,30 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io,
>  			return err;
>  	}
>  
> -	/**
> -	 * MC API version 6.3 introduced a new field to the region
> -	 * descriptor: base_address. If the older API is in use then the base
> -	 * address is set to zero to indicate it needs to be obtained elsewhere
> -	 * (typically the device tree).
> -	 */
> -	if (dprc_major_ver > 6 || (dprc_major_ver == 6 && dprc_minor_ver >= 3))
> -		cmd.header =
> -			mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V2,
> -					     cmd_flags, token);
> -	else
> -		cmd.header =
> -			mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG,
> -					     cmd_flags, token);
> +	if (dprc_major_ver > 6 || (dprc_major_ver == 6 && dprc_minor_ver >= 6)) {
> +		/**
> +		 * MC API version 6.6 changed the size of the MC portals and software
> +		 * portals to 64K (as implemented by hardware). If older API is in use the
> +		 * size reported is less (64 bytes for mc portals and 4K for software
> +		 * portals).
> +		 */

Here and below, there's no need to use kernel-doc style comments. And a
nit: there's an extra blank line here.

---
Best Regards, Laurentiu

> +		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V3,
> +						  cmd_flags, token);
> +
> +	} else if (dprc_major_ver == 6 && dprc_minor_ver >= 3) {
> +		/**
> +		 * MC API version 6.3 introduced a new field to the region
> +		 * descriptor: base_address. If the older API is in use then the base
> +		 * address is set to zero to indicate it needs to be obtained elsewhere
> +		 * (typically the device tree).
> +		 */
> +		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG_V2,
> +						  cmd_flags, token);
> +	} else {
> +		cmd.header = mc_encode_cmd_header(DPRC_CMDID_GET_OBJ_REG,
> +						  cmd_flags, token);
> +	}
>  
>  	cmd_params = (struct dprc_cmd_get_obj_region *)cmd.params;
>  	cmd_params->obj_id = cpu_to_le32(obj_id);
> diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h b/drivers/bus/fsl-mc/fsl-mc-private.h
> index e6fcff12c68d..8d65273a78d7 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-private.h
> +++ b/drivers/bus/fsl-mc/fsl-mc-private.h
> @@ -80,10 +80,12 @@ int dpmcp_reset(struct fsl_mc_io *mc_io,
>  /* DPRC command versioning */
>  #define DPRC_CMD_BASE_VERSION			1
>  #define DPRC_CMD_2ND_VERSION			2
> +#define DPRC_CMD_3RD_VERSION			3
>  #define DPRC_CMD_ID_OFFSET			4
>  
>  #define DPRC_CMD(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_BASE_VERSION)
>  #define DPRC_CMD_V2(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_2ND_VERSION)
> +#define DPRC_CMD_V3(id)	(((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_3RD_VERSION)
>  
>  /* DPRC command IDs */
>  #define DPRC_CMDID_CLOSE                        DPRC_CMD(0x800)
> @@ -105,6 +107,7 @@ int dpmcp_reset(struct fsl_mc_io *mc_io,
>  #define DPRC_CMDID_GET_OBJ                      DPRC_CMD(0x15A)
>  #define DPRC_CMDID_GET_OBJ_REG                  DPRC_CMD(0x15E)
>  #define DPRC_CMDID_GET_OBJ_REG_V2               DPRC_CMD_V2(0x15E)
> +#define DPRC_CMDID_GET_OBJ_REG_V3               DPRC_CMD_V3(0x15E)
>  #define DPRC_CMDID_SET_OBJ_IRQ                  DPRC_CMD(0x15F)
>  
>  #define DPRC_CMDID_GET_CONNECTION               DPRC_CMD(0x16C)
>