On Thu, Apr 3, 2025 at 9:28 AM Thomas Zimmermann <tzimmerm...@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 <tzimmerm...@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 <tzimmerm...@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.jakobs...@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)
>
