| Message ID | 20241003113304.11700-9-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/client: Stop using legacy crtc->mode and a bunch of cleanups | expand |
Hi Am 03.10.24 um 13:33 schrieb Ville Syrjala: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Replace the 'unsigned int i' footguns with plain old signed > int. Avoids accidents if/when someone decides they need > to iterate backwards. Why are signed types preferable here? Best regards Thomas > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_client_modeset.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index ccf5c9b5537b..875d517fa8f2 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -39,7 +39,7 @@ int drm_client_modeset_create(struct drm_client_dev *client) > unsigned int max_connector_count = 1; > struct drm_mode_set *modeset; > struct drm_crtc *crtc; > - unsigned int i = 0; > + int i = 0; > > /* Add terminating zero entry to enable index less iteration */ > client->modesets = kcalloc(num_crtc + 1, sizeof(*client->modesets), GFP_KERNEL); > @@ -75,7 +75,7 @@ static void drm_client_modeset_release(struct drm_client_dev *client) > struct drm_mode_set *modeset; > > drm_client_for_each_modeset(modeset, client) { > - unsigned int i; > + int i; > > drm_mode_destroy(client->dev, modeset->mode); > modeset->mode = NULL; > @@ -925,7 +925,7 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation) > struct drm_plane *plane = modeset->crtc->primary; > struct drm_cmdline_mode *cmdline; > u64 valid_mask = 0; > - unsigned int i; > + int i; > > if (!modeset->num_connectors) > return false;
On Mon, Oct 07, 2024 at 09:43:47AM +0200, Thomas Zimmermann wrote: > Hi > > Am 03.10.24 um 13:33 schrieb Ville Syrjala: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Replace the 'unsigned int i' footguns with plain old signed > > int. Avoids accidents if/when someone decides they need > > to iterate backwards. > > Why are signed types preferable here? If you iterate backwards you typically write for (i = max; i >= 0; i--) {...} and i>=0 is always true for unsigned types. Another danger is doing any kind of arithmetic with 'i' and expecting a signed result. Based on my experience in getting burned by C integer promotion/converison rules a good rule of thumb is to always use just "int" unless there is a very good reason for not doing so (eg. if the thing is a bitmask or some kind of other thing where negative values can never ever come up). Also IIRC there was a Linus rant about "unsigned int i" but I can't find it now.
On Tue, 08 Oct 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, Oct 07, 2024 at 09:43:47AM +0200, Thomas Zimmermann wrote: >> Hi >> >> Am 03.10.24 um 13:33 schrieb Ville Syrjala: >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > Replace the 'unsigned int i' footguns with plain old signed >> > int. Avoids accidents if/when someone decides they need >> > to iterate backwards. >> >> Why are signed types preferable here? > > If you iterate backwards you typically write > > for (i = max; i >= 0; i--) {...} > > and i>=0 is always true for unsigned types. > > Another danger is doing any kind of arithmetic > with 'i' and expecting a signed result. > > Based on my experience in getting burned by C integer > promotion/converison rules a good rule of thumb is to > always use just "int" unless there is a very good > reason for not doing so (eg. if the thing is a bitmask > or some kind of other thing where negative values > can never ever come up). Agreed. An even worse antipattern is using u8 or u16 just because it's the smallest type that is enough for the range or whatever. But then it ends up being signed int arithmetic assigned back to the small unsigned type anyway. > Also IIRC there was a Linus rant about "unsigned int i" > but I can't find it now. Another summary at [1]. BR, Jani. [1] https://hamstergene.github.io/posts/2021-10-30-do-not-use-unsigned-for-nonnegativity/
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index ccf5c9b5537b..875d517fa8f2 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -39,7 +39,7 @@ int drm_client_modeset_create(struct drm_client_dev *client) unsigned int max_connector_count = 1; struct drm_mode_set *modeset; struct drm_crtc *crtc; - unsigned int i = 0; + int i = 0; /* Add terminating zero entry to enable index less iteration */ client->modesets = kcalloc(num_crtc + 1, sizeof(*client->modesets), GFP_KERNEL); @@ -75,7 +75,7 @@ static void drm_client_modeset_release(struct drm_client_dev *client) struct drm_mode_set *modeset; drm_client_for_each_modeset(modeset, client) { - unsigned int i; + int i; drm_mode_destroy(client->dev, modeset->mode); modeset->mode = NULL; @@ -925,7 +925,7 @@ bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation) struct drm_plane *plane = modeset->crtc->primary; struct drm_cmdline_mode *cmdline; u64 valid_mask = 0; - unsigned int i; + int i; if (!modeset->num_connectors) return false;