linux-kernel - Re: [PATCH] via-agp.c fixed compilation error and warnings for 2.6.26

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] [day] [month] [year] [list]

Message-ID: <20081021042353.GL26184@parisc-linux.org>
Date:	Mon, 20 Oct 2008 22:23:54 -0600
From:	Matthew Wilcox <matthew@....cx>
To:	Dave Airlie <airlied@...ux.ie>
Cc:	nagaraj s k <nagaraj.sk@...il.com>, trivial@...nel.org,
	greg@...ah.com, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org,
	nagaraj.krishnappa@...msonreuters.com
Subject: Re: [PATCH] via-agp.c fixed compilation error and warnings for 2.6.26

On Tue, Oct 21, 2008 at 04:57:13AM +0100, Dave Airlie wrote:
> > > -    printk(KERN_ERR PFX "Unknown aperture size from AGP bridge (0x%x)\n",
> > > temp);
> > > +    printk(KERN_ERR PFX "Unknown aperture size from AGP
> > > +        bridge (0x%x)\n", temp);
> 
> Uglier code.

Not just that, but it won;t even compile with a recent compiler.  You'd
need to do it as:

	printk(KERN_ERR PFX "Unknown aperture size from AGP "
		"bridge (0x%x)\n", temp);

and that would seem like not much of a win.  What might be a win would
be:
	dev_err(&agp_bridge->dev->dev, "Unknown AGP aperture size 0x%x\n",
		temp);

> > >          if (temp == values[i].size_value) {
> > >              agp_bridge->previous_size =
> > > -                agp_bridge->current_size = (void *) (values + i);
> > > +                agp_bridge->current_size =
> > > +                     (void *) (values + i);
> 
> arguably uglier code.

No argument about it ... it's uglier.  The (void *) cast is unnecessary.

What I'd do to this function is:

        for (i = 0; i < agp_bridge->driver->num_aperture_sizes; i++) {
		if (temp != values[i].size_value)
			continue;

		agp_bridge->previous_size = agp_bridge->current_size =
			values + i;
		agp_bridge->aperture_size_idx = i;
		return values[i].size;
	}

	dev_err(&agp_bridge->dev->dev, "Unknown AGP aperture size 0x%x\n",
		temp);
	return 0;
}


> > > @@ -142,11 +144,12 @@ static int via_configure_agp3(void)
> > >
> > >      /* 1. Enable GTLB in RX90<7>, all AGP aperture access needs to fetch
> > >       *    translation table first.
> > > -     * 2. Enable AGP aperture in RX91<0>. This bit controls the enabling
> > > of the
> > > -     *    graphics AGP aperture for the AGP3.0 port.
> > > +     * 2. Enable AGP aperture in RX91<0>. This bit controls the
> > > +     * enabling of the graphics AGP aperture for the AGP3.0 port.
> 
> this is okay.

Except the missing spaces between the '*' and 'enabling'.

> > >       */
> > >      pci_read_config_dword(agp_bridge->dev, VIA_AGP3_GARTCTRL, &temp);
> > > -    pci_write_config_dword(agp_bridge->dev, VIA_AGP3_GARTCTRL, temp |
> > > (3<<7));
> > > +    pci_write_config_dword(agp_bridge->dev, VIA_AGP3_GARTCTRL,
> > > +                         temp | (3<<7));
> 
> this one is probably okay.

I'd add some more spacing:

+				temp | (3 << 7));

The driver seems to suffer from a lack of spaces around << throughout.
And most of those << should probably be defines somewhere ...

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
--
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/