linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michal Januszewski <spock@gentoo.org>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-fbdev-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] fbdev: uvesafb driver
Date: Sun, 24 Jun 2007 01:20:33 +0200	[thread overview]
Message-ID: <20070623232033.GB7148@spock.one.pl> (raw)
In-Reply-To: <20070623113557.f0295218.akpm@linux-foundation.org>

On Sat, Jun 23, 2007 at 11:35:57AM -0700, Andrew Morton wrote:
> On Sat, 23 Jun 2007 12:52:43 +0200 Michal Januszewski <spock@gentoo.org> wrote:

> > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > index 403dac7..5cc03f9 100644
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -585,6 +585,24 @@ config FB_TGA
> >  
> >  	  Say Y if you have one of those.
> >  
> > +config FB_UVESA
> > +	tristate "Userspace VESA VGA graphics support"
> > +	depends on FB && CONNECTOR
> 
> These dependencies are insufficient.

What exactly is missing here?  A dep on X86?  This would indicate the
arches on which the driver has actually been tested.  But which arches
are supported and which aren't is, in the end, up to the userspace helper.
 
> > +static struct cb_id uvesafb_cn_id = {
> > +	.idx = CN_IDX_V86D,
> > +	.val = CN_VAL_V86D_UVESAFB
> > +};
> > +static struct sock *nls;
> > +static char v86d_path[PATH_MAX] = "/sbin/v86d";
> 
> Remove the PATH_MAX, save some memory.
> 
> Oh, it gets set via sysfs.  hrm.

Hmm, I guess I could make it a hard-coded value, but paths to userspace 
helpers should generally be configurable, right?

> Now I'm wondering what this code actually does.  It would be nice to have
> an overall design and implementation description in the changelog so we
> don't have to reverse-engineer your thoughts from your implementation...

OK, I'll include a more detailed description in the next round of the
patches.
 
> > +#ifdef __i386__
> 
> CONFIG_X86 would be more typical.
> 
> Be aware that CONFIG_X86 is true for both i386 and x86_64.  You don't state
> whether this code works on x86_64.  If it can, it should.

AFAIK, it can't.  PMI code is meant to be run in 32-bit protected mode.
I'll add a note about this to the patch and change __i386__ to
CONFIG_X86_32.
 
> You might care to cc "H.  Peter Anvin" <hpa@zytor.com> on the next version
> of this patch - he's a whizz at this sort of low-level x86 bios
> communication stuff.

Thanks, I'll do that.
 
> > +	if (!request_mem_region(info->fix.smem_start, info->fix.smem_len,
> > +	    "uvesafb")) {
> > +		printk(KERN_WARNING "uvesafb: cannot reserve video memory at "
> > +		       "0x%lx\n", info->fix.smem_start);
> > +		/* We cannot make this fatal. Sometimes this comes from magic
> > +		   spaces our resource handlers simply don't know about. */
> 
> so...  what happens?  The driver starts altering mrmory regions which it
> doesn't own?

Fixed.  This was a leftover from vesafb.c.  Are there any reasons for not
fixing it there as well?
 
> > +#ifndef MODULE
> > +static int __devinit uvesafb_setup(char *options)
> > +{
> > +	char *this_opt;
> > +
> > +	if (!options || !*options)
> > +		return 0;
> > +
> > +	while ((this_opt = strsep(&options, ",")) != NULL) {
> > +		if (!*this_opt) continue;
> > +
> > +		if (!strcmp(this_opt, "redraw"))
> > +			ypan = 0;
> > +		else if (!strcmp(this_opt, "ypan"))
> > +			ypan = 1;
> > +		else if (!strcmp(this_opt, "ywrap"))
> > +			ypan = 2;
> > +		else if (!strcmp(this_opt, "vgapal"))
> > +			pmi_setpal = 0;
> > +		else if (!strcmp(this_opt, "pmipal"))
> > +			pmi_setpal = 1;
> > +		else if (!strncmp(this_opt, "mtrr:", 5))
> > +			mtrr = simple_strtoul(this_opt+5, NULL, 0);
> > +		else if (!strcmp(this_opt, "nomtrr"))
> > +			mtrr = 0;
> > +		else if (!strcmp(this_opt, "nocrtc"))
> > +			nocrtc = 1;
> > +		else if (!strcmp(this_opt, "noedid"))
> > +			noedid = 1;
> > +		else if (!strcmp(this_opt, "noblank"))
> > +			blank = 0;
> > +		else if (!strncmp(this_opt, "vtotal:", 7))
> > +			vram_total = simple_strtoul(this_opt + 7, NULL, 0);
> > +		else if (!strncmp(this_opt, "vremap:", 7))
> > +			vram_remap = simple_strtoul(this_opt + 7, NULL, 0);
> > +		else if (!strncmp(this_opt, "maxhf:", 6))
> > +			maxhf = simple_strtoul(this_opt + 6, NULL, 0);
> > +		else if (!strncmp(this_opt, "maxvf:", 6))
> > +			maxvf = simple_strtoul(this_opt + 6, NULL, 0);
> > +		else if (!strncmp(this_opt, "maxclk:", 7))
> > +			maxclk = simple_strtoul(this_opt + 7, NULL, 0);
> > +		else if (!strncmp(this_opt, "vbemode:", 8))
> > +			vbemode = simple_strtoul(this_opt + 8, NULL,0);
> > +		else if (this_opt[0] >= '0' && this_opt[0] <= '9') {
> > +			mode_option = this_opt;
> > +		} else {
> > +			printk(KERN_WARNING
> > +			       "uvesafb: unrecognized option %s\n", this_opt);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +#endif /* !MODULE */
> 
> The #ifdef MODULE is unpleasing.  Can we use module_param() here?  That'll
> work for both modular and non-modular case.

True, but it would also make uvesafb the only (?) fb driver that
doesn't support the "video=smthfb:<options>" way of setting options.
Unless it's considered deprecated, I think it would be best to leave
it as it is, for the sake of consistency.
 
Thanks for all your comments & suggestions, and I'm sorry for the screw-up
with checkpatch.pl.  I've fixed the code in my git tree and the updates
will be included in the next version of this patch.

Best regards,
Michal


  reply	other threads:[~2007-06-23 23:20 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-06-23 10:52 [PATCH 3/4] fbdev: uvesafb driver Michal Januszewski
2007-06-23 11:51 ` [Linux-fbdev-devel] " Bernhard Fischer
2007-06-23 18:35 ` Andrew Morton
2007-06-23 23:20   ` Michal Januszewski [this message]
2007-06-23 23:36     ` Andrew Morton
2007-06-24 11:15       ` Michal Januszewski
2007-06-24  3:50 ` Akinobu Mita

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070623232033.GB7148@spock.one.pl \
    --to=spock@gentoo.org \
    --cc=akpm@linux-foundation.org \
    --cc=linux-fbdev-devel@lists.sourceforge.net \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).