Re: [PATCH v3] drm/gud: move format init from gud_probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/gud: zero max_buffer_size in gud_get_max_buffer_size() for R1/XRGB1111
--

commit 9571a61953c7bcaa943faf581ba726cb4c44a2b4
Author: Ruben Wauters <rubenru09@aol.com>

drm/gud: move format init from gud_probe()

This patch factors pixel format initialization and maximum buffer size
calculation out of gud_probe() into helper functions gud_get_formats() and
gud_get_max_buffer_size().

> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> index 680447f426f37..36adf2e2b6a2e 100644
> --- a/drivers/gpu/drm/gud/gud_drv.c
> +++ b/drivers/gpu/drm/gud/gud_drv.c
> @@ -435,11 +435,115 @@ static void gud_free_buffers_and_mutex(void *data)
>  	mutex_destroy(&gdrm->ctrl_lock);
>  }
>  
> -static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
> +static size_t gud_get_max_buffer_size(struct gud_device *gdrm, u32 *formats, u32 num_formats)
> +{
> +	struct drm_device *drm = &gdrm->drm;
> +	size_t max_buffer_size = 0;
> +	size_t fmt_buf_size;
> +	int i;
> +
> +	for (i = 0; i < num_formats; i++) {
> +		const struct drm_format_info *info = drm_format_info(formats[i]);
> +
> +		/* Skip over emulated XRGB8888 */
> +		if (formats[i] == DRM_FORMAT_XRGB8888 && gdrm->xrgb8888_emulation_format)
> +			continue;
> +
> +		fmt_buf_size = drm_format_info_min_pitch(info, 0, drm->mode_config.max_width) *
> +							 drm->mode_config.max_height;
> +		max_buffer_size = max(max_buffer_size, fmt_buf_size);
> +	}
> +
> +	return max_buffer_size;
> +}

[Severity: High]
Could skipping emulated DRM_FORMAT_XRGB8888 in gud_get_max_buffer_size()
cause max_buffer_size to be calculated as 0 when a device only supports
internal formats (GUD_DRM_FORMAT_R1 or GUD_DRM_FORMAT_XRGB1111)?

In gud_get_formats(), GUD_DRM_FORMAT_R1 and GUD_DRM_FORMAT_XRGB1111 set
xrgb8888_emulation_format to &gud_drm_format_r1 or &gud_drm_format_xrgb1111,
and are then skipped before being added to formats[]:

	if (format == GUD_DRM_FORMAT_R1 || format == GUD_DRM_FORMAT_XRGB1111)
		continue; /* Internal not for userspace */

	formats[num_formats++] = format;

If the device only advertises GUD_PIXEL_FORMAT_R1 and/or
GUD_PIXEL_FORMAT_XRGB1111, num_formats remains 0 after the loop and
gud_get_formats() appends DRM_FORMAT_XRGB8888 as the sole entry in
formats[]:

	if (!xrgb8888_supported && xrgb8888_emulation_format) {
		gdrm->xrgb8888_emulation_format = xrgb8888_emulation_format;
		formats[num_formats++] = DRM_FORMAT_XRGB8888;
	}

Previously, gud_probe() computed fmt_buf_size inside the formats_dev loop
before skipping R1 and XRGB1111, so their buffer sizes were accounted for.

With this change, gud_get_max_buffer_size() skips the only entry in
formats[] and returns 0. When the optional desc.max_buffer_size descriptor
field is 0, gud_probe() sets gdrm->bulk_len = 0 and calls
gud_alloc_bulk_buffer():

gud_probe()
  gud_alloc_bulk_buffer()
    vmalloc_32(gdrm->bulk_len)

Passing 0 to vmalloc_32() triggers WARN_ON_ONCE(!size) and returns NULL,
causing gud_probe() to fail with -ENOMEM.

Should gud_get_max_buffer_size() substitute gdrm->xrgb8888_emulation_format
for info when formats[i] == DRM_FORMAT_XRGB8888 and
gdrm->xrgb8888_emulation_format is set, instead of skipping it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026210116.10082-1-rubenru09@aol.com?part=1