On Tue, Feb 06, 2024 at 12:02 PM +0800, Guenter Roeck wrote: > > 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 > Agree. Users should handle this by themselves. I'll revise it to align the behavior. Thanks Cosmo
