| Message ID | 20250325212823.669459-2-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: Make some resolution info unsigned | expand |
Hi Am 25.03.25 um 22:27 schrieb Lyude Paul: > A negative resolution doesn't really make any sense, no one goes into a TV > store and says "Hello sir, I would like a negative 4K TV please", that > would make everyone look at you funny. > > So, let's make these parameters a bit more reasonable and ensure that > they're unsigned - which makes the resulting rust bindings for this > function a bit easier to understand and work with. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Maxime Ripard <mripard@kernel.org> > --- > drivers/gpu/drm/drm_edid.c | 2 +- > include/drm/drm_edid.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 13bc4c290b17d..2e2e1d2347397 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -7099,7 +7099,7 @@ EXPORT_SYMBOL(drm_add_edid_modes); > * Return: The number of modes added or 0 if we couldn't find any. > */ > int drm_add_modes_noedid(struct drm_connector *connector, > - int hdisplay, int vdisplay) > + unsigned int hdisplay, unsigned int vdisplay) You should also remove these branches: https://elixir.bootlin.com/linux/v6.13.7/source/drivers/gpu/drm/drm_edid.c#L7109 Best regards Thomas > { > int i, count, num_modes = 0; > struct drm_display_mode *mode; > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index eaac5e665892a..b38409670868d 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -437,7 +437,7 @@ bool drm_detect_monitor_audio(const struct edid *edid); > enum hdmi_quantization_range > drm_default_rgb_quant_range(const struct drm_display_mode *mode); > int drm_add_modes_noedid(struct drm_connector *connector, > - int hdisplay, int vdisplay); > + unsigned int hdisplay, unsigned int vdisplay); > > int drm_edid_header_is_valid(const void *edid); > bool drm_edid_is_valid(struct edid *edid);
On Tue, 25 Mar 2025, Lyude Paul <lyude@redhat.com> wrote: > A negative resolution doesn't really make any sense, no one goes into a TV > store and says "Hello sir, I would like a negative 4K TV please", that > would make everyone look at you funny. That is largely the point, though. You know something fishy is going on when you have a negative resolution. Nobody blinks an eye when you ask for 4294963K telly, but it's still just as bonkers as that negative 4K. I think the change at hand is fine, but please let's not pretend using unsigned somehow protects us from negative numbers. BR, Jani.
On Wed, Mar 26, 2025 at 11:39 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > That is largely the point, though. You know something fishy is going on > when you have a negative resolution. Nobody blinks an eye when you ask > for 4294963K telly, but it's still just as bonkers as that negative 4K. > > I think the change at hand is fine, but please let's not pretend using > unsigned somehow protects us from negative numbers. Is there a reasonable maximum that could/should be checked for? (I don't know the context) In other words, if one wants to detect invalid values in a primitive type, one needs to define the valid range anyway. Using the negatives of a signed type is convenient in C, but perhaps there is a tighter threshold? If so, then an extra advantage is that on the Rust side one could also have a proper strong type for this etc. Cheers, Miguel
On Wed, 2025-03-26 at 12:39 +0200, Jani Nikula wrote: > On Tue, 25 Mar 2025, Lyude Paul <lyude@redhat.com> wrote: > > A negative resolution doesn't really make any sense, no one goes into a TV > > store and says "Hello sir, I would like a negative 4K TV please", that > > would make everyone look at you funny. > > That is largely the point, though. You know something fishy is going on > when you have a negative resolution. Nobody blinks an eye when you ask > for 4294963K telly, but it's still just as bonkers as that negative 4K. > > I think the change at hand is fine, but please let's not pretend using > unsigned somehow protects us from negative numbers. So - it actually does protect us to a limited extent on the rust side of things. With CONFIG_RUST_OVERFLOW_CHECKS=y, arithematic checks are builtin to the language. This isn't the default config of course, but it's better then nothing. I probably should have mentioned this in the commit message so I'll do that on the next respin. > > > BR, > Jani. > >
On Fri, Mar 28, 2025 at 11:27 PM Lyude Paul <lyude@redhat.com> wrote: > > So - it actually does protect us to a limited extent on the rust side of > things. With CONFIG_RUST_OVERFLOW_CHECKS=y, arithematic checks are builtin to > the language. This isn't the default config of course, but it's better then > nothing. It is the default! :) Cheers, Miguel
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 13bc4c290b17d..2e2e1d2347397 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -7099,7 +7099,7 @@ EXPORT_SYMBOL(drm_add_edid_modes); * Return: The number of modes added or 0 if we couldn't find any. */ int drm_add_modes_noedid(struct drm_connector *connector, - int hdisplay, int vdisplay) + unsigned int hdisplay, unsigned int vdisplay) { int i, count, num_modes = 0; struct drm_display_mode *mode; diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index eaac5e665892a..b38409670868d 100644 --- a/include/drm/drm_edid.h +++ b/include/drm/drm_edid.h @@ -437,7 +437,7 @@ bool drm_detect_monitor_audio(const struct edid *edid); enum hdmi_quantization_range drm_default_rgb_quant_range(const struct drm_display_mode *mode); int drm_add_modes_noedid(struct drm_connector *connector, - int hdisplay, int vdisplay); + unsigned int hdisplay, unsigned int vdisplay); int drm_edid_header_is_valid(const void *edid); bool drm_edid_is_valid(struct edid *edid);
A negative resolution doesn't really make any sense, no one goes into a TV store and says "Hello sir, I would like a negative 4K TV please", that would make everyone look at you funny. So, let's make these parameters a bit more reasonable and ensure that they're unsigned - which makes the resulting rust bindings for this function a bit easier to understand and work with. Signed-off-by: Lyude Paul <lyude@redhat.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Maxime Ripard <mripard@kernel.org> --- drivers/gpu/drm/drm_edid.c | 2 +- include/drm/drm_edid.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)