Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe()
On Fri, 30 Oct, 2020, 1:32 PM Greg KH, wrote: > On Fri, Oct 30, 2020 at 01:27:16PM +0530, Deepak R Varma wrote: > > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with > debugfs_create_file_unsafe() > > > > function in place of the debugfs_create_file() function will make the > > > > file operation struct "reset" aware of the file's lifetime. > Additional > > > > details here: > https://lists.archive.carbon60.com/linux/kernel/2369498 > > > > > > > > Issue reported by Coccinelle script: > > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > > > Signed-off-by: Deepak R Varma > > > > --- > > > > Please Note: This is a Outreachy project task patch. > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 > ++-- > > > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > index 2d125b8b15ee..f076b1ba7319 100644 > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void > *data, u64 val) > > > > return 0; > > > > } > > > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, > > > > - amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, > > > > + amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > > > Are you sure this is ok? Do these devices need this additional > > > "protection"? Do they have the problem that these macros were written > > > for? > > > > > > Same for the other patches you just submitted here, I think you need to > > > somehow "prove" that these changes are necessary, checkpatch isn't able > > > to determine this all the time. > > > > Hi Greg, > > Based on my understanding, the current function debugfs_create_file() > > adds an overhead of lifetime managing proxy for such fop structs. This > > should be applicable to these set of drivers as well. Hence I think this > > change will be useful. > > Why do these drivers need these changes? Are these files dynamically > removed from the system at random times? > > There is a reason we didn't just do a global search/replace for this in > the kernel when the new functions were added, so I don't know why > checkpatch is now saying it must be done. > Hi, Sorry to jump in on the thread this way, but what exactly does a 'lifetime managing proxy' for file operations mean? I am trying to understand how DEFINE_DEBUGFS_ATTRIBUTE changes things wrt debug_ fs file operations but can't find many resources. :( Please let me know if I should be asking this in a different mailing list/irc instead. The change seems to be suggested by a coccinelle script. Regards, Sumera thanks, > > greg k-h > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20201030080316.GA1612206%40kroah.com > . > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[Outreachy][PATCH] drm/amdgpu: use true and false for bool initialisations
Bool initialisation should use 'true' and 'false' values instead of 0 and 1. Modify amdgpu_amdkfd_gpuvm.c to initialise variable is_imported to false instead of 0. Issue found with Coccinelle. Signed-off-by: Sumera Priyadarsini --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 64d4b5ff95d6..ba4bd06bfcc5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -1288,7 +1288,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu( struct ttm_validate_buffer *bo_list_entry; unsigned int mapped_to_gpu_memory; int ret; - bool is_imported = 0; + bool is_imported = false; mutex_lock(>lock); mapped_to_gpu_memory = mem->mapped_to_gpu_memory; -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Outreachy kernel] [PATCH 4/5] gpu: drm: amdgpu: Replace snprintf() with sysfs_emit()
On Thu, Oct 22, 2020 at 7:24 PM Greg KH wrote: > On Thu, Oct 22, 2020 at 07:17:56PM +0530, Sumera Priyadarsini wrote: > > Using snprintf() for show() methods holds the risk of buffer overrun > > as snprintf() does not know the PAGE_SIZE maximum of the temporary > > buffer used to output sysfs content. > > > > Modify amdgpu_psp.c to use sysfs_emit() instead which knows the > > size of the temporary buffer. > > > > Issue found with Coccinelle. > > > > Signed-off-by: Sumera Priyadarsini > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c > > index d6c38e24f130..4d1d1e1b005d 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c > > @@ -2621,7 +2621,7 @@ static ssize_t psp_usbc_pd_fw_sysfs_read(struct > device *dev, > > return ret; > > } > > > > - return snprintf(buf, PAGE_SIZE, "%x\n", fw_ver); > > + return sysfs_emit(buf, PAGE_SIZE, "%x\n", fw_ver); > > Did you build this code? I don't think it is correct... > Yes, you are right. I compiled all of them again separately. I had based them off the usual drm tree but that is wrong because sysfs_emit has been added only in the 5.10. I will send a v2 with the proper corrections. regards, sumera > thanks, > > greg k-h > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 5/5] gpu: drm: amdgpu: Replace snprintf() with sysfs_emit()
Using snprintf() for show() methods holds the risk of buffer overrun as snprintf() does not know the PAGE_SIZE maximum of the temporary buffer used to output sysfs content. Modify amdgpu_ras.c to use sysfs_emit() instead which knows the size of the temporary buffer. Issue found with Coccinelle. Signed-off-by: Sumera Priyadarsini --- drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c index e5ea14774c0c..6d9901e1b4b0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c @@ -429,13 +429,13 @@ static ssize_t amdgpu_ras_sysfs_read(struct device *dev, }; if (!amdgpu_ras_get_error_query_ready(obj->adev)) - return snprintf(buf, PAGE_SIZE, + return sysfs_emit(buf, PAGE_SIZE, "Query currently inaccessible\n"); if (amdgpu_ras_error_query(obj->adev, )) return -EINVAL; - return snprintf(buf, PAGE_SIZE, "%s: %lu\n%s: %lu\n", + return sysfs_emit(buf, PAGE_SIZE, "%s: %lu\n%s: %lu\n", "ue", info.ue_count, "ce", info.ce_count); } -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[Outreachy kernel][PATCH 0/5] drm/amdgpu: Replace snprintf() with sysfs_emit
Using snprintf() for show() methods holds the risk of buffer overrun as snprintf() does not know the PAGE_SIZE maximum of the temporary buffer used to output sysfs content. This patchset is a series of Coccinelle cleanups across the staging directory to convert snprintf with scnprintf in the relevant files. Sumera Priyadarsini (5): gpu: drm: amdgpu: Replace snprintf() with sysfs_emit() gpu: drm: amdgpu: Replace snprintf() with sysfs_emit() gpu: drm: amdgpu: Replace snprintf() with sysfs_emit() gpu: drm: amdgpu: Replace snprintf() with sysfs_emit() gpu: drm: amdgpu: Replace snprintf() with sysfs_emit() drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 4 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/5] gpu: drm: amdgpu: Replace snprintf() with sysfs_emit()
Using snprintf() for show() methods holds the risk of buffer overrun as snprintf() does not know the PAGE_SIZE maximum of the temporary buffer used to output sysfs content. Modify amdgpu_device.c to use sysfs_emit() instead which knows the size of the temporary buffer. Issue found with Coccinelle. Signed-off-by: Sumera Priyadarsini --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index f7307af76452..7eef6b20578f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -135,7 +135,7 @@ static ssize_t amdgpu_device_get_pcie_replay_count(struct device *dev, struct amdgpu_device *adev = drm_to_adev(ddev); uint64_t cnt = amdgpu_asic_get_pcie_replay_count(adev); - return snprintf(buf, PAGE_SIZE, "%llu\n", cnt); + return sysfs_emit(buf, PAGE_SIZE, "%llu\n", cnt); } static DEVICE_ATTR(pcie_replay_count, S_IRUGO, @@ -159,7 +159,7 @@ static ssize_t amdgpu_device_get_product_name(struct device *dev, struct drm_device *ddev = dev_get_drvdata(dev); struct amdgpu_device *adev = drm_to_adev(ddev); - return snprintf(buf, PAGE_SIZE, "%s\n", adev->product_name); + return sysfs_emit(buf, PAGE_SIZE, "%s\n", adev->product_name); } static DEVICE_ATTR(product_name, S_IRUGO, @@ -181,7 +181,7 @@ static ssize_t amdgpu_device_get_product_number(struct device *dev, struct drm_device *ddev = dev_get_drvdata(dev); struct amdgpu_device *adev = drm_to_adev(ddev); - return snprintf(buf, PAGE_SIZE, "%s\n", adev->product_number); + return sysfs_emit(buf, PAGE_SIZE, "%s\n", adev->product_number); } static DEVICE_ATTR(product_number, S_IRUGO, @@ -203,7 +203,7 @@ static ssize_t amdgpu_device_get_serial_number(struct device *dev, struct drm_device *ddev = dev_get_drvdata(dev); struct amdgpu_device *adev = drm_to_adev(ddev); - return snprintf(buf, PAGE_SIZE, "%s\n", adev->serial); + return sysfs_emit(buf, PAGE_SIZE, "%s\n", adev->serial); } static DEVICE_ATTR(serial_number, S_IRUGO, -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 4/5] gpu: drm: amdgpu: Replace snprintf() with sysfs_emit()
Using snprintf() for show() methods holds the risk of buffer overrun as snprintf() does not know the PAGE_SIZE maximum of the temporary buffer used to output sysfs content. Modify amdgpu_psp.c to use sysfs_emit() instead which knows the size of the temporary buffer. Issue found with Coccinelle. Signed-off-by: Sumera Priyadarsini --- drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c index d6c38e24f130..4d1d1e1b005d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c @@ -2621,7 +2621,7 @@ static ssize_t psp_usbc_pd_fw_sysfs_read(struct device *dev, return ret; } - return snprintf(buf, PAGE_SIZE, "%x\n", fw_ver); + return sysfs_emit(buf, PAGE_SIZE, "%x\n", fw_ver); } static ssize_t psp_usbc_pd_fw_sysfs_write(struct device *dev, -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/5] gpu: drm: amdgpu: Replace snprintf() with sysfs_emit()
Using snprintf() for show() methods holds the risk of buffer overrun as snprintf() does not know the PAGE_SIZE maximum of the temporary buffer used to output sysfs content. Modify amdgpu_atombios.c to use sysfs_emit() instead which knows the size of the temporary buffer. Issue found with Coccinelle. Signed-off-by: Sumera Priyadarsini --- drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c index 469352e2d6ec..3c19862c94c7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c @@ -1947,7 +1947,7 @@ static ssize_t amdgpu_atombios_get_vbios_version(struct device *dev, struct amdgpu_device *adev = drm_to_adev(ddev); struct atom_context *ctx = adev->mode_info.atom_context; - return snprintf(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); + return sysfs_emit(buf, PAGE_SIZE, "%s\n", ctx->vbios_version); } static DEVICE_ATTR(vbios_version, 0444, amdgpu_atombios_get_vbios_version, -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 3/5] gpu: drm: amdgpu: Replace snprintf() with sysfs_emit()
Using snprintf() for show() methods holds the risk of buffer overrun as snprintf() does not know the PAGE_SIZE maximum of the temporary buffer used to output sysfs content. Modify amdgpu_gtt_mgr.c to use sysfs_emit() instead which knows the size of the temporary buffer. Issue found with Coccinelle. Signed-off-by: Sumera Priyadarsini --- drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c index 1721739def84..441e07ee1967 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c @@ -49,7 +49,7 @@ static ssize_t amdgpu_mem_info_gtt_total_show(struct device *dev, struct amdgpu_device *adev = drm_to_adev(ddev); struct ttm_resource_manager *man = ttm_manager_type(>mman.bdev, TTM_PL_TT); - return snprintf(buf, PAGE_SIZE, "%llu\n", + return sysfs_emit(buf, PAGE_SIZE, "%llu\n", man->size * PAGE_SIZE); } @@ -68,7 +68,7 @@ static ssize_t amdgpu_mem_info_gtt_used_show(struct device *dev, struct amdgpu_device *adev = drm_to_adev(ddev); struct ttm_resource_manager *man = ttm_manager_type(>mman.bdev, TTM_PL_TT); - return snprintf(buf, PAGE_SIZE, "%llu\n", + return sysfs_emit(buf, PAGE_SIZE, "%llu\n", amdgpu_gtt_mgr_usage(man)); } -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[Outreachy kernel][PATCH] gpu: amd: Return boolean types instead of integer values
Return statements for functions returning bool should use truth and false instead of 1 and 0 respectively. Modify cik_event_interrupt.c to return false instead of 0. Issue found with Coccinelle. Signed-off-by: Sumera Priyadarsini --- drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c index 24b471734117..8e64c01565ac 100644 --- a/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c +++ b/drivers/gpu/drm/amd/amdkfd/cik_event_interrupt.c @@ -66,12 +66,12 @@ static bool cik_event_interrupt_isr(struct kfd_dev *dev, vmid = (ihre->ring_id & 0xff00) >> 8; if (vmid < dev->vm_info.first_vmid_kfd || vmid > dev->vm_info.last_vmid_kfd) - return 0; + return false; /* If there is no valid PASID, it's likely a firmware bug */ pasid = (ihre->ring_id & 0x) >> 16; if (WARN_ONCE(pasid == 0, "FW bug: No PASID in KFD interrupt")) - return 0; + return false; /* Interrupt types we care about: various signals and faults. * They will be forwarded to a work queue (see below). -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
