Received-SPF: pass (google.com: domain of linux-kernel+bounces-84477-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <56b67440-f6a7-4266-a8be-1e312ebd9aae@linux.intel.com>
Date: Tue, 27 Feb 2024 19:31:59 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V2 9/9] tools: intel_sdsi: Add current meter support
Content-Language: en-US
To: "David E. Box" <david.e.box@linux.intel.com>,
 rajvi.jingar@linux.intel.com, platform-driver-x86@vger.kernel.org,
 linux-kernel@vger.kernel.org, hdegoede@redhat.com,
 ilpo.jarvinen@linux.intel.com
References: <20240228000016.1685518-1-david.e.box@linux.intel.com>
 <20240228000016.1685518-10-david.e.box@linux.intel.com>
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
In-Reply-To: <20240228000016.1685518-10-david.e.box@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 2/27/24 4:00 PM, David E. Box wrote:
> Add support to read the 'meter_current' file. The display is the same as
> the 'meter_certificate', but will show the current snapshot of the
> counters.
>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> V2 - Set the name of the file to be opened once.
>
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 49 ++++++++++++++++----------
>  1 file changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index a8fb6d17405f..325e1e41af1d 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -182,6 +182,7 @@ struct sdsi_dev {
>  enum command {
>  	CMD_SOCKET_INFO,
>  	CMD_METER_CERT,
> +	CMD_METER_CURRENT_CERT,
>  	CMD_STATE_CERT,
>  	CMD_PROV_AKC,
>  	CMD_PROV_CAP,
> @@ -329,13 +330,14 @@ static void get_feature(uint32_t encoding, char *feature)
>  	feature[0] = name[3];
>  }
>  
> -static int sdsi_meter_cert_show(struct sdsi_dev *s)
> +static int sdsi_meter_cert_show(struct sdsi_dev *s, bool show_current)
>  {
>  	char buf[METER_CERT_MAX_SIZE] = {0};
>  	struct bundle_encoding_counter *bec;
>  	struct meter_certificate *mc;
>  	uint32_t count = 0;
>  	FILE *cert_ptr;
> +	char *cert_fname;
Nit: I think this can be const char *
>  	int ret, size;
>  	char name[4];
>  
> @@ -345,7 +347,6 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  
>  	if (!s->regs.en_features.sdsi) {
>  		fprintf(stderr, "SDSi feature is present but not enabled.\n");
> -		fprintf(stderr, " Unable to read meter certificate\n");
>  		return -1;
>  	}
>  
> @@ -360,15 +361,17 @@ static int sdsi_meter_cert_show(struct sdsi_dev *s)
>  		return ret;
>  	}
>  
> -	cert_ptr = fopen("meter_certificate", "r");
> +	cert_fname = show_current ? "meter_current" : "meter_certificate";
> +	cert_ptr = fopen(cert_fname, "r");
> +
>  	if (!cert_ptr) {
> -		perror("Could not open 'meter_certificate' file");
> +		fprintf(stderr, "Could not open '%s' file: %s", cert_fname, strerror(errno));
>  		return -1;
>  	}
>  
>  	size = fread(buf, 1, sizeof(buf), cert_ptr);
>  	if (!size) {
> -		fprintf(stderr, "Could not read 'meter_certificate' file\n");
> +		fprintf(stderr, "Could not read '%s' file\n", cert_fname);
>  		fclose(cert_ptr);
>  		return -1;
>  	}
> @@ -734,7 +737,7 @@ static void sdsi_free_dev(struct sdsi_dev *s)
>  
>  static void usage(char *prog)
>  {
> -	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-m] [-a FILE] [-c FILE]]\n", prog);
> +	printf("Usage: %s [-l] [-d DEVNO [-i] [-s] [-m | -C] [-a FILE] [-c FILE]\n", prog);
>  }
>  
>  static void show_help(void)
> @@ -743,8 +746,9 @@ static void show_help(void)
>  	printf("  %-18s\t%s\n", "-l, --list",           "list available On Demand devices");
>  	printf("  %-18s\t%s\n", "-d, --devno DEVNO",    "On Demand device number");
>  	printf("  %-18s\t%s\n", "-i, --info",           "show socket information");
> -	printf("  %-18s\t%s\n", "-s, --state",          "show state certificate");
> -	printf("  %-18s\t%s\n", "-m, --meter",          "show meter certificate");
> +	printf("  %-18s\t%s\n", "-s, --state",          "show state certificate data");
> +	printf("  %-18s\t%s\n", "-m, --meter",          "show meter certificate data");
> +	printf("  %-18s\t%s\n", "-C, --meter_current",  "show live unattested meter data");
>  	printf("  %-18s\t%s\n", "-a, --akc FILE",       "provision socket with AKC FILE");
>  	printf("  %-18s\t%s\n", "-c, --cap FILE>",      "provision socket with CAP FILE");
>  }
> @@ -760,21 +764,22 @@ int main(int argc, char *argv[])
>  	int option_index = 0;
>  
>  	static struct option long_options[] = {
> -		{"akc",		required_argument,	0, 'a'},
> -		{"cap",		required_argument,	0, 'c'},
> -		{"devno",	required_argument,	0, 'd'},
> -		{"help",	no_argument,		0, 'h'},
> -		{"info",	no_argument,		0, 'i'},
> -		{"list",	no_argument,		0, 'l'},
> -		{"meter",	no_argument,		0, 'm'},
> -		{"state",	no_argument,		0, 's'},
> -		{0,		0,			0, 0 }
> +		{"akc",			required_argument,	0, 'a'},
> +		{"cap",			required_argument,	0, 'c'},
> +		{"devno",		required_argument,	0, 'd'},
> +		{"help",		no_argument,		0, 'h'},
> +		{"info",		no_argument,		0, 'i'},
> +		{"list",		no_argument,		0, 'l'},
> +		{"meter",		no_argument,		0, 'm'},
> +		{"meter_current",	no_argument,		0, 'C'},
> +		{"state",		no_argument,		0, 's'},
> +		{0,			0,			0, 0 }
>  	};
>  
>  
>  	progname = argv[0];
>  
> -	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hilms", long_options,
> +	while ((opt = getopt_long_only(argc, argv, "+a:c:d:hilmCs", long_options,
>  			&option_index)) != -1) {
>  		switch (opt) {
>  		case 'd':
> @@ -790,6 +795,9 @@ int main(int argc, char *argv[])
>  		case 'm':
>  			command = CMD_METER_CERT;
>  			break;
> +		case 'C':
> +			command = CMD_METER_CURRENT_CERT;
> +			break;
>  		case 's':
>  			command = CMD_STATE_CERT;
>  			break;
> @@ -828,7 +836,10 @@ int main(int argc, char *argv[])
>  			ret = sdsi_read_reg(s);
>  			break;
>  		case CMD_METER_CERT:
> -			ret = sdsi_meter_cert_show(s);
> +			ret = sdsi_meter_cert_show(s, false);
> +			break;
> +		case CMD_METER_CURRENT_CERT:
> +			ret = sdsi_meter_cert_show(s, true);
>  			break;
>  		case CMD_STATE_CERT:
>  			ret = sdsi_state_cert_show(s);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer