linux-kernel - Re: [PATCH -mm] working 3D/DRI intel-agp.ko resume for i815 chip; Intel chipset testers wanted! (was: Re: intel-agp PM experiences ...)

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: <20070503154736.GA23922@redhat.com>
Date:	Thu, 3 May 2007 11:47:36 -0400
From:	Dave Jones <davej@...hat.com>
To:	Pavel Machek <pavel@....cz>
Cc:	Andreas Mohr <andi@...x01.fht-esslingen.de>,
	Andrew Morton <akpm@...l.org>, linux-acpi@...r.kernel.org,
	Matthew Garrett <mjg59@...f.ucam.org>,
	kernel list <linux-kernel@...r.kernel.org>, andi@...as.de
Subject: Re: [PATCH -mm] working 3D/DRI intel-agp.ko resume for i815 chip; Intel chipset testers wanted! (was: Re: intel-agp PM experiences ...)

On Wed, May 02, 2007 at 12:17:18PM +0200, Pavel Machek wrote:

 > >  #define INTEL_I820_RDCR		0x51
 > > @@ -664,7 +671,7 @@
 > >  	if ((pg_start + mem->page_count) > num_entries)
 > >  		goto out_err;
 > >  
 > > -	/* The i830 can't check the GTT for entries since its read only,
 > > +	/* The i830 can't check the GTT for entries since it's read-only,
 > >  	 * depend on the caller to make the correct offset decisions.
 > >  	 */
 > >  
 > > @@ -787,7 +794,7 @@
 > >  	if ((pg_start + mem->page_count) > num_entries)
 > >  		goto out_err;
 > >  
 > > -	/* The i915 can't check the GTT for entries since its read only,
 > > +	/* The i915 can't check the GTT for entries since it's read-only,
 > >  	 * depend on the caller to make the correct offset decisions.
 > >  	 */
 > 
 > You should not do cleanups at the same time as code changes.

Indeed.

 > > @@ -1103,8 +1110,8 @@
 > >  	/* attbase - aperture base */
 > >  	/* the Intel 815 chipset spec. says that bits 29-31 in the
 > >  	* ATTBASE register are reserved -> try not to write them */
 > > -	if (agp_bridge->gatt_bus_addr & INTEL_815_ATTBASE_MASK) {
 > > -		printk (KERN_EMERG PFX "gatt bus addr too high");
 > > +	if (agp_bridge->gatt_bus_addr & I815_ATTBASE_MASK) {
 > > +		printk (KERN_EMERG PFX "gatt bus addr too high\n");
 > >  		return -EINVAL;
 > >  	}
 > >  
 > > @@ -1119,7 +1126,7 @@
 > >  	agp_bridge->gart_bus_addr = (temp & PCI_BASE_ADDRESS_MEM_MASK);
 > >  
 > >  	pci_read_config_dword(agp_bridge->dev, INTEL_ATTBASE, &addr);
 > > -	addr &= INTEL_815_ATTBASE_MASK;
 > > +	addr &= I815_ATTBASE_MASK;
 > >  	addr |= agp_bridge->gatt_bus_addr;
 > >  	pci_write_config_dword(agp_bridge->dev, INTEL_ATTBASE, addr);
 > 
 > And I guess macro search&replace counts as cleanup, too. It would be
 > nice to submit them separately and first.

I'm not a big fan of the s/INTEL_/I_/ change to be honest.
I'd prefer we didn't change this, as a) there may be additional
patches in flight which touch intel-agp.c depending on those defines,
b) it'll make future changes to intel-agp.c harder to backport to
-stable releases, and c) it doesn't really add any value.

 > > @@ -1355,7 +1362,7 @@
 > >  	pci_write_config_dword(agp_bridge->dev, INTEL_ATTBASE, agp_bridge->gatt_bus_addr);
 > >  
 > >  	/* agpctrl */
 > > -	pci_write_config_dword(agp_bridge->dev, INTEL_AGPCTRL, 0x0000);
 > > +	pci_write_config_dword(agp_bridge->dev, INTEL_AGPCTRL, 0x00000000);
 > 
 > Huh?

harmless, but ok.

 > > +			pci_read_config_dword(pdev, p->reg, &p->value);
 > > +	}
 > > +	return 1;
 > > +}
 > 
 > Should this betwo functions, one for save, one for restore, with "to
 > save" array being global?

yes.

 > > +/*
 > > + * set DEBUG_AGP_PM to 1 if your AGP chipset has issues resuming
 > > + * (machine lockups due to non-restored hardware registered values),
 > > + * then figure out from the log which registers have to be restored manually,
 > > + * then add specific support for your chipset, similar to what already exists
 > 
 > Can we get debugging stuff separately?

ACK.

 > > @@ -2106,6 +2282,12 @@
 > >  {
 > >  	if (agp_off)
 > >  		return -EINVAL;
 > > +	/* let people know that this is an important place to investigate at in case of resume lockups */
 > > +	printk(KERN_INFO PFX
 > > +		"suspend/resume of intel-agp.ko is NOT always stable for all Intel AGP\n"
 > > +		"chipset/BIOS combos, may cause resume lockups when DRI (3D accel) active,\n"
 > > +		"in AGP (non-PCI) mode! (see DEBUG_AGP_PM in intel-agp.c source to investigate)\n"
 > > +	);
 > 
 > I'm not sure if we want such big/ugly warnings. Can you get it to one
 > line, at least?

I'd drop this completely. The majority of users will never see it anyway,
and the boot process already spews so much stuff that it'll just get lost
in the noise.

Thanks,

	Dave

-- 
http://www.codemonkey.org.uk
-
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/