diff mbox series

[v3,3/4] PCI/ACPI: Add Broadcom bcm2711 MCFG quirk

Message ID 20210826071557.29239-4-jeremy.linton@arm.com
State New
Headers show
Series CM4 ACPI PCIe quirk | expand

Commit Message

Jeremy Linton Aug. 26, 2021, 7:15 a.m. UTC
Now that there is a bcm2711 quirk, it needs to be enabled when the
MCFG is missing. Use an ACPI namespace _DSD property
"linux-ecam-quirk-id" as an alternative to the MCFG OEM.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

nicolas saenz julienne Aug. 30, 2021, 8:37 a.m. UTC | #1
On Thu, 2021-08-26 at 02:15 -0500, Jeremy Linton wrote:
> Now that there is a bcm2711 quirk, it needs to be enabled when the
> MCFG is missing. Use an ACPI namespace _DSD property
> "linux-ecam-quirk-id" as an alternative to the MCFG OEM.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> ---

Acked-by: Nicolas Saenz Julienne <nsaenz@kernel.org>

Regards,
Nicolas
Rafael J. Wysocki Sept. 13, 2021, 4:12 p.m. UTC | #2
On Thu, Aug 26, 2021 at 9:16 AM Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> Now that there is a bcm2711 quirk, it needs to be enabled when the
> MCFG is missing. Use an ACPI namespace _DSD property
> "linux-ecam-quirk-id" as an alternative to the MCFG OEM.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index 53cab975f612..04c517418365 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = {
>         ALTRA_ECAM_QUIRK(1, 13),
>         ALTRA_ECAM_QUIRK(1, 14),
>         ALTRA_ECAM_QUIRK(1, 15),
> +
> +       { "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops,
> +         DEFINE_RES_MEM(0xFD500000, 0xA000) },
>  };
>
>  static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
>         u16 segment = root->segment;
>         struct resource *bus_range = &root->secondary;
>         struct mcfg_fixup *f;
> +       const char *soc;
>         int i;
>
> +       /*
> +        * This may be a machine with a PCI/SMC conduit, which means it doesn't
> +        * have an MCFG. Use an ACPI namespace definition instead.
> +        */
> +       if (!fwnode_property_read_string(acpi_fwnode_handle(root->device),
> +                                        "linux-ecam-quirk-id", &soc)) {
> +               if (strlen(soc) != ACPI_OEM_ID_SIZE)
> +                       dev_err(&root->device->dev, "ECAM quirk should be %d characters\n",
> +                               ACPI_OEM_ID_SIZE);
> +               else
> +                       memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE);
> +       }
> +
>         for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
>                 if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
>                         if (f->cfgres.start)
> --
> 2.31.1
>
Bjorn Helgaas Oct. 5, 2021, 3:10 p.m. UTC | #3
On Thu, Aug 26, 2021 at 02:15:56AM -0500, Jeremy Linton wrote:
> Now that there is a bcm2711 quirk, it needs to be enabled when the
> MCFG is missing. Use an ACPI namespace _DSD property
> "linux-ecam-quirk-id" as an alternative to the MCFG OEM.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index 53cab975f612..04c517418365 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = {
>  	ALTRA_ECAM_QUIRK(1, 13),
>  	ALTRA_ECAM_QUIRK(1, 14),
>  	ALTRA_ECAM_QUIRK(1, 15),
> +
> +	{ "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops,
> +	  DEFINE_RES_MEM(0xFD500000, 0xA000) },
>  };
>  
>  static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
>  	u16 segment = root->segment;
>  	struct resource *bus_range = &root->secondary;
>  	struct mcfg_fixup *f;
> +	const char *soc;
>  	int i;
>  
> +	/*
> +	 * This may be a machine with a PCI/SMC conduit, which means it doesn't
> +	 * have an MCFG. Use an ACPI namespace definition instead.
> +	 */
> +	if (!fwnode_property_read_string(acpi_fwnode_handle(root->device),
> +					 "linux-ecam-quirk-id", &soc)) {
> +		if (strlen(soc) != ACPI_OEM_ID_SIZE)

From a reviewing perspective, it's not obvious why soc must be exactly
ACPI_OEM_ID_SIZE.  Does that imply space-padding in the DT string or
something?

Is there any documentation for this DT property?

Also not obvious why strlen() is safe here.  I mean, I looked a couple
levels deep in fwnode_property_read_string(), but whatever guarantees
null termination is buried pretty deep.

It seems a little weird to use an MCFG quirk mechanism when there's no
MCFG at all on this platform.

> +			dev_err(&root->device->dev, "ECAM quirk should be %d characters\n",
> +				ACPI_OEM_ID_SIZE);
> +		else
> +			memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE);
> +	}
> +
>  	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
>  		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
>  			if (f->cfgres.start)
> -- 
> 2.31.1
>
Jeremy Linton Oct. 5, 2021, 3:43 p.m. UTC | #4
Hi,

Thanks for looking at this again.


On 10/5/21 10:10 AM, Bjorn Helgaas wrote:
> On Thu, Aug 26, 2021 at 02:15:56AM -0500, Jeremy Linton wrote:
>> Now that there is a bcm2711 quirk, it needs to be enabled when the
>> MCFG is missing. Use an ACPI namespace _DSD property
>> "linux-ecam-quirk-id" as an alternative to the MCFG OEM.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>>   drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index 53cab975f612..04c517418365 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = {
>>   	ALTRA_ECAM_QUIRK(1, 13),
>>   	ALTRA_ECAM_QUIRK(1, 14),
>>   	ALTRA_ECAM_QUIRK(1, 15),
>> +
>> +	{ "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops,
>> +	  DEFINE_RES_MEM(0xFD500000, 0xA000) },
>>   };
>>   
>>   static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
>> @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
>>   	u16 segment = root->segment;
>>   	struct resource *bus_range = &root->secondary;
>>   	struct mcfg_fixup *f;
>> +	const char *soc;
>>   	int i;
>>   
>> +	/*
>> +	 * This may be a machine with a PCI/SMC conduit, which means it doesn't
>> +	 * have an MCFG. Use an ACPI namespace definition instead.
>> +	 */
>> +	if (!fwnode_property_read_string(acpi_fwnode_handle(root->device),
>> +					 "linux-ecam-quirk-id", &soc)) {
>> +		if (strlen(soc) != ACPI_OEM_ID_SIZE)
> 
>  From a reviewing perspective, it's not obvious why soc must be exactly
> ACPI_OEM_ID_SIZE.  Does that imply space-padding in the DT string or
> something?

This is at the moment an ACPI only DSD, and it must follow the MADT 
OEM_ID format for now because we are effectively just overriding that 
field. The rest of the code in this module is just treating it as a 
fixed 6 bytes.


> 
> Is there any documentation for this DT property?

Its not a DT property, and its unclear since its linux only if it 
belongs in previously linked ACPI registry.

> 
> Also not obvious why strlen() is safe here.  I mean, I looked a couple
> levels deep in fwnode_property_read_string(), but whatever guarantees
> null termination is buried pretty deep.

I've not tracked down who, if anyone other than the AML compiler is 
guaranteeing a null. The spec says something to the effect "Most other 
string, however,  are of variable-length and are automatically null 
terminated by the compiler". Not sure if that helps any.

> 
> It seems a little weird to use an MCFG quirk mechanism when there's no
> MCFG at all on this platform.

Well its just a point to hook in a CFG space quirk, and since that is 
what most of the MCFG quirks are, it seemed reasonable to reuse it 
rather than recreate it.



PS, had some offline convo about reposing with a simple rebase and the 
ACK's applied, will do that if it helps any, but when I checked a couple 
weeks back this applied to 5.15 automatically.



> 
>> +			dev_err(&root->device->dev, "ECAM quirk should be %d characters\n",
>> +				ACPI_OEM_ID_SIZE);
>> +		else
>> +			memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE);
>> +	}
>> +
>>   	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
>>   		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
>>   			if (f->cfgres.start)
>> -- 
>> 2.31.1
>>
Pali Rohár Oct. 5, 2021, 8:02 p.m. UTC | #5
On Thursday 26 August 2021 02:15:56 Jeremy Linton wrote:
> Now that there is a bcm2711 quirk, it needs to be enabled when the
> MCFG is missing. Use an ACPI namespace _DSD property
> "linux-ecam-quirk-id" as an alternative to the MCFG OEM.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index 53cab975f612..04c517418365 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = {
>  	ALTRA_ECAM_QUIRK(1, 13),
>  	ALTRA_ECAM_QUIRK(1, 14),
>  	ALTRA_ECAM_QUIRK(1, 15),
> +
> +	{ "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops,
> +	  DEFINE_RES_MEM(0xFD500000, 0xA000) },

Hello!

According to discussion described in email [1], adding a new MCFG quirk
(like above) into kernel requires adding some errata entry for
documenting buggy HW.

But this patch series does not introduce any new errata entry.

Bjorn, could you look at how to properly document these "hw bugs"?
I guess there would be lot of more requests for adding MCFG quirks as
now according to [1], doors are open for them. And it is possible that
after more years nobody would be able to maintain these quirks if would
not be properly documented.

[1] - https://lore.kernel.org/linux-pci/20210325131231.GA18590@e121166-lin.cambridge.arm.com/

>  };
>  
>  static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
>  	u16 segment = root->segment;
>  	struct resource *bus_range = &root->secondary;
>  	struct mcfg_fixup *f;
> +	const char *soc;
>  	int i;
>  
> +	/*
> +	 * This may be a machine with a PCI/SMC conduit, which means it doesn't
> +	 * have an MCFG. Use an ACPI namespace definition instead.
> +	 */
> +	if (!fwnode_property_read_string(acpi_fwnode_handle(root->device),
> +					 "linux-ecam-quirk-id", &soc)) {
> +		if (strlen(soc) != ACPI_OEM_ID_SIZE)
> +			dev_err(&root->device->dev, "ECAM quirk should be %d characters\n",
> +				ACPI_OEM_ID_SIZE);
> +		else
> +			memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE);
> +	}
> +
>  	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
>  		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
>  			if (f->cfgres.start)
> -- 
> 2.31.1
>
Bjorn Helgaas Oct. 5, 2021, 10:31 p.m. UTC | #6
On Tue, Oct 05, 2021 at 10:43:32AM -0500, Jeremy Linton wrote:
> On 10/5/21 10:10 AM, Bjorn Helgaas wrote:
> > On Thu, Aug 26, 2021 at 02:15:56AM -0500, Jeremy Linton wrote:
> > > Now that there is a bcm2711 quirk, it needs to be enabled when the
> > > MCFG is missing. Use an ACPI namespace _DSD property
> > > "linux-ecam-quirk-id" as an alternative to the MCFG OEM.
> > > 
> > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >   drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++
> > >   1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> > > index 53cab975f612..04c517418365 100644
> > > --- a/drivers/acpi/pci_mcfg.c
> > > +++ b/drivers/acpi/pci_mcfg.c
> > > @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = {
> > >   	ALTRA_ECAM_QUIRK(1, 13),
> > >   	ALTRA_ECAM_QUIRK(1, 14),
> > >   	ALTRA_ECAM_QUIRK(1, 15),
> > > +
> > > +	{ "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops,
> > > +	  DEFINE_RES_MEM(0xFD500000, 0xA000) },
> > >   };
> > >   static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> > > @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
> > >   	u16 segment = root->segment;
> > >   	struct resource *bus_range = &root->secondary;
> > >   	struct mcfg_fixup *f;
> > > +	const char *soc;
> > >   	int i;
> > > +	/*
> > > +	 * This may be a machine with a PCI/SMC conduit, which means it doesn't
> > > +	 * have an MCFG. Use an ACPI namespace definition instead.
> > > +	 */
> > > +	if (!fwnode_property_read_string(acpi_fwnode_handle(root->device),
> > > +					 "linux-ecam-quirk-id", &soc)) {
> > > +		if (strlen(soc) != ACPI_OEM_ID_SIZE)
> > 
> > From a reviewing perspective, it's not obvious why soc must be exactly
> > ACPI_OEM_ID_SIZE.  Does that imply space-padding in the DT string or
> > something?
> 
> This is at the moment an ACPI only DSD, and it must follow the MADT OEM_ID
> format for now because we are effectively just overriding that field. The
> rest of the code in this module is just treating it as a fixed 6 bytes.
> 
> > Is there any documentation for this DT property?
> 
> Its not a DT property, and its unclear since its linux only if it
> belongs in previously linked ACPI registry.

Oh, right, it comes from a _DSD.

> > Also not obvious why strlen() is safe here.  I mean, I looked a couple
> > levels deep in fwnode_property_read_string(), but whatever guarantees
> > null termination is buried pretty deep.
> 
> I've not tracked down who, if anyone other than the AML compiler is
> guaranteeing a null. The spec says something to the effect "Most other
> string, however,  are of variable-length and are automatically null
> terminated by the compiler". Not sure if that helps any.

Doesn't help for me.  The PCI core shouldn't go in the weeds no matter
what junk we might get from an AML compiler.  Maybe
fwnode_property_read_string() guarantees null termination, but it's
not documented and not easy to verify.

I think a strncpy() here might be better.  Not sure it's worthwhile to
emit a specific error message for the wrong length.

> > It seems a little weird to use an MCFG quirk mechanism when there's no
> > MCFG at all on this platform.
> 
> Well its just a point to hook in a CFG space quirk, and since that
> is what most of the MCFG quirks are, it seemed reasonable to reuse
> it rather than recreate it.

Yeah, it's ugly no matter how we slice it.  The pci_no_msi()
especially has nothing to do with ECAM at all.  But I don't know how
to identify this thing for a quirk.  PNP0A08 devices really rely on
ECAM or a system firmware config accessor.

> > > +			dev_err(&root->device->dev, "ECAM quirk should be %d characters\n",
> > > +				ACPI_OEM_ID_SIZE);
> > > +		else
> > > +			memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE);
> > > +	}
> > > +
> > >   	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
> > >   		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
> > >   			if (f->cfgres.start)
> > > -- 
> > > 2.31.1
> > > 
>
Jeremy Linton Oct. 5, 2021, 10:44 p.m. UTC | #7
Hi,

On 10/5/21 3:02 PM, Pali Rohár wrote:
> On Thursday 26 August 2021 02:15:56 Jeremy Linton wrote:
>> Now that there is a bcm2711 quirk, it needs to be enabled when the
>> MCFG is missing. Use an ACPI namespace _DSD property
>> "linux-ecam-quirk-id" as an alternative to the MCFG OEM.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>>   drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index 53cab975f612..04c517418365 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = {
>>   	ALTRA_ECAM_QUIRK(1, 13),
>>   	ALTRA_ECAM_QUIRK(1, 14),
>>   	ALTRA_ECAM_QUIRK(1, 15),
>> +
>> +	{ "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops,
>> +	  DEFINE_RES_MEM(0xFD500000, 0xA000) },
> 
> Hello!
> 
> According to discussion described in email [1], adding a new MCFG quirk
> (like above) into kernel requires adding some errata entry for
> documenting buggy HW.
> 
> But this patch series does not introduce any new errata entry.

Nor did the quirk following that email, which was merged. Presumably 
because I'm not sure anyone can predict the future, and some of the 
points in that posting were made simply to fill gaps in the arguments 
around why the SMC is/was a better plan. The final point about requiring 
a MCFG is also flies in the face of the fact that there is an Arm/Pci 
Standardized way to deal with these problems which is supported by other 
OS's running on these platforms and it requires that MFCG is missing. 
The idea that MCFG will magically appear for only Linux and contain 
relevant info (like the non even power of two cfg region on this SoC) is 
a bit wishful.

I'm certainly in no place to claim future PCIe compliance for Broadcom's 
SoC's, nor its lifecycle. I'm betting few people submitting Linux 
patches are, particularly for mostly community supported projects like 
the RPi4/CM4/ACPI port. I'm more than happy to hack/tweak future 
problems with this code, and possibly even extend it if needed. Thats 
about the extent of what I can do.


Thanks,




> 
> Bjorn, could you look at how to properly document these "hw bugs"?
> I guess there would be lot of more requests for adding MCFG quirks as
> now according to [1], doors are open for them. And it is possible that
> after more years nobody would be able to maintain these quirks if would
> not be properly documented.
> 
> [1] - https://lore.kernel.org/linux-pci/20210325131231.GA18590@e121166-lin.cambridge.arm.com/
> 
>>   };
>>   
>>   static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
>> @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
>>   	u16 segment = root->segment;
>>   	struct resource *bus_range = &root->secondary;
>>   	struct mcfg_fixup *f;
>> +	const char *soc;
>>   	int i;
>>   
>> +	/*
>> +	 * This may be a machine with a PCI/SMC conduit, which means it doesn't
>> +	 * have an MCFG. Use an ACPI namespace definition instead.
>> +	 */
>> +	if (!fwnode_property_read_string(acpi_fwnode_handle(root->device),
>> +					 "linux-ecam-quirk-id", &soc)) {
>> +		if (strlen(soc) != ACPI_OEM_ID_SIZE)
>> +			dev_err(&root->device->dev, "ECAM quirk should be %d characters\n",
>> +				ACPI_OEM_ID_SIZE);
>> +		else
>> +			memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE);
>> +	}
>> +
>>   	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
>>   		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
>>   			if (f->cfgres.start)
>> -- 
>> 2.31.1
>>
Jeremy Linton Oct. 5, 2021, 11:32 p.m. UTC | #8
Hi,

On 10/5/21 5:31 PM, Bjorn Helgaas wrote:
> On Tue, Oct 05, 2021 at 10:43:32AM -0500, Jeremy Linton wrote:
>> On 10/5/21 10:10 AM, Bjorn Helgaas wrote:
>>> On Thu, Aug 26, 2021 at 02:15:56AM -0500, Jeremy Linton wrote:
>>>> Now that there is a bcm2711 quirk, it needs to be enabled when the
>>>> MCFG is missing. Use an ACPI namespace _DSD property
>>>> "linux-ecam-quirk-id" as an alternative to the MCFG OEM.
>>>>
>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>>> ---
>>>>    drivers/acpi/pci_mcfg.c | 17 +++++++++++++++++
>>>>    1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>>>> index 53cab975f612..04c517418365 100644
>>>> --- a/drivers/acpi/pci_mcfg.c
>>>> +++ b/drivers/acpi/pci_mcfg.c
>>>> @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = {
>>>>    	ALTRA_ECAM_QUIRK(1, 13),
>>>>    	ALTRA_ECAM_QUIRK(1, 14),
>>>>    	ALTRA_ECAM_QUIRK(1, 15),
>>>> +
>>>> +	{ "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops,
>>>> +	  DEFINE_RES_MEM(0xFD500000, 0xA000) },
>>>>    };
>>>>    static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
>>>> @@ -198,8 +201,22 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
>>>>    	u16 segment = root->segment;
>>>>    	struct resource *bus_range = &root->secondary;
>>>>    	struct mcfg_fixup *f;
>>>> +	const char *soc;
>>>>    	int i;
>>>> +	/*
>>>> +	 * This may be a machine with a PCI/SMC conduit, which means it doesn't
>>>> +	 * have an MCFG. Use an ACPI namespace definition instead.
>>>> +	 */
>>>> +	if (!fwnode_property_read_string(acpi_fwnode_handle(root->device),
>>>> +					 "linux-ecam-quirk-id", &soc)) {
>>>> +		if (strlen(soc) != ACPI_OEM_ID_SIZE)
>>>
>>>  From a reviewing perspective, it's not obvious why soc must be exactly
>>> ACPI_OEM_ID_SIZE.  Does that imply space-padding in the DT string or
>>> something?
>>
>> This is at the moment an ACPI only DSD, and it must follow the MADT OEM_ID
>> format for now because we are effectively just overriding that field. The
>> rest of the code in this module is just treating it as a fixed 6 bytes.
>>
>>> Is there any documentation for this DT property?
>>
>> Its not a DT property, and its unclear since its linux only if it
>> belongs in previously linked ACPI registry.
> 
> Oh, right, it comes from a _DSD.
> 
>>> Also not obvious why strlen() is safe here.  I mean, I looked a couple
>>> levels deep in fwnode_property_read_string(), but whatever guarantees
>>> null termination is buried pretty deep.
>>
>> I've not tracked down who, if anyone other than the AML compiler is
>> guaranteeing a null. The spec says something to the effect "Most other
>> string, however,  are of variable-length and are automatically null
>> terminated by the compiler". Not sure if that helps any.
> 
> Doesn't help for me.  The PCI core shouldn't go in the weeds no matter
> what junk we might get from an AML compiler.  Maybe
> fwnode_property_read_string() guarantees null termination, but it's
> not documented and not easy to verify.
> 
> I think a strncpy() here might be better.  Not sure it's worthwhile to
> emit a specific error message for the wrong length.

I think we went around about this a bit, but yes strncpy() is exactly 
what we want because the rest of the code assumes 6 non-null terminated 
characters and strncpy won't terminate it if its longer. OTOH, i'm not 
sure we really want shorter strings padded with nulls either.

strncpy() is easy though, so sure. <shrug>


> 
>>> It seems a little weird to use an MCFG quirk mechanism when there's no
>>> MCFG at all on this platform.
>>
>> Well its just a point to hook in a CFG space quirk, and since that
>> is what most of the MCFG quirks are, it seemed reasonable to reuse
>> it rather than recreate it.
> 
> Yeah, it's ugly no matter how we slice it.  The pci_no_msi()
> especially has nothing to do with ECAM at all.  But I don't know how
> to identify this thing for a quirk.  PNP0A08 devices really rely on
> ECAM or a system firmware config accessor.
> 
>>>> +			dev_err(&root->device->dev, "ECAM quirk should be %d characters\n",
>>>> +				ACPI_OEM_ID_SIZE);
>>>> +		else
>>>> +			memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE);
>>>> +	}
>>>> +
>>>>    	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
>>>>    		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
>>>>    			if (f->cfgres.start)
>>>> -- 
>>>> 2.31.1
>>>>
>>
diff mbox series

Patch

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index 53cab975f612..04c517418365 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -169,6 +169,9 @@  static struct mcfg_fixup mcfg_quirks[] = {
 	ALTRA_ECAM_QUIRK(1, 13),
 	ALTRA_ECAM_QUIRK(1, 14),
 	ALTRA_ECAM_QUIRK(1, 15),
+
+	{ "bc2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops,
+	  DEFINE_RES_MEM(0xFD500000, 0xA000) },
 };
 
 static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
@@ -198,8 +201,22 @@  static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
 	u16 segment = root->segment;
 	struct resource *bus_range = &root->secondary;
 	struct mcfg_fixup *f;
+	const char *soc;
 	int i;
 
+	/*
+	 * This may be a machine with a PCI/SMC conduit, which means it doesn't
+	 * have an MCFG. Use an ACPI namespace definition instead.
+	 */
+	if (!fwnode_property_read_string(acpi_fwnode_handle(root->device),
+					 "linux-ecam-quirk-id", &soc)) {
+		if (strlen(soc) != ACPI_OEM_ID_SIZE)
+			dev_err(&root->device->dev, "ECAM quirk should be %d characters\n",
+				ACPI_OEM_ID_SIZE);
+		else
+			memcpy(mcfg_oem_id, soc, ACPI_OEM_ID_SIZE);
+	}
+
 	for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) {
 		if (pci_mcfg_quirk_matches(f, segment, bus_range)) {
 			if (f->cfgres.start)