LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Cai, Crane" <Crane.Cai@amd.com>
To: "Matthew Wilcox" <matthew@wil.cx>
Cc: "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 09:43:59 +0800	[thread overview]
Message-ID: <206C6845C72CAE439DC3D9D413C571A49663E6@ssuzexmb3.amd.com> (raw)
In-Reply-To: <20080222111105.GE16995@parisc-linux.org>

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

> > > 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  1:44 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 [this message]
2008-02-25  7:31           ` Grant Grundler
2008-02-25  8:00             ` Cai, Crane

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=206C6845C72CAE439DC3D9D413C571A49663E6@ssuzexmb3.amd.com \
    --to=crane.cai@amd.com \
    --cc=akpm@osdl.org \
    --cc=gregkh@suse.de \
    --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).