| Message ID | 20240426165839.v2.1.I30fa4c8348ea316c886ef8a522a52fed617f930d@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mipi-dsi: Reduce bloat and add funcs for cleaner init seqs | expand |
On Sat, 27 Apr 2024 at 02:59, Douglas Anderson <dianders@chromium.org> wrote: > > The mipi_dsi_dcs_write_seq() macro makes a call to > mipi_dsi_dcs_write_buffer() which returns a type ssize_t. The macro > then stores it in an int and checks to see if it's negative. This > could theoretically be a problem if "ssize_t" is larger than "int". > > To see the issue, imagine that "ssize_t" is 32-bits and "int" is > 16-bits, you could see a problem if there was some code out there that > looked like: > > mipi_dsi_dcs_write_seq(dsi, cmd, <32767 bytes as arguments>); > > ...since we'd get back that 32768 bytes were transferred and 32768 > stored in a 16-bit int would look negative. > > Though there are no callsites where we'd actually hit this (even if > "int" was only 16-bit), it's cleaner to make the types match so let's > fix it. > > Fixes: 2a9e9daf7523 ("drm/mipi-dsi: Introduce mipi_dsi_dcs_write_seq macro") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v2: > - New > > include/drm/drm_mipi_dsi.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > index 82b1cc434ea3..b3576be22bfa 100644 > --- a/include/drm/drm_mipi_dsi.h > +++ b/include/drm/drm_mipi_dsi.h > @@ -337,12 +337,12 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, > do { \ > static const u8 d[] = { cmd, seq }; \ > struct device *dev = &dsi->dev; \ > - int ret; \ > + ssize_t ret; \ > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ > if (ret < 0) { \ > dev_err_ratelimited( \ > dev, "sending command %#02x failed: %d\n", \ > - cmd, ret); \ > + cmd, (int)ret); \ Please consider using %zd instead > return ret; \ > } \ > } while (0) > -- > 2.44.0.769.g3c40516874-goog >
On Sat, Apr 27, 2024 at 04:44:33AM +0300, Dmitry Baryshkov wrote: > On Sat, 27 Apr 2024 at 02:59, Douglas Anderson <dianders@chromium.org> wrote: > > > > The mipi_dsi_dcs_write_seq() macro makes a call to > > mipi_dsi_dcs_write_buffer() which returns a type ssize_t. The macro > > then stores it in an int and checks to see if it's negative. This > > could theoretically be a problem if "ssize_t" is larger than "int". > > > > To see the issue, imagine that "ssize_t" is 32-bits and "int" is > > 16-bits, you could see a problem if there was some code out there that > > looked like: > > > > mipi_dsi_dcs_write_seq(dsi, cmd, <32767 bytes as arguments>); > > > > ...since we'd get back that 32768 bytes were transferred and 32768 > > stored in a 16-bit int would look negative. > > > > Though there are no callsites where we'd actually hit this (even if > > "int" was only 16-bit), it's cleaner to make the types match so let's > > fix it. > > > > Fixes: 2a9e9daf7523 ("drm/mipi-dsi: Introduce mipi_dsi_dcs_write_seq macro") > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > Changes in v2: > > - New > > > > include/drm/drm_mipi_dsi.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > > index 82b1cc434ea3..b3576be22bfa 100644 > > --- a/include/drm/drm_mipi_dsi.h > > +++ b/include/drm/drm_mipi_dsi.h > > @@ -337,12 +337,12 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, > > do { \ > > static const u8 d[] = { cmd, seq }; \ > > struct device *dev = &dsi->dev; \ > > - int ret; \ > > + ssize_t ret; \ > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ > > if (ret < 0) { \ > > dev_err_ratelimited( \ > > dev, "sending command %#02x failed: %d\n", \ > > - cmd, ret); \ > > + cmd, (int)ret); \ > > Please consider using %zd instead Hi Douglas, please consider the above for all the pathces, there are more places where a cast can be dropped. Sam
Hi, On Fri, Apr 26, 2024 at 11:22 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > On Sat, Apr 27, 2024 at 04:44:33AM +0300, Dmitry Baryshkov wrote: > > On Sat, 27 Apr 2024 at 02:59, Douglas Anderson <dianders@chromium.org> wrote: > > > > > > The mipi_dsi_dcs_write_seq() macro makes a call to > > > mipi_dsi_dcs_write_buffer() which returns a type ssize_t. The macro > > > then stores it in an int and checks to see if it's negative. This > > > could theoretically be a problem if "ssize_t" is larger than "int". > > > > > > To see the issue, imagine that "ssize_t" is 32-bits and "int" is > > > 16-bits, you could see a problem if there was some code out there that > > > looked like: > > > > > > mipi_dsi_dcs_write_seq(dsi, cmd, <32767 bytes as arguments>); > > > > > > ...since we'd get back that 32768 bytes were transferred and 32768 > > > stored in a 16-bit int would look negative. > > > > > > Though there are no callsites where we'd actually hit this (even if > > > "int" was only 16-bit), it's cleaner to make the types match so let's > > > fix it. > > > > > > Fixes: 2a9e9daf7523 ("drm/mipi-dsi: Introduce mipi_dsi_dcs_write_seq macro") > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > --- > > > > > > Changes in v2: > > > - New > > > > > > include/drm/drm_mipi_dsi.h | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > > > index 82b1cc434ea3..b3576be22bfa 100644 > > > --- a/include/drm/drm_mipi_dsi.h > > > +++ b/include/drm/drm_mipi_dsi.h > > > @@ -337,12 +337,12 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, > > > do { \ > > > static const u8 d[] = { cmd, seq }; \ > > > struct device *dev = &dsi->dev; \ > > > - int ret; \ > > > + ssize_t ret; \ > > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ > > > if (ret < 0) { \ > > > dev_err_ratelimited( \ > > > dev, "sending command %#02x failed: %d\n", \ > > > - cmd, ret); \ > > > + cmd, (int)ret); \ > > > > Please consider using %zd instead > > Hi Douglas, > please consider the above for all the pathces, there are more places > where a cast can be dropped. Sure, I'll change in the next version. I personally prefer the %d with an "int" type because technically we're printing an error code and errors are int-sized. ...but I don't feel strongly and I guess there's a tiny chance some bug in the code could lead to a negative value that's more useful as 64-bits than 32-bits. ;-) I will note that I will still need a cast in some of the later patches for "%*ph" since, I believe, the size passed for the "*" in a printf format string is defined to be an int, not a size_t or ssize_t. -Doug
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index 82b1cc434ea3..b3576be22bfa 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -337,12 +337,12 @@ int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi, do { \ static const u8 d[] = { cmd, seq }; \ struct device *dev = &dsi->dev; \ - int ret; \ + ssize_t ret; \ ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ if (ret < 0) { \ dev_err_ratelimited( \ dev, "sending command %#02x failed: %d\n", \ - cmd, ret); \ + cmd, (int)ret); \ return ret; \ } \ } while (0)
The mipi_dsi_dcs_write_seq() macro makes a call to mipi_dsi_dcs_write_buffer() which returns a type ssize_t. The macro then stores it in an int and checks to see if it's negative. This could theoretically be a problem if "ssize_t" is larger than "int". To see the issue, imagine that "ssize_t" is 32-bits and "int" is 16-bits, you could see a problem if there was some code out there that looked like: mipi_dsi_dcs_write_seq(dsi, cmd, <32767 bytes as arguments>); ...since we'd get back that 32768 bytes were transferred and 32768 stored in a 16-bit int would look negative. Though there are no callsites where we'd actually hit this (even if "int" was only 16-bit), it's cleaner to make the types match so let's fix it. Fixes: 2a9e9daf7523 ("drm/mipi-dsi: Introduce mipi_dsi_dcs_write_seq macro") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: - New include/drm/drm_mipi_dsi.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)