| Message ID | 20220812031623.34057-1-samuel@sholland.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: dsi: Prevent underflow when computing packet sizes | expand |
Dne petek, 12. avgust 2022 ob 05:16:23 CEST je Samuel Holland napisal(a): > Currently, the packet overhead is subtracted using unsigned arithmetic. > With a short sync pulse, this could underflow and wrap around to near > the maximal u16 value. Fix this by using signed subtraction. The call to > max() will correctly handle any negative numbers that are produced. > > Apply the same fix to the other timings, even though those subtractions > are less likely to underflow. > > Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller > support") Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index b4dfa166eccd..34234a144e87 > 100644 > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > @@ -522,77 +522,77 @@ static void sun6i_dsi_setup_format(struct sun6i_dsi > *dsi, SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINE0(0xffff) | > SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINEN(0xffff)); > > regmap_write(dsi->regs, SUN6I_DSI_PIXEL_CTL0_REG, > SUN6I_DSI_PIXEL_CTL0_PD_PLUG_DISABLE | > SUN6I_DSI_PIXEL_CTL0_FORMAT(fmt)); > } > > static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, > struct drm_display_mode *mode) > { > struct mipi_dsi_device *device = dsi->device; > - unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8; > + int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8; Nit: mipi_dsi_pixel_format_to_bpp() can return -EINVAL in case of unsupported format. Would it make sense to check it? In any case: Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com> Best regards, Jernej > u16 hbp = 0, hfp = 0, hsa = 0, hblk = 0, vblk = 0; > u32 basic_ctl = 0; > size_t bytes; > u8 *buffer; > > /* Do all timing calculations up front to allocate buffer space */ > > if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) { > hblk = mode->hdisplay * Bpp; > basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST | > SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS | > SUN6I_DSI_BASIC_CTL_HBP_DIS; > > if (device->lanes == 4) > basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL | > SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc); > } else { > /* > * A sync period is composed of a blanking packet (4 > * bytes + payload + 2 bytes) and a sync event packet > * (4 bytes). Its minimal size is therefore 10 bytes > */ > #define HSA_PACKET_OVERHEAD 10 > - hsa = max((unsigned int)HSA_PACKET_OVERHEAD, > + hsa = max(HSA_PACKET_OVERHEAD, > (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD); > > /* > * The backporch is set using a blanking packet (4 > * bytes + payload + 2 bytes). Its minimal size is > * therefore 6 bytes > */ > #define HBP_PACKET_OVERHEAD 6 > - hbp = max((unsigned int)HBP_PACKET_OVERHEAD, > + hbp = max(HBP_PACKET_OVERHEAD, > (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD); > > /* > * The frontporch is set using a sync event (4 bytes) > * and two blanking packets (each one is 4 bytes + > * payload + 2 bytes). Its minimal size is therefore > * 16 bytes > */ > #define HFP_PACKET_OVERHEAD 16 > - hfp = max((unsigned int)HFP_PACKET_OVERHEAD, > + hfp = max(HFP_PACKET_OVERHEAD, > (mode->hsync_start - mode->hdisplay) * Bpp - HFP_PACKET_OVERHEAD); > > /* > * The blanking is set using a sync event (4 bytes) > * and a blanking packet (4 bytes + payload + 2 > * bytes). Its minimal size is therefore 10 bytes. > */ > #define HBLK_PACKET_OVERHEAD 10 > - hblk = max((unsigned int)HBLK_PACKET_OVERHEAD, > + hblk = max(HBLK_PACKET_OVERHEAD, > (mode->htotal - (mode->hsync_end - mode- >hsync_start)) * Bpp - > HBLK_PACKET_OVERHEAD); > > /* > * And I'm not entirely sure what vblk is about. The driver in > * Allwinner BSP is using a rather convoluted calculation > * there only for 4 lanes. However, using 0 (the !4 lanes > * case) even with a 4 lanes screen seems to work... > */ > vblk = 0; > }
On 8/14/22 2:55 AM, Jernej Škrabec wrote: > Dne petek, 12. avgust 2022 ob 05:16:23 CEST je Samuel Holland napisal(a): >> Currently, the packet overhead is subtracted using unsigned arithmetic. >> With a short sync pulse, this could underflow and wrap around to near >> the maximal u16 value. Fix this by using signed subtraction. The call to >> max() will correctly handle any negative numbers that are produced. >> >> Apply the same fix to the other timings, even though those subtractions >> are less likely to underflow. >> >> Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller >> support") Signed-off-by: Samuel Holland <samuel@sholland.org> >> --- >> >> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c >> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index b4dfa166eccd..34234a144e87 >> 100644 >> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c >> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c >> @@ -522,77 +522,77 @@ static void sun6i_dsi_setup_format(struct sun6i_dsi >> *dsi, SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINE0(0xffff) | >> SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINEN(0xffff)); >> >> regmap_write(dsi->regs, SUN6I_DSI_PIXEL_CTL0_REG, >> SUN6I_DSI_PIXEL_CTL0_PD_PLUG_DISABLE | >> SUN6I_DSI_PIXEL_CTL0_FORMAT(fmt)); >> } >> >> static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, >> struct drm_display_mode *mode) >> { >> struct mipi_dsi_device *device = dsi->device; >> - unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8; >> + int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8; > > Nit: mipi_dsi_pixel_format_to_bpp() can return -EINVAL in case of unsupported > format. Would it make sense to check it? The switch statement in mipi_dsi_pixel_format_to_bpp() handles every value in the enumeration, so I think the -EINVAL is just there to keep GCC from complaining. If we do want to handle this case, it would need to be in sun6i_dsi_attach(), since the other places we use mipi_dsi_pixel_format_to_bpp() are way too late to handle any errors. Regards, Samuel
On Thu, 11 Aug 2022 22:16:23 -0500, Samuel Holland wrote: > Currently, the packet overhead is subtracted using unsigned arithmetic. > With a short sync pulse, this could underflow and wrap around to near > the maximal u16 value. Fix this by using signed subtraction. The call to > max() will correctly handle any negative numbers that are produced. > > Apply the same fix to the other timings, even though those subtractions > are less likely to underflow. > > [...] Applied to drm/drm-misc (drm-misc-fixes). Thanks! Maxime
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index b4dfa166eccd..34234a144e87 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -522,77 +522,77 @@ static void sun6i_dsi_setup_format(struct sun6i_dsi *dsi, SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINE0(0xffff) | SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINEN(0xffff)); regmap_write(dsi->regs, SUN6I_DSI_PIXEL_CTL0_REG, SUN6I_DSI_PIXEL_CTL0_PD_PLUG_DISABLE | SUN6I_DSI_PIXEL_CTL0_FORMAT(fmt)); } static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, struct drm_display_mode *mode) { struct mipi_dsi_device *device = dsi->device; - unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8; + int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8; u16 hbp = 0, hfp = 0, hsa = 0, hblk = 0, vblk = 0; u32 basic_ctl = 0; size_t bytes; u8 *buffer; /* Do all timing calculations up front to allocate buffer space */ if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) { hblk = mode->hdisplay * Bpp; basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST | SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS | SUN6I_DSI_BASIC_CTL_HBP_DIS; if (device->lanes == 4) basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL | SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc); } else { /* * A sync period is composed of a blanking packet (4 * bytes + payload + 2 bytes) and a sync event packet * (4 bytes). Its minimal size is therefore 10 bytes */ #define HSA_PACKET_OVERHEAD 10 - hsa = max((unsigned int)HSA_PACKET_OVERHEAD, + hsa = max(HSA_PACKET_OVERHEAD, (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD); /* * The backporch is set using a blanking packet (4 * bytes + payload + 2 bytes). Its minimal size is * therefore 6 bytes */ #define HBP_PACKET_OVERHEAD 6 - hbp = max((unsigned int)HBP_PACKET_OVERHEAD, + hbp = max(HBP_PACKET_OVERHEAD, (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD); /* * The frontporch is set using a sync event (4 bytes) * and two blanking packets (each one is 4 bytes + * payload + 2 bytes). Its minimal size is therefore * 16 bytes */ #define HFP_PACKET_OVERHEAD 16 - hfp = max((unsigned int)HFP_PACKET_OVERHEAD, + hfp = max(HFP_PACKET_OVERHEAD, (mode->hsync_start - mode->hdisplay) * Bpp - HFP_PACKET_OVERHEAD); /* * The blanking is set using a sync event (4 bytes) * and a blanking packet (4 bytes + payload + 2 * bytes). Its minimal size is therefore 10 bytes. */ #define HBLK_PACKET_OVERHEAD 10 - hblk = max((unsigned int)HBLK_PACKET_OVERHEAD, + hblk = max(HBLK_PACKET_OVERHEAD, (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp - HBLK_PACKET_OVERHEAD); /* * And I'm not entirely sure what vblk is about. The driver in * Allwinner BSP is using a rather convoluted calculation * there only for 4 lanes. However, using 0 (the !4 lanes * case) even with a 4 lanes screen seems to work... */ vblk = 0; }
Currently, the packet overhead is subtracted using unsigned arithmetic. With a short sync pulse, this could underflow and wrap around to near the maximal u16 value. Fix this by using signed subtraction. The call to max() will correctly handle any negative numbers that are produced. Apply the same fix to the other timings, even though those subtractions are less likely to underflow. Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller support") Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)