[PATCH] drm/amdgpu: Fix ENOSYS means 'invalid syscall nr' in amdgpu_device.c

Russell, Kent Kent.Russell at amd.com
Tue Jul 25 13:44:24 UTC 2023


[Public]

The S_IRUGO stuff be split from the ENOSYS stuff, since they're separate. Also the S_IRUGO stuff isn't mentioned in the commit message.

 Kent

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Chen,
> Guchun
> Sent: Tuesday, July 25, 2023 1:06 AM
> To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM at amd.com>;
> Koenig, Christian <Christian.Koenig at amd.com>; Deucher, Alexander
> <Alexander.Deucher at amd.com>
> Cc: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM at amd.com>; amd-
> gfx at lists.freedesktop.org
> Subject: RE: [PATCH] drm/amdgpu: Fix ENOSYS means 'invalid syscall nr' in
> amdgpu_device.c
>
> [Public]
>
> [Public]
>
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
> > Srinivasan Shanmugam
> > Sent: Sunday, July 23, 2023 2:16 PM
> > To: Koenig, Christian <Christian.Koenig at amd.com>; Deucher, Alexander
> > <Alexander.Deucher at amd.com>
> > Cc: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM at amd.com>;
> > amd-gfx at lists.freedesktop.org
> > Subject: [PATCH] drm/amdgpu: Fix ENOSYS means 'invalid syscall nr' in
> > amdgpu_device.c
> >
> > ENOSYS should be used for nonexistent syscalls only, replace ENOSYS with
> > EINVAL & other style fixes
> >
> > WARNING: ENOSYS means 'invalid syscall nr' and nothing else
> > +       if (r == -ENOSYS)
> >
> > WARNING: ENOSYS means 'invalid syscall nr' and nothing else
> > +       if (r == -ENOSYS)
> >
> > Cc: Christian König <christian.koenig at amd.com>
> > Cc: Alex Deucher <alexander.deucher at amd.com>
> > Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
>
> Reviewed-by: Guchun Chen <guchun.chen at amd.com>
>
> Regards,
> Guchun
>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 60 +++++++++++-----------
> > drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c  |  4 +-
> >  2 files changed, 33 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 1c786190a84e..ec166c797b9a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -159,7 +159,7 @@ static ssize_t
> > amdgpu_device_get_pcie_replay_count(struct device *dev,
> >       return sysfs_emit(buf, "%llu\n", cnt);  }
> >
> > -static DEVICE_ATTR(pcie_replay_count, S_IRUGO,
> > +static DEVICE_ATTR(pcie_replay_count, 0444,
> >               amdgpu_device_get_pcie_replay_count, NULL);
> >
> >  static void amdgpu_device_get_pcie_info(struct amdgpu_device *adev);
> > @@ -183,7 +183,7 @@ static ssize_t
> > amdgpu_device_get_product_name(struct device *dev,
> >       return sysfs_emit(buf, "%s\n", adev->product_name);  }
> >
> > -static DEVICE_ATTR(product_name, S_IRUGO,
> > +static DEVICE_ATTR(product_name, 0444,
> >               amdgpu_device_get_product_name, NULL);
> >
> >  /**
> > @@ -205,7 +205,7 @@ static ssize_t
> > amdgpu_device_get_product_number(struct device *dev,
> >       return sysfs_emit(buf, "%s\n", adev->product_number);  }
> >
> > -static DEVICE_ATTR(product_number, S_IRUGO,
> > +static DEVICE_ATTR(product_number, 0444,
> >               amdgpu_device_get_product_number, NULL);
> >
> >  /**
> > @@ -227,7 +227,7 @@ static ssize_t
> > amdgpu_device_get_serial_number(struct device *dev,
> >       return sysfs_emit(buf, "%s\n", adev->serial);  }
> >
> > -static DEVICE_ATTR(serial_number, S_IRUGO,
> > +static DEVICE_ATTR(serial_number, 0444,
> >               amdgpu_device_get_serial_number, NULL);
> >
> >  /**
> > @@ -481,8 +481,7 @@ uint32_t amdgpu_device_rreg(struct amdgpu_device
> > *adev,
> >  /*
> >   * MMIO register read with bytes helper functions
> >   * @offset:bytes offset from MMIO start
> > - *
> > -*/
> > + */
> >
> >  /**
> >   * amdgpu_mm_rreg8 - read a memory mapped IO register @@ -506,8
> > +505,8 @@ uint8_t amdgpu_mm_rreg8(struct amdgpu_device *adev,
> > uint32_t offset)
> >   * MMIO register write with bytes helper functions
> >   * @offset:bytes offset from MMIO start
> >   * @value: the value want to be written to the register
> > - *
> > -*/
> > + */
> > +
> >  /**
> >   * amdgpu_mm_wreg8 - read a memory mapped IO register
> >   *
> > @@ -991,7 +990,7 @@ static void amdgpu_device_mem_scratch_fini(struct
> > amdgpu_device *adev)
> >   * @registers: pointer to the register array
> >   * @array_size: size of the register array
> >   *
> > - * Programs an array or registers with and and or masks.
> > + * Programs an array or registers with and or masks.
> >   * This is a helper for setting golden registers.
> >   */
> >  void amdgpu_device_program_register_sequence(struct amdgpu_device
> > *adev, @@ -1157,7 +1156,7 @@ int amdgpu_device_resize_fb_bar(struct
> > amdgpu_device *adev)
> >       int rbar_size = pci_rebar_bytes_to_size(adev->gmc.real_vram_size);
> >       struct pci_bus *root;
> >       struct resource *res;
> > -     unsigned i;
> > +     unsigned int i;
> >       u16 cmd;
> >       int r;
> >
> > @@ -1226,9 +1225,8 @@ int amdgpu_device_resize_fb_bar(struct
> > amdgpu_device *adev)
> >
> >  static bool amdgpu_device_read_bios(struct amdgpu_device *adev)  {
> > -     if (hweight32(adev->aid_mask) && (adev->flags & AMD_IS_APU)) {
> > +     if (hweight32(adev->aid_mask) && (adev->flags & AMD_IS_APU))
> >               return false;
> > -     }
> >
> >       return true;
> >  }
> > @@ -1264,6 +1262,7 @@ bool amdgpu_device_need_post(struct
> > amdgpu_device *adev)
> >               if (adev->asic_type == CHIP_FIJI) {
> >                       int err;
> >                       uint32_t fw_ver;
> > +
> >                       err = request_firmware(&adev->pm.fw,
> > "amdgpu/fiji_smc.bin", adev->dev);
> >                       /* force vPost if error occured */
> >                       if (err)
> > @@ -1366,6 +1365,7 @@ static unsigned int
> > amdgpu_device_vga_set_decode(struct pci_dev *pdev,
> >               bool state)
> >  {
> >       struct amdgpu_device *adev = drm_to_adev(pci_get_drvdata(pdev));
> > +
> >       amdgpu_asic_set_vga_state(adev, state);
> >       if (state)
> >               return VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
> > @@ -1388,7 +1388,8 @@ static void
> > amdgpu_device_check_block_size(struct amdgpu_device *adev)  {
> >       /* defines number of bits in page table versus page directory,
> >        * a page is 4KB so we have 12 bits offset, minimum 9 bits in the
> > -      * page table and the remaining bits are in the page directory */
> > +      * page table and the remaining bits are in the page directory
> > +      */
> >       if (amdgpu_vm_block_size == -1)
> >               return;
> >
> > @@ -1620,7 +1621,7 @@ static bool amdgpu_switcheroo_can_switch(struct
> > pci_dev *pdev)  {
> >       struct drm_device *dev = pci_get_drvdata(pdev);
> >
> > -     /*
> > +       /*
> >       * FIXME: open_count is protected by drm_global_mutex but that
> > would lead to
> >       * locking inversion with the driver load path. And the access here is
> >       * completely racy anyway. So don't bother with locking for now.
> > @@ -3265,7 +3266,7 @@ static int
> > amdgpu_device_ip_resume_phase2(struct amdgpu_device *adev)
> >   *
> >   * Main resume function for hardware IPs.  The hardware IPs
> >   * are split into two resume functions because they are
> > - * are also used in in recovering from a GPU reset and some additional
> > + * also used in recovering from a GPU reset and some additional
> >   * steps need to be take between them.  In this case (S3/S4) they are
> >   * run sequentially.
> >   * Returns 0 on success, negative error code on failure.
> > @@ -3367,8 +3368,7 @@ bool amdgpu_device_asic_has_dc_support(enum
> > amd_asic_type asic_type)  #else
> >       default:
> >               if (amdgpu_dc > 0)
> > -                     DRM_INFO_ONCE("Display Core has been requested
> > via kernel parameter "
> > -                                      "but isn't supported by ASIC,
> > ignoring\n");
> > +                     DRM_INFO_ONCE("Display Core has been requested
> > via kernel parameter
> > +but isn't supported by ASIC, ignoring\n");
> >               return false;
> >  #endif
> >       }
> > @@ -3616,7 +3616,8 @@ int amdgpu_device_init(struct amdgpu_device
> > *adev,
> >                pdev->subsystem_vendor, pdev->subsystem_device, pdev-
> > >revision);
> >
> >       /* mutex initialization are all done here so we
> > -      * can recall function without having locking issues */
> > +      * can recall function without having locking issues
> > +      */
> >       mutex_init(&adev->firmware.mutex);
> >       mutex_init(&adev->pm.mutex);
> >       mutex_init(&adev->gfx.gpu_clock_mutex);
> > @@ -3693,11 +3694,11 @@ int amdgpu_device_init(struct amdgpu_device
> > *adev,
> >               atomic_set(&adev->pm.pwr_state[i],
> > POWER_STATE_UNKNOWN);
> >
> >       adev->rmmio = ioremap(adev->rmmio_base, adev->rmmio_size);
> > -     if (adev->rmmio == NULL) {
> > +     if (!adev->rmmio)
> >               return -ENOMEM;
> > -     }
> > +
> >       DRM_INFO("register mmio base: 0x%08X\n", (uint32_t)adev-
> > >rmmio_base);
> > -     DRM_INFO("register mmio size: %u\n", (unsigned)adev-
> > >rmmio_size);
> > +     DRM_INFO("register mmio size: %u\n", (unsigned int)adev-
> > >rmmio_size);
> >
> >       /*
> >        * Reset domain needs to be present early, before XGMI hive
> > discovered @@ -3951,7 +3952,8 @@ int amdgpu_device_init(struct
> > amdgpu_device *adev,
> >
> >       /* if we have > 1 VGA cards, then disable the amdgpu VGA resources
> > */
> >       /* this will fail for cards that aren't VGA class devices, just
> > -      * ignore it */
> > +      * ignore it
> > +      */
> >       if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> >               vga_client_register(adev->pdev,
> > amdgpu_device_vga_set_decode);
> >
> > @@ -4034,7 +4036,7 @@ void amdgpu_device_fini_hw(struct
> > amdgpu_device *adev)
> >
> >       /* make sure IB test finished before entering exclusive mode
> >        * to avoid preemption on IB test
> > -      * */
> > +      */
> >       if (amdgpu_sriov_vf(adev)) {
> >               amdgpu_virt_request_full_gpu(adev, false);
> >               amdgpu_virt_fini_data_exchange(adev);
> > @@ -4771,8 +4773,9 @@ int amdgpu_device_pre_asic_reset(struct
> > amdgpu_device *adev,
> >               if (!ring || !ring->sched.thread)
> >                       continue;
> >
> > -             /*clear job fence from fence drv to avoid force_completion
> > -              *leave NULL and vm flush fence in fence drv */
> > +             /* Clear job fence from fence drv to avoid force_completion
> > +              * leave NULL and vm flush fence in fence drv
> > +              */
> >               amdgpu_fence_driver_clear_job_fences(ring);
> >
> >               /* after all hw jobs are reset, hw fence is meaningless, so
> > force_completion */ @@ -4786,7 +4789,7 @@ int
> > amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
> >
> >       r = amdgpu_reset_prepare_hwcontext(adev, reset_context);
> >       /* If reset handler not implemented, continue; otherwise return */
> > -     if (r == -ENOSYS)
> > +     if (r == -EINVAL)
> >               r = 0;
> >       else
> >               return r;
> > @@ -4904,7 +4907,7 @@ int amdgpu_do_asic_reset(struct list_head
> > *device_list_handle,
> >       reset_context->reset_device_list = device_list_handle;
> >       r = amdgpu_reset_perform_reset(tmp_adev, reset_context);
> >       /* If reset handler not implemented, continue; otherwise return */
> > -     if (r == -ENOSYS)
> > +     if (r == -EINVAL)
> >               r = 0;
> >       else
> >               return r;
> > @@ -5393,9 +5396,8 @@ int amdgpu_device_gpu_recover(struct
> > amdgpu_device *adev,
> >               if (adev->enable_mes && adev->ip_versions[GC_HWIP][0] !=
> > IP_VERSION(11, 0, 3))
> >                       amdgpu_mes_self_test(tmp_adev);
> >
> > -             if
> > (!drm_drv_uses_atomic_modeset(adev_to_drm(tmp_adev))
> > && !job_signaled) {
> > +             if
> > (!drm_drv_uses_atomic_modeset(adev_to_drm(tmp_adev)) &&
> > +!job_signaled)
> >
> >       drm_helper_resume_force_mode(adev_to_drm(tmp_adev));
> > -             }
> >
> >               if (tmp_adev->asic_reset_res)
> >                       r = tmp_adev->asic_reset_res;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
> > index eec41ad30406..12515e40b693 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
> > @@ -87,7 +87,7 @@ int amdgpu_reset_prepare_hwcontext(struct
> > amdgpu_device *adev,
> >               reset_handler = adev->reset_cntl->get_reset_handler(
> >                       adev->reset_cntl, reset_context);
> >       if (!reset_handler)
> > -             return -ENOSYS;
> > +             return -EINVAL;
> >
> >       return reset_handler->prepare_hwcontext(adev->reset_cntl,
> >                                               reset_context);
> > @@ -103,7 +103,7 @@ int amdgpu_reset_perform_reset(struct
> > amdgpu_device *adev,
> >               reset_handler = adev->reset_cntl->get_reset_handler(
> >                       adev->reset_cntl, reset_context);
> >       if (!reset_handler)
> > -             return -ENOSYS;
> > +             return -EINVAL;
> >
> >       ret = reset_handler->perform_reset(adev->reset_cntl, reset_context);
> >       if (ret)
> > --
> > 2.25.1
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: winmail.dat
Type: application/ms-tnef
Size: 20693 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20230725/589696dd/attachment-0001.bin>


More information about the amd-gfx mailing list