On 2/5/24 19:53, Cosmo Chou wrote:
On Tue, Feb 06, 2024 at 11:26 AM +0800, Guenter Roeck wrote:
On 2/5/24 19:05, Cosmo Chou wrote:
On Tue, Feb 06, 2024 at 3:43 AM +0800, Guenter Roeck wrote:
On Mon, Feb 05, 2024 at 11:20:13PM +0800, Cosmo Chou wrote:
This driver implements support for temperature monitoring of Astera Labs
PT5161L series PCIe retimer chips.
This driver implementation originates from the CSDK available at
Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14
The communication protocol utilized is based on the I2C/SMBus standard.
Signed-off-by: Cosmo Chou <chou.cosmo@xxxxxxxxx>
---
[ ... ]
+static ssize_t pt5161l_debugfs_read_fw_ver(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct pt5161l_data *data = file->private_data;
+ int ret;
+ char ver[32];
+
+ mutex_lock(&data->lock);
+ ret = pt5161l_fwsts_check(data);
+ mutex_unlock(&data->lock);
+ if (ret)
+ return ret;
+
+ ret = snprintf(ver, sizeof(ver), "%u.%u.%u\n", data->fw_ver.major,
+ data->fw_ver.minor, data->fw_ver.build);
+ if (ret < 0)
+ return ret;
+
You almost got me here ;-). snprintf() never returns a negative error code,
so checking for it is not necessary.
Oh! You're right.
+ return simple_read_from_buffer(buf, count, ppos, ver, ret + 1);
Number of bytes written plus 1 ? Why ?
It's just to include the string terminator '\0'.
If that was needed, it would be risky. snprintf() truncates the output
if the buffer is not large enough. You might want to consider using
scnprintf() instead. But then I am not sure if that is needed in the first
place. Almost all code I checked doesn't do that, and it seems to be likely
that the few drivers who do that are simply wrong. Can you explain why the
string terminator needs to be added to the output ?
Thanks,
Guenter
It's just in case someone reads and prints this, but with a dirty
buffer and doesn't handle the terminator.
That needs a better reason. It is not conceivable that 99% of drivers
don't do this but this one would need it for some reason. I am not going
to accept this unless you can show that debugfs files are supposed to
include a terminating '\0' in the response. This is like claiming that
printf() should include a terminating '\0' in the output just in case
the output is read by a broken application which needs to see the
terminator.
Guenter
