linux-kernel - Re: [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110220175349.GD1898@bicker>
Date:	Sun, 20 Feb 2011 20:53:49 +0300
From:	Dan Carpenter <error27@...il.com>
To:	Javier Martinez Canillas <martinez.javier@...il.com>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>,
	Randy Dunlap <randy.dunlap@...cle.com>,
	devel@...verdev.osuosl.org, kernel-janitors@...r.kernel.org,
	linux-kernel@...r.kernel.org, Arnaud Patard <apatard@...driva.com>
Subject: Re: [PATCH 1/2] Staging: xgifb: Remove CRIT[FLAGS | BEGIN | END]
 defines usage

Hi Javier,

Your idea was good but there are a couple problems with this patch.  I'm
afraid you're going to need to fix it and resend.

You put the patch description in the 0/2 patch.  It was a pretty decent
patch description.  Unfortunately the 0/2 descriptions get thrown away,
and do not get included in the final git log.  So they should go with
the individual patches.

On Sun, Feb 20, 2011 at 05:53:17PM +0100, Javier Martinez Canillas wrote:
>  int fbcon_XGI_sync(struct fb_info *info)
>  {
> -    if(!XGIfb_accel) return 0;
> -    CRITFLAGS
> +	unsigned long critflags = 0;

It's better to just call it "flags" instead of "critflags" so that it's
consistent with the rest of the kernel.

It's also not a good idea to initialize it to zero.  I know that gcc
prints a warning, but you can't just set it to zero to silence the
warning. Also this change should have been mentioned in the patch
description.  Every behavior change should be described in the change
log.

>  
> -    XGI310Sync();
> +	if (!XGIfb_accel)
> +		return 0;
> +
> +	XGI310Sync();
>  
> -   CRITEND
> -   return 0;
> +	spin_unlock_irqrestore(&xgi_video_info.lockaccel, critflags);

I know you didn't introduce it, but this makes no sense at all.  It's a
random unlock, and in the original code critflags was uninitialized.

The only reason this works is because XGIfb_accel is always 0.  So you 
can remove this function, and remove all the code that depends on
XGIfb_accel.  (In a separate patch of course).

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/