LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Cai, Crane" <Crane.Cai@amd.com>
To: "Grant Grundler" <grundler@parisc-linux.org>
Cc: "Matthew Wilcox" <matthew@wil.cx>,
	"Greg Kroah-Hartman" <gregkh@suse.de>,
	<linux-pci@atrey.karlin.mff.cuni.cz>,
	<linux-kernel@vger.kernel.org>,
	"Linus Torvalds" <torvalds@osdl.org>,
	"Andrew Morton" <akpm@osdl.org>
Subject: RE: [PATCH 03/10] PCI: AMD SATA IDE mode quirk
Date: Mon, 25 Feb 2008 16:00:48 +0800	[thread overview]
Message-ID: <206C6845C72CAE439DC3D9D413C571A49664E8@ssuzexmb3.amd.com> (raw)
In-Reply-To: <20080225073100.GE26674@colo.lackof.org>

> On Mon, Feb 25, 2008 at 09:43:59AM +0800, Cai, Crane wrote:
> > > On Fri, Feb 22, 2008 at 01:49:20PM +0800, Cai, Crane wrote:
> > > > > On Thu, Feb 21, 2008 at 03:47:33PM -0800, Greg
> > > Kroah-Hartman wrote:
> > > > > > +static void __devinit quirk_amd_ide_mode(struct pci_dev 
> > > > > > +*pdev)
> > > > > >  {
> > > > > > -	/* set sb600 sata to ahci mode */
> > > > > > -	if ((pdev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
> > > > > > -		u8 tmp;
> > > > > > +	/* set sb600/sb700/sb800 sata to ahci mode */
> > > > > > +	u8 tmp;
> > > > > >  
> > > > > > +	pci_read_config_byte(pdev, PCI_CLASS_DEVICE, &tmp);
> > > > > > +	if (tmp == 0x01) {
> > > > > >  		pci_read_config_byte(pdev, 0x40, &tmp);
> > > > > 
> > > > > This seems like a dis-improvement.  Why are we reading a
> > > config byte
> > > > > for something we already have in the pci_dev?
> > > > > Why are we now checking against 0x01 instead of a
> > > symbolic constant?  
> > > > > Why are we no longer checking that this is 
> PCI_BASE_CLASS_STORAGE?
> > > > It is a quirk. In pci_ids.h did have PCI_CLASS_STORAGE_IDE and 
> > > > PCI_BASE_CLASS_STORAGE, these can not represent the right
> > > situation we
> > > > want to check. 0x01 represents PCI_CLASS_STORAGE_IDE last 2
> > > bit. Also
> > > > because it is a quirk, I do not think we need to change
> > > pci_ids.h. So
> > > > 0x01 used.
> > > 
> > > You haven't explained what is wrong with the original code:
> > > 
> > > 	if ((pdev->class >> 8) == PCI_CLASS_STORAGE_IDE) {
> > > 
> > 
> > When resume, this pdev->class is quirked, however BIOS has modified 
> > pci configuration too. Inconsistance occurs.
> 
> Can you update pdev->class from the quirk?
I think we can, there are many places where pdev->class is changed in quirks.c.
> It would be consistent then.
> That would leave the code as-is except it's re-reading the 
> field from config space.
> 
> hth,
> grant

> > > > > Nothing in the changelog entry suggests why we now need
> > > FIXUP_RESUME
> > > > > entries when we didn't before.
> > > > > 
> > > > PCI configuration space will be changed by BIOS and then in
> > > pci init
> > > > and restore. So resume also needed.
> > > 
> > > That information needed to be in the changelog.
> > 
> > This info, is a normal info. If maintainer need us to added 
> in source code. I preferred too. 
> > > --
> > > Intel are signing my paycheques ... these opinions are still mine 
> > > "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."
> > > 
> > > 
> > > 
> 
> 
> 


      reply	other threads:[~2008-02-25  8:02 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-21 23:46 [GIT PATCH] PCI fixes for 2.6.25-rc2 git tree Greg KH
     [not found] ` <1203637660-7247-3-git-send-email-gregkh@suse.de>
2008-02-22  4:17   ` [PATCH 03/10] PCI: AMD SATA IDE mode quirk Matthew Wilcox
2008-02-22  5:49     ` Cai, Crane
2008-02-22 11:11       ` Matthew Wilcox
2008-02-25  1:43         ` Cai, Crane
2008-02-25  7:31           ` Grant Grundler
2008-02-25  8:00             ` Cai, Crane [this message]

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=206C6845C72CAE439DC3D9D413C571A49664E8@ssuzexmb3.amd.com \
    --to=crane.cai@amd.com \
    --cc=akpm@osdl.org \
    --cc=gregkh@suse.de \
    --cc=grundler@parisc-linux.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@atrey.karlin.mff.cuni.cz \
    --cc=matthew@wil.cx \
    --cc=torvalds@osdl.org \
    --subject='RE: [PATCH 03/10] PCI: AMD SATA IDE mode quirk' \
    /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

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).