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

[Tarun Vyas <tarun.vyas@intel.com>]

On Wed, Sep 05, 2018 at 11:17:59AM -0700, Rodrigo Vivi wrote:
> 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.
> 
> >
I tried multiple reads with pread() and it works OK. The doc mentions that the file offset will not be changed by pread() so all the offsets passed to it will be relative to the begining of the file.
> > > +	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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev