Re: [PATCH v5 1/1] hwmon: Add driver for Astera Labs PT5161L retimer

[Guenter Roeck]

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