* [PATCH 1/9] drm/udl: Remove unused field dev from struct udl_device
2025-04-01 16:12 [PATCH 0/9] drm/udl: Support adapters without vendor firmware descriptor Thomas Zimmermann
@ 2025-04-01 16:12 ` Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 2/9] drm/udl: Remove unused field gem_lock " Thomas Zimmermann
` (7 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2025-04-01 16:12 UTC (permalink / raw)
To: airlied, sean, patrik.r.jakobsson; +Cc: dri-devel, Thomas Zimmermann
Reduce the size of struct udl_device by removing the unused
field dev.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/udl/udl_drv.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index e67e7e2e6f1f7..7bae28885f923 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -50,7 +50,6 @@ struct urb_list {
struct udl_device {
struct drm_device drm;
- struct device *dev;
struct drm_plane primary_plane;
struct drm_crtc crtc;
--
2.49.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 2/9] drm/udl: Remove unused field gem_lock from struct udl_device
2025-04-01 16:12 [PATCH 0/9] drm/udl: Support adapters without vendor firmware descriptor Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 1/9] drm/udl: Remove unused field dev from struct udl_device Thomas Zimmermann
@ 2025-04-01 16:12 ` Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 3/9] drm/udl: Improve type safety when using " Thomas Zimmermann
` (6 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2025-04-01 16:12 UTC (permalink / raw)
To: airlied, sean, patrik.r.jakobsson; +Cc: dri-devel, Thomas Zimmermann
Reduce the size of struct udl_device by removing the unused
field gem_lock.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/udl/udl_drv.h | 2 --
drivers/gpu/drm/udl/udl_main.c | 2 --
2 files changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 7bae28885f923..1204319fc8e33 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -56,8 +56,6 @@ struct udl_device {
struct drm_encoder encoder;
struct drm_connector connector;
- struct mutex gem_lock;
-
int sku_pixel_limit;
struct urb_list urbs;
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 48260a821b8d1..f1ffa928d5d9e 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -320,8 +320,6 @@ int udl_init(struct udl_device *udl)
drm_warn(dev, "buffer sharing not supported"); /* not an error */
}
- mutex_init(&udl->gem_lock);
-
if (!udl_parse_vendor_descriptor(udl)) {
ret = -ENODEV;
DRM_ERROR("firmware not recognized. Assume incompatible device\n");
--
2.49.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 3/9] drm/udl: Improve type safety when using struct udl_device
2025-04-01 16:12 [PATCH 0/9] drm/udl: Support adapters without vendor firmware descriptor Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 1/9] drm/udl: Remove unused field dev from struct udl_device Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 2/9] drm/udl: Remove unused field gem_lock " Thomas Zimmermann
@ 2025-04-01 16:12 ` Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 4/9] drm/udl: The number of pixels is always positive Thomas Zimmermann
` (5 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2025-04-01 16:12 UTC (permalink / raw)
To: airlied, sean, patrik.r.jakobsson; +Cc: dri-devel, Thomas Zimmermann
Push upcasts from struct drm_device to struct udl_device outwards
in the call chain; cast earlier and call functions with the upcasted
value. Improves type safety.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/udl/udl_drv.c | 6 ++++--
drivers/gpu/drm/udl/udl_drv.h | 12 ++++++------
drivers/gpu/drm/udl/udl_main.c | 28 ++++++++++++----------------
drivers/gpu/drm/udl/udl_modeset.c | 21 ++++++++++++---------
drivers/gpu/drm/udl/udl_transfer.c | 6 +++---
5 files changed, 37 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index d1bc3f165b27d..1922988625eb0 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -22,13 +22,14 @@ static int udl_usb_suspend(struct usb_interface *interface,
pm_message_t message)
{
struct drm_device *dev = usb_get_intfdata(interface);
+ struct udl_device *udl = to_udl(dev);
int ret;
ret = drm_mode_config_helper_suspend(dev);
if (ret)
return ret;
- udl_sync_pending_urbs(dev);
+ udl_sync_pending_urbs(udl);
return 0;
}
@@ -109,9 +110,10 @@ static int udl_usb_probe(struct usb_interface *interface,
static void udl_usb_disconnect(struct usb_interface *interface)
{
struct drm_device *dev = usb_get_intfdata(interface);
+ struct udl_device *udl = to_udl(dev);
drm_dev_unplug(dev);
- udl_drop_usb(dev);
+ udl_drop_usb(udl);
}
/*
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 1204319fc8e33..918738e549d6d 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -69,22 +69,22 @@ static inline struct usb_device *udl_to_usb_device(struct udl_device *udl)
}
/* modeset */
-int udl_modeset_init(struct drm_device *dev);
+int udl_modeset_init(struct udl_device *udl);
struct drm_connector *udl_connector_init(struct drm_device *dev);
-struct urb *udl_get_urb(struct drm_device *dev);
+struct urb *udl_get_urb(struct udl_device *udl);
-int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
-void udl_sync_pending_urbs(struct drm_device *dev);
+int udl_submit_urb(struct udl_device *udl, struct urb *urb, size_t len);
+void udl_sync_pending_urbs(struct udl_device *udl);
void udl_urb_completion(struct urb *urb);
int udl_init(struct udl_device *udl);
-int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
+int udl_render_hline(struct udl_device *udl, int log_bpp, struct urb **urb_ptr,
const char *front, char **urb_buf_ptr,
u32 byte_offset, u32 device_byte_offset, u32 byte_width);
-int udl_drop_usb(struct drm_device *dev);
+int udl_drop_usb(struct udl_device *udl);
int udl_select_std_channel(struct udl_device *udl);
#endif
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index f1ffa928d5d9e..47fb6c34bfde3 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -145,9 +145,8 @@ void udl_urb_completion(struct urb *urb)
wake_up(&udl->urbs.sleep);
}
-static void udl_free_urb_list(struct drm_device *dev)
+static void udl_free_urb_list(struct udl_device *udl)
{
- struct udl_device *udl = to_udl(dev);
struct urb_node *unode;
struct urb *urb;
@@ -172,9 +171,8 @@ static void udl_free_urb_list(struct drm_device *dev)
wake_up_all(&udl->urbs.sleep);
}
-static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
+static int udl_alloc_urb_list(struct udl_device *udl, int count, size_t size)
{
- struct udl_device *udl = to_udl(dev);
struct urb *urb;
struct urb_node *unode;
char *buf;
@@ -210,7 +208,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
usb_free_urb(urb);
if (size > PAGE_SIZE) {
size /= 2;
- udl_free_urb_list(dev);
+ udl_free_urb_list(udl);
goto retry;
}
break;
@@ -259,9 +257,8 @@ static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout)
}
#define GET_URB_TIMEOUT HZ
-struct urb *udl_get_urb(struct drm_device *dev)
+struct urb *udl_get_urb(struct udl_device *udl)
{
- struct udl_device *udl = to_udl(dev);
struct urb *urb;
spin_lock_irq(&udl->urbs.lock);
@@ -270,9 +267,8 @@ struct urb *udl_get_urb(struct drm_device *dev)
return urb;
}
-int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
+int udl_submit_urb(struct udl_device *udl, struct urb *urb, size_t len)
{
- struct udl_device *udl = to_udl(dev);
int ret;
if (WARN_ON(len > udl->urbs.size)) {
@@ -290,9 +286,9 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
}
/* wait until all pending URBs have been processed */
-void udl_sync_pending_urbs(struct drm_device *dev)
+void udl_sync_pending_urbs(struct udl_device *udl)
{
- struct udl_device *udl = to_udl(dev);
+ struct drm_device *dev = &udl->drm;
spin_lock_irq(&udl->urbs.lock);
/* 2 seconds as a sane timeout */
@@ -329,13 +325,13 @@ int udl_init(struct udl_device *udl)
if (udl_select_std_channel(udl))
DRM_ERROR("Selecting channel failed\n");
- if (!udl_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
+ if (!udl_alloc_urb_list(udl, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
DRM_ERROR("udl_alloc_urb_list failed\n");
goto err;
}
DRM_DEBUG("\n");
- ret = udl_modeset_init(dev);
+ ret = udl_modeset_init(udl);
if (ret)
goto err;
@@ -343,14 +339,14 @@ int udl_init(struct udl_device *udl)
err:
if (udl->urbs.count)
- udl_free_urb_list(dev);
+ udl_free_urb_list(udl);
DRM_ERROR("%d\n", ret);
return ret;
}
-int udl_drop_usb(struct drm_device *dev)
+int udl_drop_usb(struct udl_device *udl)
{
- udl_free_urb_list(dev);
+ udl_free_urb_list(udl);
return 0;
}
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index 3b65e93ea0ae8..231e829bd709a 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -205,6 +205,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
const struct drm_rect *clip)
{
struct drm_device *dev = fb->dev;
+ struct udl_device *udl = to_udl(dev);
void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
int i, ret;
char *cmd;
@@ -216,7 +217,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
return ret;
log_bpp = ret;
- urb = udl_get_urb(dev);
+ urb = udl_get_urb(udl);
if (!urb)
return -ENOMEM;
cmd = urb->transfer_buffer;
@@ -226,7 +227,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
const int byte_offset = line_offset + (clip->x1 << log_bpp);
const int dev_byte_offset = (fb->width * i + clip->x1) << log_bpp;
const int byte_width = drm_rect_width(clip) << log_bpp;
- ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
+ ret = udl_render_hline(udl, log_bpp, &urb, (char *)vaddr,
&cmd, byte_offset, dev_byte_offset,
byte_width);
if (ret)
@@ -239,7 +240,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
if (cmd < (char *)urb->transfer_buffer + urb->transfer_buffer_length)
*cmd++ = UDL_MSG_BULK;
len = cmd - (char *)urb->transfer_buffer;
- ret = udl_submit_urb(dev, urb, len);
+ ret = udl_submit_urb(udl, urb, len);
} else {
udl_urb_completion(urb);
}
@@ -330,6 +331,7 @@ static const struct drm_plane_funcs udl_primary_plane_funcs = {
static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_state *state)
{
struct drm_device *dev = crtc->dev;
+ struct udl_device *udl = to_udl(dev);
struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
struct drm_display_mode *mode = &crtc_state->mode;
struct urb *urb;
@@ -339,7 +341,7 @@ static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atom
if (!drm_dev_enter(dev, &idx))
return;
- urb = udl_get_urb(dev);
+ urb = udl_get_urb(udl);
if (!urb)
goto out;
@@ -355,7 +357,7 @@ static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atom
buf = udl_vidreg_unlock(buf);
buf = udl_dummy_render(buf);
- udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
+ udl_submit_urb(udl, urb, buf - (char *)urb->transfer_buffer);
out:
drm_dev_exit(idx);
@@ -364,6 +366,7 @@ static void udl_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct drm_atom
static void udl_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
{
struct drm_device *dev = crtc->dev;
+ struct udl_device *udl = to_udl(dev);
struct urb *urb;
char *buf;
int idx;
@@ -371,7 +374,7 @@ static void udl_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_ato
if (!drm_dev_enter(dev, &idx))
return;
- urb = udl_get_urb(dev);
+ urb = udl_get_urb(udl);
if (!urb)
goto out;
@@ -381,7 +384,7 @@ static void udl_crtc_helper_atomic_disable(struct drm_crtc *crtc, struct drm_ato
buf = udl_vidreg_unlock(buf);
buf = udl_dummy_render(buf);
- udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
+ udl_submit_urb(udl, urb, buf - (char *)urb->transfer_buffer);
out:
drm_dev_exit(idx);
@@ -476,9 +479,9 @@ static const struct drm_mode_config_funcs udl_mode_config_funcs = {
.atomic_commit = drm_atomic_helper_commit,
};
-int udl_modeset_init(struct drm_device *dev)
+int udl_modeset_init(struct udl_device *udl)
{
- struct udl_device *udl = to_udl(dev);
+ struct drm_device *dev = &udl->drm;
struct drm_plane *primary_plane;
struct drm_crtc *crtc;
struct drm_encoder *encoder;
diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c
index 62224992988f2..7d670b3a52939 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -170,7 +170,7 @@ static void udl_compress_hline16(
* (that we can only write to, slowly, and can never read), and (optionally)
* our shadow copy that tracks what's been sent to that hardware buffer.
*/
-int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
+int udl_render_hline(struct udl_device *udl, int log_bpp, struct urb **urb_ptr,
const char *front, char **urb_buf_ptr,
u32 byte_offset, u32 device_byte_offset,
u32 byte_width)
@@ -199,10 +199,10 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
if (cmd >= cmd_end) {
int len = cmd - (u8 *) urb->transfer_buffer;
- int ret = udl_submit_urb(dev, urb, len);
+ int ret = udl_submit_urb(udl, urb, len);
if (ret)
return ret;
- urb = udl_get_urb(dev);
+ urb = udl_get_urb(udl);
if (!urb)
return -EAGAIN;
*urb_ptr = urb;
--
2.49.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 4/9] drm/udl: The number of pixels is always positive
2025-04-01 16:12 [PATCH 0/9] drm/udl: Support adapters without vendor firmware descriptor Thomas Zimmermann
` (2 preceding siblings ...)
2025-04-01 16:12 ` [PATCH 3/9] drm/udl: Improve type safety when using " Thomas Zimmermann
@ 2025-04-01 16:12 ` Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 5/9] drm/udl: Handle errors from usb_get_descriptor() Thomas Zimmermann
` (4 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2025-04-01 16:12 UTC (permalink / raw)
To: airlied, sean, patrik.r.jakobsson; +Cc: dri-devel, Thomas Zimmermann
Store sku_pixel_limit as type unsigned long instead of int. The
number of pixels available is always positive.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/udl/udl_drv.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 918738e549d6d..145bb95ccc480 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -51,13 +51,13 @@ struct urb_list {
struct udl_device {
struct drm_device drm;
+ unsigned long sku_pixel_limit;
+
struct drm_plane primary_plane;
struct drm_crtc crtc;
struct drm_encoder encoder;
struct drm_connector connector;
- int sku_pixel_limit;
-
struct urb_list urbs;
};
--
2.49.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 5/9] drm/udl: Handle errors from usb_get_descriptor()
2025-04-01 16:12 [PATCH 0/9] drm/udl: Support adapters without vendor firmware descriptor Thomas Zimmermann
` (3 preceding siblings ...)
2025-04-01 16:12 ` [PATCH 4/9] drm/udl: The number of pixels is always positive Thomas Zimmermann
@ 2025-04-01 16:12 ` Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 6/9] drm/udl: Return error if vendor descriptor is too short Thomas Zimmermann
` (3 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2025-04-01 16:12 UTC (permalink / raw)
To: airlied, sean, patrik.r.jakobsson; +Cc: dri-devel, Thomas Zimmermann
Reading the vendor descriptor from the udl device can fail with
an error, which the current code fails to capture. Store the return
value in an integer and test for the error. Abort parsing on errors
or treat the value as length on success.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/udl/udl_main.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 47fb6c34bfde3..4291ddb7158c4 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -31,28 +31,32 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
char *desc;
char *buf;
char *desc_end;
-
- u8 total_len = 0;
+ int ret;
+ unsigned int len;
buf = kzalloc(MAX_VENDOR_DESCRIPTOR_SIZE, GFP_KERNEL);
if (!buf)
return false;
desc = buf;
- total_len = usb_get_descriptor(udev, 0x5f, /* vendor specific */
- 0, desc, MAX_VENDOR_DESCRIPTOR_SIZE);
- if (total_len > 5) {
- DRM_INFO("vendor descriptor length:%x data:%11ph\n",
- total_len, desc);
+ ret = usb_get_descriptor(udev, 0x5f, /* vendor specific */
+ 0, desc, MAX_VENDOR_DESCRIPTOR_SIZE);
+ if (ret < 0)
+ goto unrecognized;
+ len = ret;
+
+ if (len > 5) {
+ DRM_INFO("vendor descriptor length: %u data:%11ph\n",
+ len, desc);
- if ((desc[0] != total_len) || /* descriptor length */
+ if ((desc[0] != len) || /* descriptor length */
(desc[1] != 0x5f) || /* vendor descriptor type */
(desc[2] != 0x01) || /* version (2 bytes) */
(desc[3] != 0x00) ||
- (desc[4] != total_len - 2)) /* length after type */
+ (desc[4] != len - 2)) /* length after type */
goto unrecognized;
- desc_end = desc + total_len;
+ desc_end = desc + len;
desc += 5; /* the fixed header we've already parsed */
while (desc < desc_end) {
--
2.49.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 6/9] drm/udl: Return error if vendor descriptor is too short
2025-04-01 16:12 [PATCH 0/9] drm/udl: Support adapters without vendor firmware descriptor Thomas Zimmermann
` (4 preceding siblings ...)
2025-04-01 16:12 ` [PATCH 5/9] drm/udl: Handle errors from usb_get_descriptor() Thomas Zimmermann
@ 2025-04-01 16:12 ` Thomas Zimmermann
2025-04-02 13:16 ` Patrik Jakobsson
2025-04-01 16:12 ` [PATCH 7/9] drm/udl: Treat vendor descriptor as u8 Thomas Zimmermann
` (2 subsequent siblings)
8 siblings, 1 reply; 14+ messages in thread
From: Thomas Zimmermann @ 2025-04-01 16:12 UTC (permalink / raw)
To: airlied, sean, patrik.r.jakobsson; +Cc: dri-devel, Thomas Zimmermann
There need to be least 5 bytes in the vendor descriptor. Return
an error otherwise. Also change the branching to early-out on
the error. Adjust indention of the rest of the parser function.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/udl/udl_main.c | 72 +++++++++++++++++-----------------
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 4291ddb7158c4..58d6065589d3a 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -45,43 +45,43 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
goto unrecognized;
len = ret;
- if (len > 5) {
- DRM_INFO("vendor descriptor length: %u data:%11ph\n",
- len, desc);
-
- if ((desc[0] != len) || /* descriptor length */
- (desc[1] != 0x5f) || /* vendor descriptor type */
- (desc[2] != 0x01) || /* version (2 bytes) */
- (desc[3] != 0x00) ||
- (desc[4] != len - 2)) /* length after type */
- goto unrecognized;
-
- desc_end = desc + len;
- desc += 5; /* the fixed header we've already parsed */
-
- while (desc < desc_end) {
- u8 length;
- u16 key;
-
- key = le16_to_cpu(*((u16 *) desc));
- desc += sizeof(u16);
- length = *desc;
- desc++;
-
- switch (key) {
- case 0x0200: { /* max_area */
- u32 max_area;
- max_area = le32_to_cpu(*((u32 *)desc));
- DRM_DEBUG("DL chip limited to %d pixel modes\n",
- max_area);
- udl->sku_pixel_limit = max_area;
- break;
- }
- default:
- break;
- }
- desc += length;
+ if (len < 5)
+ goto unrecognized;
+
+ DRM_INFO("vendor descriptor length: %u data:%11ph\n", len, desc);
+
+ if ((desc[0] != len) || /* descriptor length */
+ (desc[1] != 0x5f) || /* vendor descriptor type */
+ (desc[2] != 0x01) || /* version (2 bytes) */
+ (desc[3] != 0x00) ||
+ (desc[4] != len - 2)) /* length after type */
+ goto unrecognized;
+
+ desc_end = desc + len;
+ desc += 5; /* the fixed header we've already parsed */
+
+ while (desc < desc_end) {
+ u8 length;
+ u16 key;
+
+ key = le16_to_cpu(*((u16 *)desc));
+ desc += sizeof(u16);
+ length = *desc;
+ desc++;
+
+ switch (key) {
+ case 0x0200: { /* max_area */
+ u32 max_area = le32_to_cpu(*((u32 *)desc));
+
+ DRM_DEBUG("DL chip limited to %d pixel modes\n",
+ max_area);
+ udl->sku_pixel_limit = max_area;
+ break;
+ }
+ default:
+ break;
}
+ desc += length;
}
goto success;
--
2.49.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 6/9] drm/udl: Return error if vendor descriptor is too short
2025-04-01 16:12 ` [PATCH 6/9] drm/udl: Return error if vendor descriptor is too short Thomas Zimmermann
@ 2025-04-02 13:16 ` Patrik Jakobsson
2025-04-03 7:28 ` Thomas Zimmermann
0 siblings, 1 reply; 14+ messages in thread
From: Patrik Jakobsson @ 2025-04-02 13:16 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: airlied, sean, dri-devel
On Tue, Apr 1, 2025 at 6:23 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> There need to be least 5 bytes in the vendor descriptor. Return
> an error otherwise. Also change the branching to early-out on
> the error. Adjust indention of the rest of the parser function.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/gpu/drm/udl/udl_main.c | 72 +++++++++++++++++-----------------
> 1 file changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 4291ddb7158c4..58d6065589d3a 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -45,43 +45,43 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
> goto unrecognized;
> len = ret;
>
> - if (len > 5) {
> - DRM_INFO("vendor descriptor length: %u data:%11ph\n",
> - len, desc);
> -
> - if ((desc[0] != len) || /* descriptor length */
> - (desc[1] != 0x5f) || /* vendor descriptor type */
> - (desc[2] != 0x01) || /* version (2 bytes) */
> - (desc[3] != 0x00) ||
> - (desc[4] != len - 2)) /* length after type */
> - goto unrecognized;
> -
> - desc_end = desc + len;
> - desc += 5; /* the fixed header we've already parsed */
> -
> - while (desc < desc_end) {
> - u8 length;
> - u16 key;
> -
> - key = le16_to_cpu(*((u16 *) desc));
> - desc += sizeof(u16);
> - length = *desc;
> - desc++;
> -
> - switch (key) {
> - case 0x0200: { /* max_area */
> - u32 max_area;
> - max_area = le32_to_cpu(*((u32 *)desc));
> - DRM_DEBUG("DL chip limited to %d pixel modes\n",
> - max_area);
> - udl->sku_pixel_limit = max_area;
> - break;
> - }
> - default:
> - break;
> - }
> - desc += length;
> + if (len < 5)
Hi Thomas,
Shouldn't this be if (len <= 5)? The old code only parsed if the
descriptor returned at least 6 bytes.
-Patrik
> + goto unrecognized;
> +
> + DRM_INFO("vendor descriptor length: %u data:%11ph\n", len, desc);
> +
> + if ((desc[0] != len) || /* descriptor length */
> + (desc[1] != 0x5f) || /* vendor descriptor type */
> + (desc[2] != 0x01) || /* version (2 bytes) */
> + (desc[3] != 0x00) ||
> + (desc[4] != len - 2)) /* length after type */
> + goto unrecognized;
> +
> + desc_end = desc + len;
> + desc += 5; /* the fixed header we've already parsed */
> +
> + while (desc < desc_end) {
> + u8 length;
> + u16 key;
> +
> + key = le16_to_cpu(*((u16 *)desc));
> + desc += sizeof(u16);
> + length = *desc;
> + desc++;
> +
> + switch (key) {
> + case 0x0200: { /* max_area */
> + u32 max_area = le32_to_cpu(*((u32 *)desc));
> +
> + DRM_DEBUG("DL chip limited to %d pixel modes\n",
> + max_area);
> + udl->sku_pixel_limit = max_area;
> + break;
> + }
> + default:
> + break;
> }
> + desc += length;
> }
>
> goto success;
> --
> 2.49.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 6/9] drm/udl: Return error if vendor descriptor is too short
2025-04-02 13:16 ` Patrik Jakobsson
@ 2025-04-03 7:28 ` Thomas Zimmermann
2025-04-03 11:06 ` Patrik Jakobsson
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Zimmermann @ 2025-04-03 7:28 UTC (permalink / raw)
To: Patrik Jakobsson; +Cc: airlied, sean, dri-devel
Hi
Am 02.04.25 um 15:16 schrieb Patrik Jakobsson:
> On Tue, Apr 1, 2025 at 6:23 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> There need to be least 5 bytes in the vendor descriptor. Return
>> an error otherwise. Also change the branching to early-out on
>> the error. Adjust indention of the rest of the parser function.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/gpu/drm/udl/udl_main.c | 72 +++++++++++++++++-----------------
>> 1 file changed, 36 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
>> index 4291ddb7158c4..58d6065589d3a 100644
>> --- a/drivers/gpu/drm/udl/udl_main.c
>> +++ b/drivers/gpu/drm/udl/udl_main.c
>> @@ -45,43 +45,43 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
>> goto unrecognized;
>> len = ret;
>>
>> - if (len > 5) {
>> - DRM_INFO("vendor descriptor length: %u data:%11ph\n",
>> - len, desc);
>> -
>> - if ((desc[0] != len) || /* descriptor length */
>> - (desc[1] != 0x5f) || /* vendor descriptor type */
>> - (desc[2] != 0x01) || /* version (2 bytes) */
>> - (desc[3] != 0x00) ||
>> - (desc[4] != len - 2)) /* length after type */
>> - goto unrecognized;
>> -
>> - desc_end = desc + len;
>> - desc += 5; /* the fixed header we've already parsed */
>> -
>> - while (desc < desc_end) {
>> - u8 length;
>> - u16 key;
>> -
>> - key = le16_to_cpu(*((u16 *) desc));
>> - desc += sizeof(u16);
>> - length = *desc;
>> - desc++;
>> -
>> - switch (key) {
>> - case 0x0200: { /* max_area */
>> - u32 max_area;
>> - max_area = le32_to_cpu(*((u32 *)desc));
>> - DRM_DEBUG("DL chip limited to %d pixel modes\n",
>> - max_area);
>> - udl->sku_pixel_limit = max_area;
>> - break;
>> - }
>> - default:
>> - break;
>> - }
>> - desc += length;
>> + if (len < 5)
> Hi Thomas,
>
> Shouldn't this be if (len <= 5)? The old code only parsed if the
> descriptor returned at least 6 bytes.
Right, I also noticed that. But I though it was a mistake. The header is
5 bytes and if there are no key-value pairs it's still a valid
descriptor AFAICT. Patch 9 of the series sets a default for the pixel
limit and the adapter would be usable. I rather not change the new
logic, but add an explanation to the commit description. Ok?
Best regards
Thomas
>
> -Patrik
>
>> + goto unrecognized;
>> +
>> + DRM_INFO("vendor descriptor length: %u data:%11ph\n", len, desc);
>> +
>> + if ((desc[0] != len) || /* descriptor length */
>> + (desc[1] != 0x5f) || /* vendor descriptor type */
>> + (desc[2] != 0x01) || /* version (2 bytes) */
>> + (desc[3] != 0x00) ||
>> + (desc[4] != len - 2)) /* length after type */
>> + goto unrecognized;
>> +
>> + desc_end = desc + len;
>> + desc += 5; /* the fixed header we've already parsed */
>> +
>> + while (desc < desc_end) {
>> + u8 length;
>> + u16 key;
>> +
>> + key = le16_to_cpu(*((u16 *)desc));
>> + desc += sizeof(u16);
>> + length = *desc;
>> + desc++;
>> +
>> + switch (key) {
>> + case 0x0200: { /* max_area */
>> + u32 max_area = le32_to_cpu(*((u32 *)desc));
>> +
>> + DRM_DEBUG("DL chip limited to %d pixel modes\n",
>> + max_area);
>> + udl->sku_pixel_limit = max_area;
>> + break;
>> + }
>> + default:
>> + break;
>> }
>> + desc += length;
>> }
>>
>> goto success;
>> --
>> 2.49.0
>>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 6/9] drm/udl: Return error if vendor descriptor is too short
2025-04-03 7:28 ` Thomas Zimmermann
@ 2025-04-03 11:06 ` Patrik Jakobsson
0 siblings, 0 replies; 14+ messages in thread
From: Patrik Jakobsson @ 2025-04-03 11:06 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: airlied, sean, dri-devel
On Thu, Apr 3, 2025 at 9:28 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 02.04.25 um 15:16 schrieb Patrik Jakobsson:
> > On Tue, Apr 1, 2025 at 6:23 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> There need to be least 5 bytes in the vendor descriptor. Return
> >> an error otherwise. Also change the branching to early-out on
> >> the error. Adjust indention of the rest of the parser function.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >> drivers/gpu/drm/udl/udl_main.c | 72 +++++++++++++++++-----------------
> >> 1 file changed, 36 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> >> index 4291ddb7158c4..58d6065589d3a 100644
> >> --- a/drivers/gpu/drm/udl/udl_main.c
> >> +++ b/drivers/gpu/drm/udl/udl_main.c
> >> @@ -45,43 +45,43 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
> >> goto unrecognized;
> >> len = ret;
> >>
> >> - if (len > 5) {
> >> - DRM_INFO("vendor descriptor length: %u data:%11ph\n",
> >> - len, desc);
> >> -
> >> - if ((desc[0] != len) || /* descriptor length */
> >> - (desc[1] != 0x5f) || /* vendor descriptor type */
> >> - (desc[2] != 0x01) || /* version (2 bytes) */
> >> - (desc[3] != 0x00) ||
> >> - (desc[4] != len - 2)) /* length after type */
> >> - goto unrecognized;
> >> -
> >> - desc_end = desc + len;
> >> - desc += 5; /* the fixed header we've already parsed */
> >> -
> >> - while (desc < desc_end) {
> >> - u8 length;
> >> - u16 key;
> >> -
> >> - key = le16_to_cpu(*((u16 *) desc));
> >> - desc += sizeof(u16);
> >> - length = *desc;
> >> - desc++;
> >> -
> >> - switch (key) {
> >> - case 0x0200: { /* max_area */
> >> - u32 max_area;
> >> - max_area = le32_to_cpu(*((u32 *)desc));
> >> - DRM_DEBUG("DL chip limited to %d pixel modes\n",
> >> - max_area);
> >> - udl->sku_pixel_limit = max_area;
> >> - break;
> >> - }
> >> - default:
> >> - break;
> >> - }
> >> - desc += length;
> >> + if (len < 5)
> > Hi Thomas,
> >
> > Shouldn't this be if (len <= 5)? The old code only parsed if the
> > descriptor returned at least 6 bytes.
>
> Right, I also noticed that. But I though it was a mistake. The header is
> 5 bytes and if there are no key-value pairs it's still a valid
> descriptor AFAICT. Patch 9 of the series sets a default for the pixel
> limit and the adapter would be usable. I rather not change the new
> logic, but add an explanation to the commit description. Ok?
Sounds good. With that and the small fix in patch 9/9 everything else
looks fine.
With the mentioned fixes done:
Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
>
> Best regards
> Thomas
>
> >
> > -Patrik
> >
> >> + goto unrecognized;
> >> +
> >> + DRM_INFO("vendor descriptor length: %u data:%11ph\n", len, desc);
> >> +
> >> + if ((desc[0] != len) || /* descriptor length */
> >> + (desc[1] != 0x5f) || /* vendor descriptor type */
> >> + (desc[2] != 0x01) || /* version (2 bytes) */
> >> + (desc[3] != 0x00) ||
> >> + (desc[4] != len - 2)) /* length after type */
> >> + goto unrecognized;
> >> +
> >> + desc_end = desc + len;
> >> + desc += 5; /* the fixed header we've already parsed */
> >> +
> >> + while (desc < desc_end) {
> >> + u8 length;
> >> + u16 key;
> >> +
> >> + key = le16_to_cpu(*((u16 *)desc));
> >> + desc += sizeof(u16);
> >> + length = *desc;
> >> + desc++;
> >> +
> >> + switch (key) {
> >> + case 0x0200: { /* max_area */
> >> + u32 max_area = le32_to_cpu(*((u32 *)desc));
> >> +
> >> + DRM_DEBUG("DL chip limited to %d pixel modes\n",
> >> + max_area);
> >> + udl->sku_pixel_limit = max_area;
> >> + break;
> >> + }
> >> + default:
> >> + break;
> >> }
> >> + desc += length;
> >> }
> >>
> >> goto success;
> >> --
> >> 2.49.0
> >>
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 7/9] drm/udl: Treat vendor descriptor as u8
2025-04-01 16:12 [PATCH 0/9] drm/udl: Support adapters without vendor firmware descriptor Thomas Zimmermann
` (5 preceding siblings ...)
2025-04-01 16:12 ` [PATCH 6/9] drm/udl: Return error if vendor descriptor is too short Thomas Zimmermann
@ 2025-04-01 16:12 ` Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 8/9] drm/udl: Validate length in vendor-descriptor parser Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 9/9] drm/udl: Support adapters without firmware descriptor Thomas Zimmermann
8 siblings, 0 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2025-04-01 16:12 UTC (permalink / raw)
To: airlied, sean, patrik.r.jakobsson; +Cc: dri-devel, Thomas Zimmermann
The vendor descriptor is an array of unsigned bytes. It is raw data
that is not to be modified. Declare it as 'const u8'.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/udl/udl_main.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 58d6065589d3a..d3a04bcb65d25 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -28,19 +28,18 @@ static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout);
static int udl_parse_vendor_descriptor(struct udl_device *udl)
{
struct usb_device *udev = udl_to_usb_device(udl);
- char *desc;
- char *buf;
- char *desc_end;
+ void *buf;
int ret;
unsigned int len;
+ const u8 *desc;
+ const u8 *desc_end;
buf = kzalloc(MAX_VENDOR_DESCRIPTOR_SIZE, GFP_KERNEL);
if (!buf)
return false;
- desc = buf;
ret = usb_get_descriptor(udev, 0x5f, /* vendor specific */
- 0, desc, MAX_VENDOR_DESCRIPTOR_SIZE);
+ 0, buf, MAX_VENDOR_DESCRIPTOR_SIZE);
if (ret < 0)
goto unrecognized;
len = ret;
@@ -48,6 +47,9 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
if (len < 5)
goto unrecognized;
+ desc = buf;
+ desc_end = desc + len;
+
DRM_INFO("vendor descriptor length: %u data:%11ph\n", len, desc);
if ((desc[0] != len) || /* descriptor length */
@@ -56,9 +58,7 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
(desc[3] != 0x00) ||
(desc[4] != len - 2)) /* length after type */
goto unrecognized;
-
- desc_end = desc + len;
- desc += 5; /* the fixed header we've already parsed */
+ desc += 5;
while (desc < desc_end) {
u8 length;
--
2.49.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 8/9] drm/udl: Validate length in vendor-descriptor parser
2025-04-01 16:12 [PATCH 0/9] drm/udl: Support adapters without vendor firmware descriptor Thomas Zimmermann
` (6 preceding siblings ...)
2025-04-01 16:12 ` [PATCH 7/9] drm/udl: Treat vendor descriptor as u8 Thomas Zimmermann
@ 2025-04-01 16:12 ` Thomas Zimmermann
2025-04-01 16:12 ` [PATCH 9/9] drm/udl: Support adapters without firmware descriptor Thomas Zimmermann
8 siblings, 0 replies; 14+ messages in thread
From: Thomas Zimmermann @ 2025-04-01 16:12 UTC (permalink / raw)
To: airlied, sean, patrik.r.jakobsson; +Cc: dri-devel, Thomas Zimmermann
Rewrite the parser for the vendor firmware descriptor with the
following improvements.
- Validate the key-value length given in a vendor descriptor
against the length of the descriptor. The current code fails
to do this and might read more bytes than available. This can
lead to out-of-bounds reads of the allocated buffer.
- Read raw data with helpers for unaligned data. This allows
the code to run on platforms that do now support unaligned memory
access by default.
- Validate the pixel limit against a default value. The default
comes from real-world devices. If the reported number of pixels
is significantly above the limit, it is likely invalid.
- Drop the obsolete print macros. There is still a warning about
invalid firmware descriptors. The rest of the output is bogus.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/udl/udl_main.c | 77 ++++++++++++++++++++++------------
1 file changed, 51 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index d3a04bcb65d25..b5a6b254a2028 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -8,6 +8,8 @@
* Copyright (C) 2009 Bernie Thompson <bernie@plugable.com>
*/
+#include <linux/unaligned.h>
+
#include <drm/drm.h>
#include <drm/drm_print.h>
#include <drm/drm_probe_helper.h>
@@ -23,10 +25,56 @@
#define WRITES_IN_FLIGHT (20)
#define MAX_VENDOR_DESCRIPTOR_SIZE 256
+#define UDL_SKU_PIXEL_LIMIT_DEFAULT 2080000
+
static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout);
+/*
+ * Try to make sense of whatever we parse. Therefore return @end on
+ * errors, but don't fail hard.
+ */
+static const u8 *udl_parse_key_value_pair(struct udl_device *udl, const u8 *pos, const u8 *end)
+{
+ u16 key;
+ u8 len;
+
+ /* read key */
+ if (pos >= end - 2)
+ return end;
+ key = get_unaligned_le16(pos);
+ pos += 2;
+
+ /* read value length */
+ if (pos >= end - 1)
+ return end;
+ len = *pos++;
+
+ /* read value */
+ if (pos >= end - len)
+ return end;
+ switch (key) {
+ case 0x0200: { /* maximum number of pixels */
+ unsigned int sku_pixel_limit;
+
+ if (len < sizeof(__le32))
+ break;
+ sku_pixel_limit = get_unaligned_le32(pos);
+ if (sku_pixel_limit >= 16 * UDL_SKU_PIXEL_LIMIT_DEFAULT)
+ break; /* almost 100 MiB, so probably bogus */
+ udl->sku_pixel_limit = sku_pixel_limit;
+ break;
+ }
+ default:
+ break;
+ }
+ pos += len;
+
+ return pos;
+}
+
static int udl_parse_vendor_descriptor(struct udl_device *udl)
{
+ struct drm_device *dev = &udl->drm;
struct usb_device *udev = udl_to_usb_device(udl);
void *buf;
int ret;
@@ -50,8 +98,6 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
desc = buf;
desc_end = desc + len;
- DRM_INFO("vendor descriptor length: %u data:%11ph\n", len, desc);
-
if ((desc[0] != len) || /* descriptor length */
(desc[1] != 0x5f) || /* vendor descriptor type */
(desc[2] != 0x01) || /* version (2 bytes) */
@@ -60,35 +106,14 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
goto unrecognized;
desc += 5;
- while (desc < desc_end) {
- u8 length;
- u16 key;
-
- key = le16_to_cpu(*((u16 *)desc));
- desc += sizeof(u16);
- length = *desc;
- desc++;
-
- switch (key) {
- case 0x0200: { /* max_area */
- u32 max_area = le32_to_cpu(*((u32 *)desc));
-
- DRM_DEBUG("DL chip limited to %d pixel modes\n",
- max_area);
- udl->sku_pixel_limit = max_area;
- break;
- }
- default:
- break;
- }
- desc += length;
- }
+ while (desc < desc_end)
+ desc = udl_parse_key_value_pair(udl, desc, desc_end);
goto success;
unrecognized:
/* allow udlfb to load for now even if firmware unrecognized */
- DRM_ERROR("Unrecognized vendor firmware descriptor\n");
+ drm_warn(dev, "Unrecognized vendor firmware descriptor\n");
success:
kfree(buf);
--
2.49.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 9/9] drm/udl: Support adapters without firmware descriptor
2025-04-01 16:12 [PATCH 0/9] drm/udl: Support adapters without vendor firmware descriptor Thomas Zimmermann
` (7 preceding siblings ...)
2025-04-01 16:12 ` [PATCH 8/9] drm/udl: Validate length in vendor-descriptor parser Thomas Zimmermann
@ 2025-04-01 16:12 ` Thomas Zimmermann
2025-04-03 11:03 ` Patrik Jakobsson
8 siblings, 1 reply; 14+ messages in thread
From: Thomas Zimmermann @ 2025-04-01 16:12 UTC (permalink / raw)
To: airlied, sean, patrik.r.jakobsson; +Cc: dri-devel, Thomas Zimmermann
Set default limit on the number of pixels for adapters without
vendor firmware descriptor. The devices work as expected, they
just don't provide any description.
If parsing the vendor firmware descriptor fails, the device falls
back to the given default limits. Failing to allocate memory is
still an error.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/udl/udl_main.c | 37 +++++++++++++++++++---------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index b5a6b254a2028..2685608af8cec 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -76,6 +76,7 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
{
struct drm_device *dev = &udl->drm;
struct usb_device *udev = udl_to_usb_device(udl);
+ bool detected = false;
void *buf;
int ret;
unsigned int len;
@@ -84,16 +85,16 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
buf = kzalloc(MAX_VENDOR_DESCRIPTOR_SIZE, GFP_KERNEL);
if (!buf)
- return false;
+ return -ENOMEM;
ret = usb_get_descriptor(udev, 0x5f, /* vendor specific */
0, buf, MAX_VENDOR_DESCRIPTOR_SIZE);
if (ret < 0)
- goto unrecognized;
+ goto out;
len = ret;
if (len < 5)
- goto unrecognized;
+ goto out;
desc = buf;
desc_end = desc + len;
@@ -103,21 +104,20 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
(desc[2] != 0x01) || /* version (2 bytes) */
(desc[3] != 0x00) ||
(desc[4] != len - 2)) /* length after type */
- goto unrecognized;
+ goto out;
desc += 5;
+ detected = true;
+
while (desc < desc_end)
desc = udl_parse_key_value_pair(udl, desc, desc_end);
- goto success;
-
-unrecognized:
- /* allow udlfb to load for now even if firmware unrecognized */
- drm_warn(dev, "Unrecognized vendor firmware descriptor\n");
-
-success:
+out:
+ if (!detected)
+ drm_warn(dev, "Unrecognized vendor firmware descriptor\n");
kfree(buf);
- return true;
+
+ return 0;
}
/*
@@ -345,11 +345,16 @@ int udl_init(struct udl_device *udl)
drm_warn(dev, "buffer sharing not supported"); /* not an error */
}
- if (!udl_parse_vendor_descriptor(udl)) {
- ret = -ENODEV;
- DRM_ERROR("firmware not recognized. Assume incompatible device\n");
+ /*
+ * Not all devices provide vendor descriptors with device
+ * information. Initialize to default values from real-world
+ * devices. It is just enough memory for FullHD.
+ */
+ udl->sku_pixel_limit = USL_SKU_PIXEL_LIMIT_DEFAULT;
+
+ ret = udl_parse_vendor_descriptor(udl);
+ if (ret)
goto err;
- }
if (udl_select_std_channel(udl))
DRM_ERROR("Selecting channel failed\n");
--
2.49.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 9/9] drm/udl: Support adapters without firmware descriptor
2025-04-01 16:12 ` [PATCH 9/9] drm/udl: Support adapters without firmware descriptor Thomas Zimmermann
@ 2025-04-03 11:03 ` Patrik Jakobsson
0 siblings, 0 replies; 14+ messages in thread
From: Patrik Jakobsson @ 2025-04-03 11:03 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: airlied, sean, dri-devel
On Tue, Apr 1, 2025 at 6:23 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Set default limit on the number of pixels for adapters without
> vendor firmware descriptor. The devices work as expected, they
> just don't provide any description.
>
> If parsing the vendor firmware descriptor fails, the device falls
> back to the given default limits. Failing to allocate memory is
> still an error.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/gpu/drm/udl/udl_main.c | 37 +++++++++++++++++++---------------
> 1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index b5a6b254a2028..2685608af8cec 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -76,6 +76,7 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
> {
> struct drm_device *dev = &udl->drm;
> struct usb_device *udev = udl_to_usb_device(udl);
> + bool detected = false;
> void *buf;
> int ret;
> unsigned int len;
> @@ -84,16 +85,16 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
>
> buf = kzalloc(MAX_VENDOR_DESCRIPTOR_SIZE, GFP_KERNEL);
> if (!buf)
> - return false;
> + return -ENOMEM;
>
> ret = usb_get_descriptor(udev, 0x5f, /* vendor specific */
> 0, buf, MAX_VENDOR_DESCRIPTOR_SIZE);
> if (ret < 0)
> - goto unrecognized;
> + goto out;
> len = ret;
>
> if (len < 5)
> - goto unrecognized;
> + goto out;
>
> desc = buf;
> desc_end = desc + len;
> @@ -103,21 +104,20 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
> (desc[2] != 0x01) || /* version (2 bytes) */
> (desc[3] != 0x00) ||
> (desc[4] != len - 2)) /* length after type */
> - goto unrecognized;
> + goto out;
> desc += 5;
>
> + detected = true;
> +
> while (desc < desc_end)
> desc = udl_parse_key_value_pair(udl, desc, desc_end);
>
> - goto success;
> -
> -unrecognized:
> - /* allow udlfb to load for now even if firmware unrecognized */
> - drm_warn(dev, "Unrecognized vendor firmware descriptor\n");
> -
> -success:
> +out:
> + if (!detected)
> + drm_warn(dev, "Unrecognized vendor firmware descriptor\n");
> kfree(buf);
> - return true;
> +
> + return 0;
> }
>
> /*
> @@ -345,11 +345,16 @@ int udl_init(struct udl_device *udl)
> drm_warn(dev, "buffer sharing not supported"); /* not an error */
> }
>
> - if (!udl_parse_vendor_descriptor(udl)) {
> - ret = -ENODEV;
> - DRM_ERROR("firmware not recognized. Assume incompatible device\n");
> + /*
> + * Not all devices provide vendor descriptors with device
> + * information. Initialize to default values from real-world
> + * devices. It is just enough memory for FullHD.
> + */
> + udl->sku_pixel_limit = USL_SKU_PIXEL_LIMIT_DEFAULT;
Should be UDL_SKU_PIXEL_LIMIT_DEFAULT
> +
> + ret = udl_parse_vendor_descriptor(udl);
> + if (ret)
> goto err;
> - }
>
> if (udl_select_std_channel(udl))
> DRM_ERROR("Selecting channel failed\n");
> --
> 2.49.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread