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 <[email protected]>
> ---
[ ... ]
> +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.
> + return simple_read_from_buffer(buf, count, ppos, ver, ret + 1);
Number of bytes written plus 1 ? Why ?
Thanks,
Guenter
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 <[email protected]>
> > ---
> [ ... ]
>
> > +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'.
>
> Thanks,
> Guenter
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 <[email protected]>
>>> ---
>> [ ... ]
>>
>>> +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
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 <[email protected]>
> >>> ---
> >> [ ... ]
> >>
> >>> +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.
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 <[email protected]>
>>>>> ---
>>>> [ ... ]
>>>>
>>>>> +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
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 <[email protected]>
> >>>>> ---
> >>>> [ ... ]
> >>>>
> >>>>> +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