Re: [igt-dev] [PATCH i-g-t 3/6] tools/dpcd_reg: Unify file handling.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Sep 04, 2018 at 05:56:01PM -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-08-31 at 21:18 -0700, Rodrigo Vivi wrote:
> > Besides removing few code duplication this will
> > allow us to read multiple regs without having
> > to keep closing and opening the file for every
> > read.
> > 
> > Cc: Tarun Vyas <tarun.vyas@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  tools/dpcd_reg.c | 98 +++++++++++++++++++++++++++-------------------
> > --
> >  1 file changed, 55 insertions(+), 43 deletions(-)
> > 
> > diff --git a/tools/dpcd_reg.c b/tools/dpcd_reg.c
> > index d139007c..09da4109 100644
> > --- a/tools/dpcd_reg.c
> > +++ b/tools/dpcd_reg.c
> > @@ -53,61 +53,56 @@ static void print_usage(char *tool, int help)
> >  	exit((help == 1)? EXIT_SUCCESS : EXIT_FAILURE);
> >  }
> >  
> > -static int dpcd_read(char *device, const uint32_t offset, size_t
> > count)
> > +static int dpcd_read(int fd, const uint32_t offset, size_t count)
> >  {
> > -	int fd, ret, i;
> > +	int ret, i;
> >  	void *buf = malloc(sizeof(uint8_t) * count);
> >  
> > +	ret = lseek(fd, offset, SEEK_SET);
> Does this offer any benefit over pread()/pwrite()?

I'm honestly not sure. Can we trust pread/pwrite that offsets are always
relative to the begin of the file or relative to where the pointer
was left after last read/write operation but file not closed?

looking to the docs lseek seemed more crispy on the definition
of its navigation and since I cannot guarantee order on dpcd_list
I preferred lseek.

but if you are sure pread/pwrite is reliable and not relative position
and you have strong preference on that I can change back.

Thanks,
Rodrigo.

> 
> > +	if (ret < 0) {
> > +		igt_warn("lseek to %04x failed!\n", offset);
> > +		return ret;
> > +	}
> > +
> >  	if (NULL != buf)
> >  		memset(buf, 0, sizeof(uint8_t) * count);
> >  	else {
> >  		igt_warn("Can't allocate read buffer\n");
> > -		return EXIT_FAILURE;
> > +		return -1;
> >  	}
> >  
> > -	fd = open(device, O_RDONLY);
> > -	if (fd > 0) {
> > -		ret = pread(fd, buf, count, offset);
> > -		close(fd);
> > -		if (ret != count) {
> > -			igt_warn("Failed to read from %s aux device
> > - error %s\n", device, strerror(errno));
> > -			ret = EXIT_FAILURE;
> > -			goto out;
> > -		}
> > -
> > -		igt_info("%04x:", offset);
> > -		for (i = 0; i < count; i++)
> > -			igt_info(" %02x", *(((uint8_t *)(buf)) +
> > i));
> > -		igt_info("\n");
> > -	}
> > -	else {
> > -		igt_warn("Failed to open %s aux device - error:
> > %s\n", device, strerror(errno));
> > -		ret = EXIT_FAILURE;
> > +	ret = read(fd, buf, count);
> > +	if (ret != count) {
> > +		igt_warn("Failed to read - error %s\n",
> > strerror(errno));
> > +		goto out;
> >  	}
> > +
> > +	igt_info("%04x:", offset);
> > +	for (i = 0; i < count; i++)
> > +		igt_info(" %02x", *(((uint8_t *)(buf)) + i));
> > +	igt_info("\n");
> > +
> >  out:
> >  	free(buf);
> >  	return ret;
> >  }
> >  
> > -static int dpcd_write(char *device, const uint32_t offset, const
> > uint8_t *val)
> > +static int dpcd_write(int fd, const uint32_t offset, const uint8_t
> > *val)
> >  {
> > -	int fd, ret;
> > -
> > -	fd = open(device, O_RDWR);
> > -	if (fd > 0) {
> > -		ret = pwrite(fd, (const void *)val, sizeof(uint8_t),
> > offset);
> > -		close(fd);
> > -		if (ret < 0) {
> > -			igt_warn("Failed to write to %s aux device -
> > error %s\n", device, strerror(errno));
> > -			ret = EXIT_FAILURE;
> > -			goto out;
> > -		}
> > -		ret = dpcd_read(device, offset, sizeof(uint8_t));
> > +	int ret;
> > +
> > +	ret = lseek(fd, offset, SEEK_SET);
> > +	if (ret < 0) {
> > +		igt_warn("lseek to %04x failed!\n", offset);
> > +		return ret;
> >  	}
> > -	else {
> > -		igt_warn("Failed to open %s aux device - error:
> > %s\n", device, strerror(errno));
> > -		ret = EXIT_FAILURE;
> > +
> > +	ret = write(fd, (const void *)val, sizeof(uint8_t));
> > +	if (ret < 0) {
> > +		igt_warn("Failed to write - error %s\n",
> > strerror(errno));
> > +		goto out;
> >  	}
> > +	ret = dpcd_read(fd, offset, sizeof(uint8_t));
> >  out:
> >  	return ret;
> >  }
> > @@ -115,10 +110,11 @@ out:
> >  int main(int argc, char **argv)
> >  {
> >  	char dev_name[20];
> > -	int ret, devid, help_flg;
> > +	int fd, ret, devid, help_flg;
> >  	uint32_t offset;
> >  	uint8_t val;
> >  	size_t count;
> > +	int file_op = O_RDONLY;
> >  
> >  	enum command {
> >  		INV = -1,
> > @@ -157,11 +153,12 @@ int main(int argc, char **argv)
> >  			break;
> >  		/* Command parsing */
> >  		case 1:
> > -			if (strcmp(optarg, "read") == 0)
> > +			if (strcmp(optarg, "read") == 0) {
> >  				cmd = READ;
> > -			else if (strcmp(optarg, "write") == 0)
> > +			} else if (strcmp(optarg, "write") == 0) {
> >  				cmd = WRITE;
> > -			break;
> > +				file_op = O_RDWR;
> > +			} break;
> >  		case ':':
> >  			igt_warn("The -%c option of %s requires an
> > argument\n",
> >  				 optopt, argv[0]);
> > @@ -185,20 +182,33 @@ int main(int argc, char **argv)
> >  	memset(dev_name, '\0', 20);
> >  	snprintf(dev_name, sizeof(aux_dev) + 2, "%s%d", aux_dev,
> > devid);
> >  
> > +	fd = open(dev_name, file_op);
> > +	if (fd < 0) {
> > +		igt_warn("Failed to open %s aux device - error:
> > %s\n", dev_name,
> > +			 strerror(errno));
> > +		return EXIT_FAILURE;
> > +	}
> > +
> >  	switch (cmd) {
> >  	case READ:
> >  		if (count == INVALID) {
> >  			igt_warn("Please specify the count in
> > bytes\n");
> >  			print_usage(argv[0], 0);
> >  		}
> > -		ret = dpcd_read(dev_name, offset, count);
> > +		ret = dpcd_read(fd, offset, count);
> > +		if (ret != count)
> > +			igt_warn("Failed to read from %s aux
> > device\n",
> > +				 dev_name);
> >  		break;
> >  	case WRITE:
> >  		if (val == INVALID) {
> >  			igt_warn("Write value is missing\n");
> >  			print_usage(argv[0], 0);
> >  		}
> > -		ret = dpcd_write(dev_name, offset, &val);
> > +		ret = dpcd_write(fd, offset, &val);
> > +		if (ret < 0)
> > +			igt_warn("Failed to write to %s aux
> > device\n",
> > +				 dev_name);
> >  		break;
> >  	case INV:
> >  	default:
> > @@ -206,5 +216,7 @@ int main(int argc, char **argv)
> >  		print_usage(argv[0], 0);
> >  	}
> >  
> > +	close(fd);
> > +
> >  	return ret;
> >  }
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev