| Message ID | 20250401161929.283244-7-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/udl: Support adapters without vendor firmware descriptor | expand |
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 >
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 >>
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) >
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;
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(-)