Re: [PATCH v3] video: hyperv: hyperv_fb: refresh the VM screen by force on VM panic

On 31/07/14 13:11, Dexuan Cui wrote:

quoted

-----Original Message-----
From: Tomi Valkeinen [mailto:tomi.valkeinen@ti.com]
Sent: Wednesday, July 30, 2014 22:24 PM

quoted

+static struct fb_info *hvfb_info;

Static variables like these are usually a no-no. This prevents you from
having multiple device instances.

I agree.

quoted

quoted

 static uint screen_width = HVFB_WIDTH;
 static uint screen_height = HVFB_HEIGHT;
 static uint screen_depth;
 static uint screen_fb_size;

+/* If true, the VSC notifies the VSP on every framebuffer change */
+static bool synchronous_fb;
+

Same comment here.

However, if (and only if) the driver is already designed to work only
with single device instance, then this patch is probably ok. But even

IMO the host should only provide at most 1 synthetic video device to a VM. :-)

quoted

then, I'd prefer this to be handled without static variables so that the
driver could eventually be changed to support multiple device instances.

Hi Tomi,
Maybe we can remove these static stuff:
+static struct fb_info *hvfb_info;
+static struct notifier_block hvfb_panic_nb = {
+	.notifier_call = hvfb_on_panic,
+};
by kmalloc()-ing  a new struct:
struct hv_fb_panic_nb {
        struct fb_info *hvfb_info;
        struct notifier_block nb;
}
?
I think in hvfb_on_panic() we should be able to get the
hvfb_info pointer by
hvfb_info = container_of(nb, struct hv_fb_panic_nb, nb).

If you like that or have a better idea, please let me know so
I can make a new patch.

To be honest, I haven't been using notifiers much. But the above looks
ok to me.

Or maybe you can add the notifier_block and the synchronous_fb to
hvfb_par? From the notifier_block pointer you could then get hvfp_par,
and from there hvfb_info.

 Tomi