Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings

"Lazar, Lijo" <lijo.lazar@xxxxxxx> · Thu, 9 Sep 2021 14:06:57 +0530

On 9/9/2021 1:31 PM, Christian König wrote:
Am 09.09.21 um 05:28 schrieb Lazar, Lijo:
On 9/9/2021 8:13 AM, Yu, Lang wrote:
[AMD Official Use Only]

So the final decision is rollback to scnprintf().

If we can define our own helper functions like sysfs_emit/sysfs_emit_at

but without page boundary aligned limitation to make life easier?

No, we do want to make it clear that this function is used for sysfs 
files and make use of the extra checks provided by the sysfs_emit* 
functions. Looking at the origins of sysf_emit_at() specifically, 
there are indeed some cases of printing more than one values per file 
and multi-line usage.

Correct, but those are rather limited and well documented special cases. 
E.g. for example if you need to grab a lock to get multiple values which 
are supposed to be coherent to each other.

I think that's the case here, so printing multiple values is probably ok 
in general. But we still need to get the implementation straight.

So I'm fine with your original patch. Maybe, you can make the 
intention explicit by keeping the offset and buf start calculations in 
a separate inline function.
    smu_get_sysfs_buf()

Exactly that is what is not ok. So once more the intended use case of 
those functions is:

offs = sysfs_emit(page, ...);
offs += sysfs_emit_at(page, offs, ....);
offs += sysfs_emit_at(page, offs, ....);
...

Another possible alternative which I think should be allowed is:

offs = 0;
for_each_clock_in_my_device(..) {
     offs += sysfs_emit_at(page, offs, ....);
}

But when you are calculating the initial offset manually then there is 
certainly something wrong here and that is not the intended usage pattern.

Actually, the issue is not within one function invocation. The issue is 
at the caller side with multiple invocations -

                 size = amdgpu_dpm_print_clock_levels(adev, OD_SCLK, buf);
                 size += amdgpu_dpm_print_clock_levels(adev, OD_MCLK,
buf+size);

Having amdgpu_dpm_print_clock_levels() helped to consolidate sysfs calls 
in single function for different parameters and used for different 
nodes. However in this case, different parameters are presented as a 
single "logical entity" in a sysfs node and the function is called 
multiple times for different parameters (questionable as per sysfs 
guidelines, Alex needs to come back on this).

Within one invocation of amdgpu_dpm_print_clock_levels(), the APIs are 
used correctly. For the second call, it needs to pass the page aligned 
buf pointer correctly to sysfs_emit* calls.

Presently, amdgpu_dpm_print_clock_levels() takes only buff pointer as 
argument and probably it is that way since the function existed before 
sysfs_emit* patches got added and was originally using sprintfs.

Now, two possible options are -

1) Make a pre-requisite that this function is always going to print to 
sysfs files. For that use sysfs_emit*. Also, as with a sysfs buffer 
calculate the page aligned start address of buf and offset for use with 
sysfs_emit* in the beginning. At least for now, this assumption is 
inline with the buffer start address requirement in sysfs_emit* patches. 
This is what the original patch does. That said, if the buffer 
properties change in future this will not hold good.

2) Pass the offset along with the buff in API. That will be extensive 
since it affects older powerplay based HWs also.

There may be other ways, but those could be even more extensive than 2).

Thanks,
Lijo

Regards,
Christian.

Thanks,
Lijo

Regards,

Lang

*From:* Powell, Darren <Darren.Powell@xxxxxxx>
*Sent:* Thursday, September 9, 2021 6:18 AM
*To:* Christian König <ckoenig.leichtzumerken@xxxxxxxxx>; Lazar, Lijo 
<Lijo.Lazar@xxxxxxx>; Yu, Lang <Lang.Yu@xxxxxxx>; 
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
*Cc:* Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Huang, Ray 
<Ray.Huang@xxxxxxx>; Tian Tao <tiantao6@xxxxxxxxxxxxx>
*Subject:* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings

[AMD Official Use Only]

------------------------------------------------------------------------

*From:*Christian König <ckoenig.leichtzumerken@xxxxxxxxx 
<mailto:ckoenig.leichtzumerken@xxxxxxxxx>>
*Sent:* Wednesday, September 8, 2021 8:43 AM
*To:* Lazar, Lijo <Lijo.Lazar@xxxxxxx <mailto:Lijo.Lazar@xxxxxxx>>; 
Yu, Lang <Lang.Yu@xxxxxxx <mailto:Lang.Yu@xxxxxxx>>; 
amd-gfx@xxxxxxxxxxxxxxxxxxxxx <mailto:amd-gfx@xxxxxxxxxxxxxxxxxxxxx> 
<amd-gfx@xxxxxxxxxxxxxxxxxxxxx <mailto:amd-gfx@xxxxxxxxxxxxxxxxxxxxx>>
*Cc:* Deucher, Alexander <Alexander.Deucher@xxxxxxx 
<mailto:Alexander.Deucher@xxxxxxx>>; Huang, Ray <Ray.Huang@xxxxxxx 
<mailto:Ray.Huang@xxxxxxx>>; Tian Tao <tiantao6@xxxxxxxxxxxxx 
<mailto:tiantao6@xxxxxxxxxxxxx>>; Powell, Darren 
<Darren.Powell@xxxxxxx <mailto:Darren.Powell@xxxxxxx>>
*Subject:* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings

Am 08.09.21 um 12:22 schrieb Lazar, Lijo:
 > On 9/8/2021 3:08 PM, Christian König wrote:
 >> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
 >>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
 >>>> [AMD Official Use Only]
 >>>>> -----Original Message-----
 >>>>> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx 
<mailto:Lijo.Lazar@xxxxxxx>>
 >>>>> Sent: Wednesday, September 8, 2021 4:55 PM
 >>>>> To: Yu, Lang <Lang.Yu@xxxxxxx <mailto:Lang.Yu@xxxxxxx>>; 
Christian König
 >>>>> <ckoenig.leichtzumerken@xxxxxxxxx 
<mailto:ckoenig.leichtzumerken@xxxxxxxxx>>; 
amd-gfx@xxxxxxxxxxxxxxxxxxxxx <mailto:amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
 >>>>> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx 
<mailto:Alexander.Deucher@xxxxxxx>>; Huang, Ray
 >>>>> <Ray.Huang@xxxxxxx <mailto:Ray.Huang@xxxxxxx>>; Tian Tao 
<tiantao6@xxxxxxxxxxxxx <mailto:tiantao6@xxxxxxxxxxxxx>>
 >>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
 >>>>> warnings
 >>>>>
 >>>>>
 >>>>>
 >>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
 >>>>>> [AMD Official Use Only]
 >>>>>>
 >>>>>>
 >>>>>>
 >>>>>>> -----Original Message-----
 >>>>>>> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx 
<mailto:Lijo.Lazar@xxxxxxx>>
 >>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
 >>>>>>> To: Christian König <ckoenig.leichtzumerken@xxxxxxxxx 
<mailto:ckoenig.leichtzumerken@xxxxxxxxx>>; Yu, Lang
 >>>>>>> <Lang.Yu@xxxxxxx <mailto:Lang.Yu@xxxxxxx>>; 
amd-gfx@xxxxxxxxxxxxxxxxxxxxx <mailto:amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
 >>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx 
<mailto:Alexander.Deucher@xxxxxxx>>; Huang, Ray
 >>>>>>> <Ray.Huang@xxxxxxx <mailto:Ray.Huang@xxxxxxx>>; Tian Tao 
<tiantao6@xxxxxxxxxxxxx <mailto:tiantao6@xxxxxxxxxxxxx>>
 >>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
 >>>>>>> warnings
 >>>>>>>
 >>>>>>>
 >>>>>>>
 >>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
 >>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
 >>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary 
aligned buf
 >>>>>>>>> address. Make them happy!
 >>>>>>>>>
 >>>>>>>>> Warning Log:
 >>>>>>>>> [  492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde 
at:0 [
 >>>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
 >>>>>>>>> sysfs_emit_at+0x4a/0xa0
 >>>>>>>>> [  492.654805] Call Trace:
 >>>>>>>>> [  492.655353]  ? smu_cmn_get_metrics_table+0x40/0x50 
[amdgpu] [
 >>>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
 >>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 
[amdgpu] [
 >>>>>>>>> 492.659733]  ? preempt_schedule_common+0x18/0x30 [ 
492.660713]
 >>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
 >>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 
492.663620]
 >>>>>>>>> dev_attr_show+0x1d/0x40
 >>>>>>>>
 >>>>>>>> Mhm, that at least partially doesn't looks like the right
 >>>>>>>> approach to me.
 >>>>>>>>
 >>>>>>>> Why do we have string printing and sysfs code in the hardware
 >>>>>>>> version specific backend in the first place?
 >>>>>>>>
 >>>>>>>
 >>>>>>> This is a callback meant for printing ASIC specific 
information to
 >>>>>>> sysfs node. The buffer passed in sysfs read is passed as it is
 >>>>>>> to the callback API.
 >>>>>>>
 >>>>>>>> That stuff needs to be implemented for each hardware 
generation
 >>>>>>>> and
 >>>>>>>> is now cluttered with sysfs buffer offset calculations.
 >>>>>>>>
 >>>>>>>
 >>>>>>> Looks like the warning happened because of this usage.
 >>>>>>>
 >>>>>>>                   size = amdgpu_dpm_print_clock_levels(adev,
 >>>>>>> OD_SCLK, buf);
 >>>>>>>                   size += amdgpu_dpm_print_clock_levels(adev,
 >>>>>>> OD_MCLK,
 >>>>>>> buf+size);
 >>>>>>>                   size += amdgpu_dpm_print_clock_levels(adev,
 >>>>>>> OD_VDDC_CURVE, buf+size);
 >>>>>>>                   size += amdgpu_dpm_print_clock_levels(adev,
 >>>>>>> OD_VDDGFX_OFFSET, buf+size);
 >>>>>>>                   size += amdgpu_dpm_print_clock_levels(adev,
 >>>>>>> OD_RANGE,
 >>>>>>> buf+size);
 >>>>>>>                   size += amdgpu_dpm_print_clock_levels(adev,
 >>>>>>> OD_CCLK,
 >>>>>>> buf+size);
 >>>>>>>
 >>>>>>>
 >>>>>> [Yu, Lang]
 >>>>>> Yes. So it is fine we just fix the caller
 >>>>>> amdgpu_get_pp_od_clk_voltage like
 >>>>> following:
 >>>>>>
 >>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
 >>>>>>         struct device_attribute *attr,
 >>>>>>         char *buf)
 >>>>>> {
 >>>>>>     struct drm_device *ddev = dev_get_drvdata(dev);
 >>>>>>     struct amdgpu_device *adev = drm_to_adev(ddev);
 >>>>>>     ssize_t size, offset;
 >>>>>>     int ret, i;
 >>>>>>     char temp_buf[512];
 >>>>>>     char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
 >>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
 >>>>>>
 >>>>>>     if (amdgpu_in_reset(adev))
 >>>>>>         return -EPERM;
 >>>>>>     if (adev->in_suspend && !adev->in_runpm)
 >>>>>>         return -EPERM;
 >>>>>>
 >>>>>>     ret = pm_runtime_get_sync(ddev->dev);
 >>>>>>     if (ret < 0) {
 >>>>>> pm_runtime_put_autosuspend(ddev->dev);
 >>>>>>         return ret;
 >>>>>>     }
 >>>>>>
 >>>>>>     offset = 0;
 >>>>>>
 >>>>>>     if (adev->powerplay.pp_funcs->print_clock_levels) {
 >>>>>>         for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
 >>>>>>             size = amdgpu_dpm_print_clock_levels(adev,
 >>>>> clock_type[i], buf);
 >>>>>>             if (offset + size > PAGE_SIZE)
 >>>>>>                 break;
 >>>>>>             memcpy(temp_buf + offset, buf, size);
 >>>>>>             offset += size;
 >>>>>>         }
 >>>>>>         memcpy(buf, temp_buf, offset);
 >>>>>>         size = offset;
 >>>>>>     } else {
 >>>>>>         size = sysfs_emit(buf, "\n");
 >>>>>>     }
 >>>>>>     pm_runtime_mark_last_busy(ddev->dev);
 >>>>>>     pm_runtime_put_autosuspend(ddev->dev);
 >>>>>>
 >>>>>>     return size;
 >>>>>> }
 >>>>>>
 >>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
 >>>>> another arg to
 >>>>> pass offset along with buf?
 >>>>>
 >>>> [Yu, Lang]
 >>>> Actually, the buf address contains the 
offset(offset_in_page(buf)) .
 >>>
 >>> Though it's not a problem based on codeflow, static analysis tools
 >>> might complain.
 >>>
 >>>> Or we just rollback to sprintf/snprintf.
 >>>>
 >>>
 >>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took
 >>> the effort to convert these, he may have some other ideas.
The changes I made were intended to simply replace snprintf with 
sysfs_emit as per

linux/Documentation/filesystems/sysfs.rst:246,247

  -  show() should only use sysfs_emit() or sysfs_emit_at() when 
formatting

     the value to be returned to user space.

I specifically tried not to change the design, but only as I didn't have

background in Power Management.

 >>
 >> This is not what I meant. See from the design point of view the
 >> print_clock_levels() callback is the bad idea to begin with.
 >>
 >> What we should have instead is a callback which returns the clock as
 >> a value which is then printed in the amdgpu_get_pp_od_clk_voltage()
 >> function.
 >>
 >> This avoids passing around the buffer and remaining size everywhere
 >> and also guarantees that the sysfs have unified printing over all
 >> hardware generations.
 >>
 >
 > The scenario is one node used for multiple parameters - OD_SCLK,
 > OD_CCLK, OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with
 > lots of nodes). On top of it, the parameters supported (for ex: CCLK
 > is not valid on dGPUs),  the number of levels supported etc. vary
 > across ASICs. There has to be multiple calls or the call needs to
 > return multiple values for a single parameter (for ex: up to 4, 8 or
 > 16 levels of GFXCLK depending on ASIC).

Well exactly that is questionable design for sysfs.

See the sysfs_emit() and sysfs_emit_at() functions are designed the way
they are because you should have only one value per file, which is then
printed at exactly one location.

Take a look at the documentation for sysfs for more details.

 > I don't know the history of the callback, mostly it was considered
 > more efficient to print it directly rather than fetch and print.
 > Alex/Evan may know the details.

Yeah, somebody with a bit more background in power management needs to
take a closer look at this here. Just keep me looped in.

Regards,
Christian.

 >
 > Thanks,
 > Lijo
 >
 >> Regards,
 >> Christian.
 >>