Hi Philipp,
On 1/23/24 10:43, Philipp Stanner wrote:
> When the PCI devres API was introduced to this driver, it was wrongly
> assumed that initializing the device with pcim_enable_device() instead
> of pci_enable_device() will make all PCI functions managed.
>
> This is wrong and was caused by the quite confusing devres API for PCI
> in which some, but not all, functions become managed that way.
>
> The function pci_iomap_range() is never managed.
>
> Replace pci_iomap_range() with the actually managed function
> pcim_iomap_range().
>
> Additionally, add a call to pcim_request_region() to ensure exclusive
> access to BAR 0.
I'm a bit worried about this last change. There might be
issues where the pcim_request_region() fails due to
e.g. a conflict with the simplefb / simpledrm code.
There is a drm_aperture_remove_conflicting_pci_framebuffers()
call done before hw_init() gets called, but still this
has been known to cause issues in the past.
Can you split out the adding of the pcim_request_region()
into a separate patch and *not* mark that separate patch
for stable ?
Regards,
Hans
>
> CC: <sta...@kernel.vger.org> # v5.10+
> Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions")
> Signed-off-by: Philipp Stanner <pstan...@redhat.com>
> ---
> drivers/gpu/drm/vboxvideo/vbox_main.c | 24 +++++++++++++-----------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c
> b/drivers/gpu/drm/vboxvideo/vbox_main.c
> index 42c2d8a99509..7f686a0190e6 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
> @@ -42,12 +42,11 @@ static int vbox_accel_init(struct vbox_private *vbox)
> /* Take a command buffer for each screen from the end of usable VRAM. */
> vbox->available_vram_size -= vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE;
>
> - vbox->vbva_buffers = pci_iomap_range(pdev, 0,
> - vbox->available_vram_size,
> - vbox->num_crtcs *
> - VBVA_MIN_BUFFER_SIZE);
> - if (!vbox->vbva_buffers)
> - return -ENOMEM;
> + vbox->vbva_buffers = pcim_iomap_range(
> + pdev, 0, vbox->available_vram_size,
> + vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE);
> + if (IS_ERR(vbox->vbva_buffers))
> + return PTR_ERR(vbox->vbva_buffers);
>
> for (i = 0; i < vbox->num_crtcs; ++i) {
> vbva_setup_buffer_context(&vbox->vbva_info[i],
> @@ -115,12 +114,15 @@ int vbox_hw_init(struct vbox_private *vbox)
>
> DRM_INFO("VRAM %08x\n", vbox->full_vram_size);
>
> + ret = pcim_request_region(pdev, 0, "vboxvideo");
> + if (ret)
> + return ret;
> +
> /* Map guest-heap at end of vram */
> - vbox->guest_heap =
> - pci_iomap_range(pdev, 0, GUEST_HEAP_OFFSET(vbox),
> - GUEST_HEAP_SIZE);
> - if (!vbox->guest_heap)
> - return -ENOMEM;
> + vbox->guest_heap = pcim_iomap_range(pdev, 0,
> + GUEST_HEAP_OFFSET(vbox), GUEST_HEAP_SIZE);
> + if (IS_ERR(vbox->guest_heap))
> + return PTR_ERR(vbox->guest_heap);
>
> /* Create guest-heap mem-pool use 2^4 = 16 byte chunks */
> vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4, -1,
