Patchwork [v2,1/4] PCI/quirks: Fix PIIX4 memory base and size mask

login
register
mail settings
Submitter Deng-Cheng Zhu
Date Oct. 4, 2013, 11:30 p.m.
Message ID <1380929453-15428-2-git-send-email-dengcheng.zhu@imgtec.com>
Download mbox | patch
Permalink /patch/280744/
State Rejected
Headers show

Comments

Deng-Cheng Zhu - Oct. 4, 2013, 11:30 p.m.
From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>

According to Intel PIIX4 datasheet: The memory address consists of a 17-bit
base address (AD[31:15]) and a 7-bit mask (AD[21:15]). This provides memory
ranges from 32 Kbytes to 4 Mbytes in size.

This is true for both devices 12 and 13. This patch fixes it.

Ref 1: www.intel.com/assets/pdf/datasheet/290562.pdf
       April 1997 version 001 (p131 bottom, p226 top)
Ref 2: www.intel.com/assets/pdf/specupdate/297738.pdf
       January 2002 version 017 (Nothing against the fix in the
       specification update was found.)

Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
Reviewed-by: James Hogan <james.hogan@imgtec.com>
---
 drivers/pci/quirks.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Bjorn Helgaas - Oct. 5, 2013, 2:13 a.m.
On Fri, Oct 4, 2013 at 5:30 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
> From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>
> According to Intel PIIX4 datasheet: The memory address consists of a 17-bit
> base address (AD[31:15]) and a 7-bit mask (AD[21:15]). This provides memory
> ranges from 32 Kbytes to 4 Mbytes in size.
>
> This is true for both devices 12 and 13. This patch fixes it.

Does this fix a user-visible problem?  If so, what does it look like
when the problem occurs?

I assume the other patches (2-4) are cleanups that don't fix any problems.

> Ref 1: www.intel.com/assets/pdf/datasheet/290562.pdf
>        April 1997 version 001 (p131 bottom, p226 top)
> Ref 2: www.intel.com/assets/pdf/specupdate/297738.pdf
>        January 2002 version 017 (Nothing against the fix in the
>        specification update was found.)
>
> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
> Reviewed-by: James Hogan <james.hogan@imgtec.com>
> ---
>  drivers/pci/quirks.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index f6c31fa..3e7e489 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -414,9 +414,9 @@ static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int
>         pci_read_config_dword(dev, port, &devres);
>         if ((devres & enable) != enable)
>                 return;
> -       base = devres & 0xffff0000;
> -       mask = (devres & 0x3f) << 16;
> -       size = 128 << 16;
> +       base = devres & 0xffff8000;
> +       mask = (devres & 0x7f) << 15;
> +       size = 128 << 15;
>         for (;;) {
>                 unsigned bit = size >> 1;
>                 if ((bit & mask) == bit)
> --
> 1.7.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Deng-Cheng Zhu - Oct. 5, 2013, 3:42 a.m.
> Does this fix a user-visible problem?  If so, what does it look like
> when the problem occurs?

No, I found the problem while debugging another issue and trying to
understand this piece of code and after looking into different versions of
PIIX4 datasheets. But I don't think it should prevent such a fix because
the code is straightforward and the spec is clear enough. If the existing
encoding was intentionally made like this by the code author due to the
inaccuracy of the spec, then it's very likely some code comments were
placed here. There are 2 possibilities:

- This fix breaks something. People should have to bisect the problem.
- This fix is valid. Some day people need PIIX4 mem quirks and they don't
  have to run into a possibly well-hidden issue.
  
What do you think?

> I assume the other patches (2-4) are cleanups that don't fix any
> problems.

Definitely, as indicated by patch titles and commit messages.


Deng-Cheng
Bjorn Helgaas - Oct. 5, 2013, 4:37 a.m.
On Fri, Oct 4, 2013 at 9:42 PM, DengCheng Zhu <DengCheng.Zhu@imgtec.com> wrote:
>> Does this fix a user-visible problem?  If so, what does it look like
>> when the problem occurs?
>
> No, I found the problem while debugging another issue and trying to
> understand this piece of code and after looking into different versions of
> PIIX4 datasheets. But I don't think it should prevent such a fix because
> the code is straightforward and the spec is clear enough. If the existing
> encoding was intentionally made like this by the code author due to the
> inaccuracy of the spec, then it's very likely some code comments were
> placed here. There are 2 possibilities:
>
> - This fix breaks something. People should have to bisect the problem.
> - This fix is valid. Some day people need PIIX4 mem quirks and they don't
>   have to run into a possibly well-hidden issue.
>
> What do you think?

Don't worry, I'm willing to fix it even if nobody has actually
reported a problem.  It's just nice to include the symptoms if
somebody *has* reported it, so when other people see the same symptom,
they can more easily find the fix.

Bjorn

>> I assume the other patches (2-4) are cleanups that don't fix any
>> problems.
>
> Definitely, as indicated by patch titles and commit messages.
>
>
> Deng-Cheng
>
> ________________________________________
> From: Bjorn Helgaas [bhelgaas@google.com]
> Sent: Friday, October 04, 2013 7:13 PM
> To: DengCheng Zhu
> Cc: linux-pci@vger.kernel.org; James Hogan; Qais Yousef
> Subject: Re: [PATCH v2 1/4] PCI/quirks: Fix PIIX4 memory base and size mask
>
> On Fri, Oct 4, 2013 at 5:30 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
>> From: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>>
>> According to Intel PIIX4 datasheet: The memory address consists of a 17-bit
>> base address (AD[31:15]) and a 7-bit mask (AD[21:15]). This provides memory
>> ranges from 32 Kbytes to 4 Mbytes in size.
>>
>> This is true for both devices 12 and 13. This patch fixes it.
>
> Does this fix a user-visible problem?  If so, what does it look like
> when the problem occurs?
>
> I assume the other patches (2-4) are cleanups that don't fix any problems.
>
>> Ref 1: www.intel.com/assets/pdf/datasheet/290562.pdf
>>        April 1997 version 001 (p131 bottom, p226 top)
>> Ref 2: www.intel.com/assets/pdf/specupdate/297738.pdf
>>        January 2002 version 017 (Nothing against the fix in the
>>        specification update was found.)
>>
>> Signed-off-by: Deng-Cheng Zhu <dengcheng.zhu@imgtec.com>
>> Reviewed-by: James Hogan <james.hogan@imgtec.com>
>> ---
>>  drivers/pci/quirks.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index f6c31fa..3e7e489 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -414,9 +414,9 @@ static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int
>>         pci_read_config_dword(dev, port, &devres);
>>         if ((devres & enable) != enable)
>>                 return;
>> -       base = devres & 0xffff0000;
>> -       mask = (devres & 0x3f) << 16;
>> -       size = 128 << 16;
>> +       base = devres & 0xffff8000;
>> +       mask = (devres & 0x7f) << 15;
>> +       size = 128 << 15;
>>         for (;;) {
>>                 unsigned bit = size >> 1;
>>                 if ((bit & mask) == bit)
>> --
>> 1.7.1
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Deng-Cheng Zhu - Oct. 7, 2013, 9:14 p.m.
[Resend to the mailing list due to the bounceback caused by html format]

On 10/04/2013 09:37 PM, Bjorn Helgaas wrote:
> On Fri, Oct 4, 2013 at 9:42 PM, DengCheng Zhu <DengCheng.Zhu@imgtec.com> wrote:
>>> Does this fix a user-visible problem?  If so, what does it look like
>>> when the problem occurs?
>> No, I found the problem while debugging another issue and trying to
>> understand this piece of code and after looking into different versions of
>> PIIX4 datasheets. But I don't think it should prevent such a fix because
>> the code is straightforward and the spec is clear enough. If the existing
>> encoding was intentionally made like this by the code author due to the
>> inaccuracy of the spec, then it's very likely some code comments were
>> placed here. There are 2 possibilities:
>>
>> - This fix breaks something. People should have to bisect the problem.
>> - This fix is valid. Some day people need PIIX4 mem quirks and they don't
>>    have to run into a possibly well-hidden issue.
>>
>> What do you think?
> Don't worry, I'm willing to fix it even if nobody has actually
> reported a problem.  It's just nice to include the symptoms if
> somebody *has* reported it, so when other people see the same symptom,
> they can more easily find the fix.

Ah, I see. Thanks.

So far I didn't see them on any branch in kernel/git/helgaas/pci.git 
<https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/>
Is this the right place to look at? Thanks.


Deng-Cheng


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Oct. 8, 2013, 11:13 p.m.
On Mon, Oct 7, 2013 at 3:14 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
> [Resend to the mailing list due to the bounceback caused by html format]
>
>
> On 10/04/2013 09:37 PM, Bjorn Helgaas wrote:
>>
>> On Fri, Oct 4, 2013 at 9:42 PM, DengCheng Zhu <DengCheng.Zhu@imgtec.com>
>> wrote:
>>>>
>>>> Does this fix a user-visible problem?  If so, what does it look like
>>>> when the problem occurs?
>>>
>>> No, I found the problem while debugging another issue and trying to
>>> understand this piece of code and after looking into different versions
>>> of
>>> PIIX4 datasheets. But I don't think it should prevent such a fix because
>>> the code is straightforward and the spec is clear enough. If the existing
>>> encoding was intentionally made like this by the code author due to the
>>> inaccuracy of the spec, then it's very likely some code comments were
>>> placed here. There are 2 possibilities:
>>>
>>> - This fix breaks something. People should have to bisect the problem.
>>> - This fix is valid. Some day people need PIIX4 mem quirks and they don't
>>>    have to run into a possibly well-hidden issue.
>>>
>>> What do you think?
>>
>> Don't worry, I'm willing to fix it even if nobody has actually
>> reported a problem.  It's just nice to include the symptoms if
>> somebody *has* reported it, so when other people see the same symptom,
>> they can more easily find the fix.
>
>
> Ah, I see. Thanks.
>
> So far I didn't see them on any branch in kernel/git/helgaas/pci.git
> <https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/>

This change definitely makes the code match the specs you mentioned.
But we don't have any problem reports, and when Linus added this code
(6693e74a "PCI: be more verbose about resource quirks), he presumably
was looking at a spec, too.  It's possible he transcribed the masks
incorrectly, but twice in one patch?

The PIIX4 is an old part, and for something like that I would
generally say "don't touch it" because the risk of breakage outweighs
a possible fix for machines nobody uses anymore.

However, I see that the PIIX4 is emulated in, e.g., qemu, so it's
still relevant.  But before I apply this, can you please research qemu
and how it uses these registers?  For example, if you can show that
qemu emulates the registers with the additional bits you add to the
masks here, then we should be able to make linux act incorrectly by
setting those bits in a qemu BIOS.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Deng-Cheng Zhu - Oct. 9, 2013, 12:29 a.m.
On 10/08/2013 04:13 PM, Bjorn Helgaas wrote:
> On Mon, Oct 7, 2013 at 3:14 PM, Deng-Cheng Zhu <dengcheng.zhu@imgtec.com> wrote:
>> [Resend to the mailing list due to the bounceback caused by html format]
>>
>>
>> On 10/04/2013 09:37 PM, Bjorn Helgaas wrote:
>>> On Fri, Oct 4, 2013 at 9:42 PM, DengCheng Zhu <DengCheng.Zhu@imgtec.com>
>>> wrote:
>>>>> Does this fix a user-visible problem?  If so, what does it look like
>>>>> when the problem occurs?
>>>> No, I found the problem while debugging another issue and trying to
>>>> understand this piece of code and after looking into different versions
>>>> of
>>>> PIIX4 datasheets. But I don't think it should prevent such a fix because
>>>> the code is straightforward and the spec is clear enough. If the existing
>>>> encoding was intentionally made like this by the code author due to the
>>>> inaccuracy of the spec, then it's very likely some code comments were
>>>> placed here. There are 2 possibilities:
>>>>
>>>> - This fix breaks something. People should have to bisect the problem.
>>>> - This fix is valid. Some day people need PIIX4 mem quirks and they don't
>>>>     have to run into a possibly well-hidden issue.
>>>>
>>>> What do you think?
>>> Don't worry, I'm willing to fix it even if nobody has actually
>>> reported a problem.  It's just nice to include the symptoms if
>>> somebody *has* reported it, so when other people see the same symptom,
>>> they can more easily find the fix.
>>
>> Ah, I see. Thanks.
>>
>> So far I didn't see them on any branch in kernel/git/helgaas/pci.git
>> <https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/>
> This change definitely makes the code match the specs you mentioned.
> But we don't have any problem reports,
It's very likely that nobody really used PIIX4 ACPI devices 12 and 13. 
Maybe some day they'll be used, or maybe they can be deleted from the code. 
But if they'll be kept in the code, had better fix it IMO.

> and when Linus added this code
> (6693e74a "PCI: be more verbose about resource quirks), he presumably
> was looking at a spec, too.  It's possible he transcribed the masks
> incorrectly, but twice in one patch?
>
> The PIIX4 is an old part, and for something like that I would
> generally say "don't touch it" because the risk of breakage outweighs
> a possible fix for machines nobody uses anymore.
>
> However, I see that the PIIX4 is emulated in, e.g., qemu, so it's
> still relevant.  But before I apply this, can you please research qemu
> and how it uses these registers?  For example, if you can show that
> qemu emulates the registers with the additional bits you add to the
> masks here, then we should be able to make linux act incorrectly by
> setting those bits in a qemu BIOS.

QEMU doesn't emulate PIIX4 ACPI devices 12 & 13, so these registers are not 
used in it.


Deng-Cheng


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f6c31fa..3e7e489 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -414,9 +414,9 @@  static void piix4_mem_quirk(struct pci_dev *dev, const char *name, unsigned int
 	pci_read_config_dword(dev, port, &devres);
 	if ((devres & enable) != enable)
 		return;
-	base = devres & 0xffff0000;
-	mask = (devres & 0x3f) << 16;
-	size = 128 << 16;
+	base = devres & 0xffff8000;
+	mask = (devres & 0x7f) << 15;
+	size = 128 << 15;
 	for (;;) {
 		unsigned bit = size >> 1;
 		if ((bit & mask) == bit)