On 31/07/14 13:11, Dexuan Cui wrote: >> -----Original Message----- >> From: Tomi Valkeinen [mailto:tomi.valkeinen@xxxxxx] >> Sent: Wednesday, July 30, 2014 22:24 PM >>> +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. > >>> 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. :-) > >> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
