Re: [PATCH v5 02/13] iio: core: support 64-bit register through debugfs

[Rodrigo Alencar]

On 26/05/17 06:56PM, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Silent truncation of 64-bit values when written to a 32-bit register access function.
> --
> 
> commit 64785a0c1416e2cf718b558aa7a2cc9ac39b2d81
> Author: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> iio: core: support 64-bit register through debugfs
> 
> This commit introduces a debugfs_reg64_access function pointer to iio_info
> to allow 64-bit register reads and writes through debugfs. The file operation
> callbacks are modified to prefer the 64-bit variant when available.
> 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index e587aca79b8e7..5c8404efd0a56 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> 
> [ ... ]
> 
> > @@ -429,7 +439,7 @@ static ssize_t iio_debugfs_write_reg(struct file *file,
> >  
> >  	buf[ret] = '\0';
> >  
> > -	ret = sscanf(buf, "%i %i", &reg, &val);
> > +	ret = sscanf(buf, "%i %lli", &reg, &val64);
> >  
> >  	switch (ret) {
> >  	case 1:
> > @@ -437,8 +447,12 @@ static ssize_t iio_debugfs_write_reg(struct file *file,
> >  		break;
> >  	case 2:
> >  		iio_dev_opaque->cached_reg_addr = reg;
> > -		ret = indio_dev->info->debugfs_reg_access(indio_dev, reg,
> > -							  val, NULL);
> > +		if (indio_dev->info->debugfs_reg64_access)
> > +			ret = indio_dev->info->debugfs_reg64_access(indio_dev, reg,
> > +								    val64, NULL);
> > +		else
> > +			ret = indio_dev->info->debugfs_reg_access(indio_dev, reg,
> > +								  val64, NULL);
> 
> Could this introduce a regression where values are silently truncated if the
> provided input exceeds 32 bits?
> 
> Since the input is now unconditionally parsed as a 64-bit integer, falling
> back to debugfs_reg_access (which takes an unsigned int for writeval) will
> implicitly cast val64. If a user provides a value larger than UINT_MAX to a
> driver that only provides the 32-bit callback, it appears the value will be
> truncated rather than rejected with an error like -EINVAL.

Maybe -EINVAL can be returned for that case indeed. For a debug interface I
did not see that as a problem.
 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260517-ad9910-iio-driver-v5-0-31599c88314a@xxxxxxxxxx?part=2

-- 
Kind regards,

Rodrigo Alencar