Re: [PATCH] staging: vboxvideo: Fix reporting invalid suggested-offset-properties

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 13-10-17 08:26, Michael Thayer wrote:
Hello Hans,

12.10.2017 20:10, Hans de Goede wrote:
The x and y hints receives from the host are unsigned 32 bit integers and
they get set to -1 (0xffffffff) when invalid. Before this commit the
vboxvideo driver was storing them in an u16 causing the -1 to be truncated
to 65535 which, once reported to userspace, was breaking gnome 3.26+
in Wayland mode.

This commit stores the host values in 32 bit variables, removing the
truncation and checks for -1, replacing it with 0 as -1 is not a valid
suggested-offset-property value. Likewise the properties are now
initialized to 0 instead of -1, since -1 is not a valid value.
This fixes gnome 3.26+ in Wayland mode not working with the vboxvideo
driver.

Thank you, looks sensible.  Minor quibble: wouldn't it be nicer to
compare the unsigned values vbox_connector->vbox_crtc->x_hint and y_hint
against ~0 below, rather than -1 (see inline below)?

It seems to me that the intent clearly is for -1 to be used as a
"not set / invalid" value even though it is an unsigned integer.

unsigned vs signed comparisons on integers of the same size are
really only a problem when using < or > not when using == or !=,
so this really is fine as is IMHO.

Regards,

Hans


Reported-by: Gianfranco Costamagna <locutusofborg@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Cc: Michael Thayer <michael.thayer@xxxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---пишет
  drivers/staging/vboxvideo/vbox_drv.h  |  8 ++++----
  drivers/staging/vboxvideo/vbox_irq.c  |  4 ++--
  drivers/staging/vboxvideo/vbox_mode.c | 26 ++++++++++++++++++--------
  3 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
index 4b9302703b36..eeac4f0cb2c6 100644
--- a/drivers/staging/vboxvideo/vbox_drv.h
+++ b/drivers/staging/vboxvideo/vbox_drv.h
@@ -137,8 +137,8 @@ struct vbox_connector {
  	char name[32];
  	struct vbox_crtc *vbox_crtc;
  	struct {
-		u16 width;
-		u16 height;
+		u32 width;
+		u32 height;
  		bool disconnected;
  	} mode_hint;
  };
@@ -150,8 +150,8 @@ struct vbox_crtc {
  	unsigned int crtc_id;
  	u32 fb_offset;
  	bool cursor_enabled;
-	u16 x_hint;
-	u16 y_hint;
+	u32 x_hint;
+	u32 y_hint;
  };
struct vbox_encoder {
diff --git a/drivers/staging/vboxvideo/vbox_irq.c b/drivers/staging/vboxvideo/vbox_irq.c
index 3ca8bec62ac4..74abdf02d9fd 100644
--- a/drivers/staging/vboxvideo/vbox_irq.c
+++ b/drivers/staging/vboxvideo/vbox_irq.c
@@ -150,8 +150,8 @@ static void vbox_update_mode_hints(struct vbox_private *vbox)
disconnected = !(hints->enabled);
  		crtc_id = vbox_conn->vbox_crtc->crtc_id;
-		vbox_conn->mode_hint.width = hints->cx & 0x8fff;
-		vbox_conn->mode_hint.height = hints->cy & 0x8fff;
+		vbox_conn->mode_hint.width = hints->cx;
+		vbox_conn->mode_hint.height = hints->cy;
  		vbox_conn->vbox_crtc->x_hint = hints->dx;
  		vbox_conn->vbox_crtc->y_hint = hints->dy;
  		vbox_conn->mode_hint.disconnected = disconnected;
diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
index 257a77830410..6f08dc966719 100644
--- a/drivers/staging/vboxvideo/vbox_mode.c
+++ b/drivers/staging/vboxvideo/vbox_mode.c
@@ -553,12 +553,22 @@ static int vbox_get_modes(struct drm_connector *connector)
  		++num_modes;
  	}
  	vbox_set_edid(connector, preferred_width, preferred_height);
-	drm_object_property_set_value(
-		&connector->base, vbox->dev->mode_config.suggested_x_property,
-		vbox_connector->vbox_crtc->x_hint);
-	drm_object_property_set_value(
-		&connector->base, vbox->dev->mode_config.suggested_y_property,
-		vbox_connector->vbox_crtc->y_hint);
+
+	if (vbox_connector->vbox_crtc->x_hint != -1)

Unsigned with signed comparison above.

+		drm_object_property_set_value(&connector->base,
+			vbox->dev->mode_config.suggested_x_property,
+			vbox_connector->vbox_crtc->x_hint);
+	else
+		drm_object_property_set_value(&connector->base,
+			vbox->dev->mode_config.suggested_x_property, 0);
+
+	if (vbox_connector->vbox_crtc->y_hint != -1)

And here.

+		drm_object_property_set_value(&connector->base,
+			vbox->dev->mode_config.suggested_y_property,
+			vbox_connector->vbox_crtc->y_hint);
+	else
+		drm_object_property_set_value(&connector->base,
+			vbox->dev->mode_config.suggested_y_property, 0);
return num_modes;
  }
@@ -640,9 +650,9 @@ static int vbox_connector_init(struct drm_device *dev,
drm_mode_create_suggested_offset_properties(dev);
  	drm_object_attach_property(&connector->base,
-				   dev->mode_config.suggested_x_property, -1);
+				   dev->mode_config.suggested_x_property, 0);
  	drm_object_attach_property(&connector->base,
-				   dev->mode_config.suggested_y_property, -1);
+				   dev->mode_config.suggested_y_property, 0);
  	drm_connector_register(connector);
drm_mode_connector_attach_encoder(connector, encoder);





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]