Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1312357 > unrolled thread

Re: [PATCH V3 11/21] pci, acpi: Move ACPI host bridge device companion assignment to core code.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V3 11/21] pci, acpi: Move ACPI host bridge device  companion assignment to core code. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-01-19 17:10 +0100
    Re: [PATCH V3 11/21] pci, acpi: Move ACPI host bridge device  companion assignment to core code. Tomasz Nowicki <tn@semihalf.com> - 2016-01-20 12:30 +0100
      Re: [PATCH V3 11/21] pci, acpi: Move ACPI host bridge device  companion assignment to core code. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-01-20 13:40 +0100
        Re: [PATCH V3 11/21] pci, acpi: Move ACPI host bridge device  companion assignment to core code. Tomasz Nowicki <tn@semihalf.com> - 2016-01-20 14:50 +0100
          Re: [PATCH V3 11/21] pci, acpi: Move ACPI host bridge device  companion assignment to core code. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-01-20 15:30 +0100
            Re: [PATCH V3 11/21] pci, acpi: Move ACPI host bridge device  companion assignment to core code. Tomasz Nowicki <tn@semihalf.com> - 2016-01-20 15:50 +0100
              Re: [PATCH V3 11/21] pci, acpi: Move ACPI host bridge device  companion assignment to core code. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-01-27 18:50 +0100

#1312357 — Re: [PATCH V3 11/21] pci, acpi: Move ACPI host bridge device companion assignment to core code.

On Wed, Jan 13, 2016 at 02:20:57PM +0100, Tomasz Nowicki wrote:
> Currently we have two platforms (x86 & ia64) capable of PCI ACPI host
> bridge initialization. They both use sysdata pill to pass down parent

pill ?

> device reference and both relay on NULL parent in pci_create_root_bus()

s/relay/rely

> to validate sysdata content.
> 
> It looks hacky and prevent us from getting some frimware specific

"prevents us from getting some firmware..."

> info for PCI host controller e.g. bus domain number.
> However, it seems we can overcome that blocker by passing down parent

It does not seem, either this patch overcomes the issue and it is
accepted, or it does not and it is discarded.

> device via pci_create_root_bus parameter (as the ACPI device type)

I do not understand what you mean by "as the ACPI device type".

> and using ACPI_COMPANION_SET in core code for ACPI boot method.
> ACPI_COMPANION_SET is safe to run for all cases DT, ACPI and DT&ACPI.
> 
> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/acpi/pci_root.c | 5 ++++-
>  drivers/pci/probe.c     | 2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index ae3fe4e..a65c8c2 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -846,7 +846,10 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  
>  	pci_acpi_root_add_resources(info);
>  	pci_add_resource(&info->resources, &root->secondary);
> -	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> +
> +	/* Root bridge device needs to be sure of parent ACPI type */
> +	ACPI_COMPANION_SET(&device->dev, device);

I do not understand why the code above is needed, can you elaborate
please ?

Thanks,
Lorenzo

> +	bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
>  				  sysdata, &info->resources);
>  	if (!bus)
>  		goto out_release_info;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 553a029..cad836f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2107,6 +2107,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	bridge->dev.parent = parent;
>  	bridge->dev.release = pci_release_host_bridge_dev;
>  	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> +	ACPI_COMPANION_SET(&bridge->dev,
> +			   parent ? to_acpi_device_node(parent->fwnode) : NULL);
>  	error = pcibios_root_bridge_prepare(bridge);
>  	if (error) {
>  		kfree(bridge);
> -- 
> 1.9.1
> 

[toc] | [next] | [standalone]

#1313141

On 19.01.2016 17:02, Lorenzo Pieralisi wrote:
> On Wed, Jan 13, 2016 at 02:20:57PM +0100, Tomasz Nowicki wrote:
[...]
>> device via pci_create_root_bus parameter (as the ACPI device type)
>
> I do not understand what you mean by "as the ACPI device type".

See below.

>
>> and using ACPI_COMPANION_SET in core code for ACPI boot method.
>> ACPI_COMPANION_SET is safe to run for all cases DT, ACPI and DT&ACPI.
>>
>> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> ---
>>   drivers/acpi/pci_root.c | 5 ++++-
>>   drivers/pci/probe.c     | 2 ++
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index ae3fe4e..a65c8c2 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -846,7 +846,10 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>
>>   	pci_acpi_root_add_resources(info);
>>   	pci_add_resource(&info->resources, &root->secondary);
>> -	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
>> +
>> +	/* Root bridge device needs to be sure of parent ACPI type */
>> +	ACPI_COMPANION_SET(&device->dev, device);
>
> I do not understand why the code above is needed, can you elaborate
> please ?
>

This makes sure that device->dev can be identified as ACPI device, so we 
can use to_acpi_device_node() and assign companion safely below.

>
>> +	bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
>>   				  sysdata, &info->resources);
>>   	if (!bus)
>>   		goto out_release_info;
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 553a029..cad836f 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2107,6 +2107,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>>   	bridge->dev.parent = parent;
>>   	bridge->dev.release = pci_release_host_bridge_dev;
>>   	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
>> +	ACPI_COMPANION_SET(&bridge->dev,
>> +			   parent ? to_acpi_device_node(parent->fwnode) : NULL);

pci_create_root_bus() can be called with DT device, then 
ACPI_COMPANION_SET assign no companion to bridge device.

Thanks,
Tomasz

[toc] | [prev] | [next] | [standalone]

#1313192

On Wed, Jan 20, 2016 at 12:20:07PM +0100, Tomasz Nowicki wrote:
> On 19.01.2016 17:02, Lorenzo Pieralisi wrote:
> >On Wed, Jan 13, 2016 at 02:20:57PM +0100, Tomasz Nowicki wrote:
> [...]
> >>device via pci_create_root_bus parameter (as the ACPI device type)
> >
> >I do not understand what you mean by "as the ACPI device type".
> 
> See below.
> 
> >
> >>and using ACPI_COMPANION_SET in core code for ACPI boot method.
> >>ACPI_COMPANION_SET is safe to run for all cases DT, ACPI and DT&ACPI.
> >>
> >>Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> >>---
> >>  drivers/acpi/pci_root.c | 5 ++++-
> >>  drivers/pci/probe.c     | 2 ++
> >>  2 files changed, 6 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> >>index ae3fe4e..a65c8c2 100644
> >>--- a/drivers/acpi/pci_root.c
> >>+++ b/drivers/acpi/pci_root.c
> >>@@ -846,7 +846,10 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >>
> >>  	pci_acpi_root_add_resources(info);
> >>  	pci_add_resource(&info->resources, &root->secondary);
> >>-	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> >>+
> >>+	/* Root bridge device needs to be sure of parent ACPI type */
> >>+	ACPI_COMPANION_SET(&device->dev, device);
> >
> >I do not understand why the code above is needed, can you elaborate
> >please ?
> >
> 
> This makes sure that device->dev can be identified as ACPI device,
> so we can use to_acpi_device_node() and assign companion safely
> below.

I do not follow. If you refer to the fwnode handle type, that is
already set by ACPI core code (before acpi_pci_root_add() is called,
in acpi_init_device_object()).

Did you add the code above to solve a real issue you encountered ?

Thanks,
Lorenzo

[toc] | [prev] | [next] | [standalone]

#1313230

On 20.01.2016 13:38, Lorenzo Pieralisi wrote:
> On Wed, Jan 20, 2016 at 12:20:07PM +0100, Tomasz Nowicki wrote:
>> On 19.01.2016 17:02, Lorenzo Pieralisi wrote:
>>> On Wed, Jan 13, 2016 at 02:20:57PM +0100, Tomasz Nowicki wrote:
>> [...]
>>>> device via pci_create_root_bus parameter (as the ACPI device type)
>>>
>>> I do not understand what you mean by "as the ACPI device type".
>>
>> See below.
>>
>>>
>>>> and using ACPI_COMPANION_SET in core code for ACPI boot method.
>>>> ACPI_COMPANION_SET is safe to run for all cases DT, ACPI and DT&ACPI.
>>>>
>>>> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>>>> ---
>>>>   drivers/acpi/pci_root.c | 5 ++++-
>>>>   drivers/pci/probe.c     | 2 ++
>>>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>>>> index ae3fe4e..a65c8c2 100644
>>>> --- a/drivers/acpi/pci_root.c
>>>> +++ b/drivers/acpi/pci_root.c
>>>> @@ -846,7 +846,10 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>>>
>>>>   	pci_acpi_root_add_resources(info);
>>>>   	pci_add_resource(&info->resources, &root->secondary);
>>>> -	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
>>>> +
>>>> +	/* Root bridge device needs to be sure of parent ACPI type */
>>>> +	ACPI_COMPANION_SET(&device->dev, device);
>>>
>>> I do not understand why the code above is needed, can you elaborate
>>> please ?
>>>
>>
>> This makes sure that device->dev can be identified as ACPI device,
>> so we can use to_acpi_device_node() and assign companion safely
>> below.
>
> I do not follow. If you refer to the fwnode handle type, that is
> already set by ACPI core code (before acpi_pci_root_add() is called,
> in acpi_init_device_object()).

acpi_init_device_object() sets ACPI fwnode handle type only for 
"device", but not for "device->dev" which is what is passed as an 
argument to pci_create_root_bus().

Without ACPI_COMPANION_SET(&device->dev, device) here, no one can be 
sure if we have ACPI device in pci_create_root_bus().

>
> Did you add the code above to solve a real issue you encountered ?
Yes. You can try to run series with this patch:

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 88c9d20..1b74bf6 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -967,7 +967,11 @@ struct pci_bus *acpi_pci_root_create(struct 
acpi_pci_root *root,
         pci_add_resource(&info->resources, &root->secondary);

-        /* Root bridge device needs to be sure of parent ACPI type */
-       ACPI_COMPANION_SET(&device->dev, device);
+       if (to_acpi_device_node(device->dev.fwnode) == NULL)
+               pr_err("NON-ACPI TYPE\n");
+       else
+               pr_err("ACPI TYPE\n");
+
         bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
                                   sysdata, &info->resources);


Thanks,
Tomasz

[toc] | [prev] | [next] | [standalone]

#1313248

On Wed, Jan 20, 2016 at 02:40:08PM +0100, Tomasz Nowicki wrote:

[...]

> >>>>+	/* Root bridge device needs to be sure of parent ACPI type */
> >>>>+	ACPI_COMPANION_SET(&device->dev, device);
> >>>
> >>>I do not understand why the code above is needed, can you elaborate
> >>>please ?
> >>>
> >>
> >>This makes sure that device->dev can be identified as ACPI device,
> >>so we can use to_acpi_device_node() and assign companion safely
> >>below.
> >
> >I do not follow. If you refer to the fwnode handle type, that is
> >already set by ACPI core code (before acpi_pci_root_add() is called,
> >in acpi_init_device_object()).
> 
> acpi_init_device_object() sets ACPI fwnode handle type only for
> "device", but not for "device->dev" which is what is passed as an
> argument to pci_create_root_bus().
> 
> Without ACPI_COMPANION_SET(&device->dev, device) here, no one can be
> sure if we have ACPI device in pci_create_root_bus().

Ok, got it. The question is whether this should be done in ACPI
core instead (is there a reason why it should *not* be done ?), but I
now understand your point.

Thanks,
Lorenzo

[toc] | [prev] | [next] | [standalone]

#1313263

On 20.01.2016 15:22, Lorenzo Pieralisi wrote:
> On Wed, Jan 20, 2016 at 02:40:08PM +0100, Tomasz Nowicki wrote:
>
> [...]
>
>>>>>> +	/* Root bridge device needs to be sure of parent ACPI type */
>>>>>> +	ACPI_COMPANION_SET(&device->dev, device);
>>>>>
>>>>> I do not understand why the code above is needed, can you elaborate
>>>>> please ?
>>>>>
>>>>
>>>> This makes sure that device->dev can be identified as ACPI device,
>>>> so we can use to_acpi_device_node() and assign companion safely
>>>> below.
>>>
>>> I do not follow. If you refer to the fwnode handle type, that is
>>> already set by ACPI core code (before acpi_pci_root_add() is called,
>>> in acpi_init_device_object()).
>>
>> acpi_init_device_object() sets ACPI fwnode handle type only for
>> "device", but not for "device->dev" which is what is passed as an
>> argument to pci_create_root_bus().
>>
>> Without ACPI_COMPANION_SET(&device->dev, device) here, no one can be
>> sure if we have ACPI device in pci_create_root_bus().
>
> Ok, got it. The question is whether this should be done in ACPI
> core instead (is there a reason why it should *not* be done ?), but I
> now understand your point.
>

I am not able to answer that question, but I see lots of changes in this 
area made by Rafael.

Rafael, can you please shed some light on this?

Thanks,
Tomasz

[toc] | [prev] | [next] | [standalone]

#1319186

On Wed, Jan 20, 2016 at 03:41:56PM +0100, Tomasz Nowicki wrote:
> On 20.01.2016 15:22, Lorenzo Pieralisi wrote:
> >On Wed, Jan 20, 2016 at 02:40:08PM +0100, Tomasz Nowicki wrote:
> >
> >[...]
> >
> >>>>>>+	/* Root bridge device needs to be sure of parent ACPI type */
> >>>>>>+	ACPI_COMPANION_SET(&device->dev, device);
> >>>>>
> >>>>>I do not understand why the code above is needed, can you elaborate
> >>>>>please ?
> >>>>>
> >>>>
> >>>>This makes sure that device->dev can be identified as ACPI device,
> >>>>so we can use to_acpi_device_node() and assign companion safely
> >>>>below.
> >>>
> >>>I do not follow. If you refer to the fwnode handle type, that is
> >>>already set by ACPI core code (before acpi_pci_root_add() is called,
> >>>in acpi_init_device_object()).
> >>
> >>acpi_init_device_object() sets ACPI fwnode handle type only for
> >>"device", but not for "device->dev" which is what is passed as an
> >>argument to pci_create_root_bus().
> >>
> >>Without ACPI_COMPANION_SET(&device->dev, device) here, no one can be
> >>sure if we have ACPI device in pci_create_root_bus().
> >
> >Ok, got it. The question is whether this should be done in ACPI
> >core instead (is there a reason why it should *not* be done ?), but I
> >now understand your point.
> >
> 
> I am not able to answer that question, but I see lots of changes in
> this area made by Rafael.

I had a further look and I am not sure why the fwnode_handle* in the
dev member of the acpi_device is not made to point at the fwnode member
of the acpi_device by *default* (acpi_init_device_object() ?), probably
because it is never used as such but certainly Rafael knows, so I would
ask him to clarify please it is unclear to me.

I do not see anything wrong in what you are doing in this patch.

Rafael, any comments ?

Thanks,
Lorenzo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web