diff mbox series

[v6,7/7] xen/arm: do not use void pointer in pci_host_common_probe

Message ID 20211105063326.939843-8-andr2000@gmail.com (mailing list archive)
State Superseded
Headers show
Series PCI devices passthrough on Arm, part 2 | expand

Commit Message

Oleksandr Andrushchenko Nov. 5, 2021, 6:33 a.m. UTC 
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

There is no reason to use void pointer while passing ECAM ops to the
pci_host_common_probe function as it is anyway casted to struct pci_ecam_ops
inside. For that reason remove the void pointer and pass struct pci_ecam_ops
pointer as is.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

---
New in v4
---
 xen/arch/arm/pci/ecam.c            | 4 ++--
 xen/arch/arm/pci/pci-host-common.c | 6 ++----
 xen/include/asm-arm/pci.h          | 5 +++--
 3 files changed, 7 insertions(+), 8 deletions(-)

Comments

Rahul Singh Nov. 17, 2021, 11:12 a.m. UTC | #1 
Hi Oleksandr,

> On 5 Nov 2021, at 6:33 am, Oleksandr Andrushchenko <andr2000@gmail.com> wrote:
> 
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> There is no reason to use void pointer while passing ECAM ops to the
> pci_host_common_probe function as it is anyway casted to struct pci_ecam_ops
> inside. For that reason remove the void pointer and pass struct pci_ecam_ops
> pointer as is.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul
> 
> ---
> New in v4
> ---
> xen/arch/arm/pci/ecam.c            | 4 ++--
> xen/arch/arm/pci/pci-host-common.c | 6 ++----
> xen/include/asm-arm/pci.h          | 5 +++--
> 3 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
> index 4f71b11c3057..6aeea12a68bf 100644
> --- a/xen/arch/arm/pci/ecam.c
> +++ b/xen/arch/arm/pci/ecam.c
> @@ -24,8 +24,8 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>                                pci_sbdf_t sbdf, uint32_t where)
> {
>     const struct pci_config_window *cfg = bridge->cfg;
> -    struct pci_ecam_ops *ops =
> -        container_of(bridge->ops, struct pci_ecam_ops, pci_ops);
> +    const struct pci_ecam_ops *ops =
> +        container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
>     unsigned int devfn_shift = ops->bus_shift - 8;
>     void __iomem *base;
> 
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 6af845ab9d6c..1aad664b213e 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -194,15 +194,13 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>     return domain;
> }
> 
> -int pci_host_common_probe(struct dt_device_node *dev, const void *data)
> +int pci_host_common_probe(struct dt_device_node *dev,
> +                          const struct pci_ecam_ops *ops)
> {
>     struct pci_host_bridge *bridge;
>     struct pci_config_window *cfg;
> -    struct pci_ecam_ops *ops;
>     int err;
> 
> -    ops = (struct pci_ecam_ops *) data;
> -
>     bridge = pci_alloc_host_bridge();
>     if ( !bridge )
>         return -ENOMEM;
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 3d706fdd1d88..4199e0267d24 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -65,7 +65,7 @@ struct pci_host_bridge {
>     struct list_head node;           /* Node in list of host bridges */
>     uint16_t segment;                /* Segment number */
>     struct pci_config_window* cfg;   /* Pointer to the bridge config window */
> -    struct pci_ops *ops;
> +    const struct pci_ops *ops;
> };
> 
> struct pci_ops {
> @@ -94,7 +94,8 @@ struct pci_ecam_ops {
> /* Default ECAM ops */
> extern const struct pci_ecam_ops pci_generic_ecam_ops;
> 
> -int pci_host_common_probe(struct dt_device_node *dev, const void *data);
> +int pci_host_common_probe(struct dt_device_node *dev,
> +                          const struct pci_ecam_ops *ops);
> int pci_generic_config_read(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
>                             uint32_t reg, uint32_t len, uint32_t *value);
> int pci_generic_config_write(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
> -- 
> 2.25.1
> 
>
Julien Grall Nov. 17, 2021, 9:45 p.m. UTC | #2 
Hi Oleksandr,

On 05/11/2021 06:33, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> There is no reason to use void pointer while passing ECAM ops to the
> pci_host_common_probe function as it is anyway casted to struct pci_ecam_ops
> inside. For that reason remove the void pointer and pass struct pci_ecam_ops
> pointer as is.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

I was going to ack and push the patch. But then I couldn't apply the 
patch...

> 
> ---
> New in v4
> ---
>   xen/arch/arm/pci/ecam.c            | 4 ++--
>   xen/arch/arm/pci/pci-host-common.c | 6 ++----
>   xen/include/asm-arm/pci.h          | 5 +++--
>   3 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
> index 4f71b11c3057..6aeea12a68bf 100644
> --- a/xen/arch/arm/pci/ecam.c
> +++ b/xen/arch/arm/pci/ecam.c
> @@ -24,8 +24,8 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>                                  pci_sbdf_t sbdf, uint32_t where)
>   {
>       const struct pci_config_window *cfg = bridge->cfg;
> -    struct pci_ecam_ops *ops =
> -        container_of(bridge->ops, struct pci_ecam_ops, pci_ops);
> +    const struct pci_ecam_ops *ops =
> +        container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
>       unsigned int devfn_shift = ops->bus_shift - 8;
>       void __iomem *base;
>   
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 6af845ab9d6c..1aad664b213e 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -194,15 +194,13 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>       return domain;
>   }
>   
> -int pci_host_common_probe(struct dt_device_node *dev, const void *data)
> +int pci_host_common_probe(struct dt_device_node *dev,
> +                          const struct pci_ecam_ops *ops)
>   {
>       struct pci_host_bridge *bridge;
>       struct pci_config_window *cfg;
> -    struct pci_ecam_ops *ops;
>       int err;

... in staging, the code has an two additional lines here:

     if ( dt_device_for_passthrough(dev) )
         return 0;

Is this series relying on patch that are not yet upstreamed?

Cheers,
Oleksandr Andrushchenko Nov. 18, 2021, 7:34 a.m. UTC | #3 
Hi, Julien!

On 17.11.21 23:45, Julien Grall wrote:
> Hi Oleksandr,
>
> On 05/11/2021 06:33, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> There is no reason to use void pointer while passing ECAM ops to the
>> pci_host_common_probe function as it is anyway casted to struct pci_ecam_ops
>> inside. For that reason remove the void pointer and pass struct pci_ecam_ops
>> pointer as is.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> I was going to ack and push the patch. But then I couldn't apply the patch...
>
>>
>> ---
>> New in v4
>> ---
>>   xen/arch/arm/pci/ecam.c            | 4 ++--
>>   xen/arch/arm/pci/pci-host-common.c | 6 ++----
>>   xen/include/asm-arm/pci.h          | 5 +++--
>>   3 files changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
>> index 4f71b11c3057..6aeea12a68bf 100644
>> --- a/xen/arch/arm/pci/ecam.c
>> +++ b/xen/arch/arm/pci/ecam.c
>> @@ -24,8 +24,8 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>>                                  pci_sbdf_t sbdf, uint32_t where)
>>   {
>>       const struct pci_config_window *cfg = bridge->cfg;
>> -    struct pci_ecam_ops *ops =
>> -        container_of(bridge->ops, struct pci_ecam_ops, pci_ops);
>> +    const struct pci_ecam_ops *ops =
>> +        container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
>>       unsigned int devfn_shift = ops->bus_shift - 8;
>>       void __iomem *base;
>>   diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>> index 6af845ab9d6c..1aad664b213e 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -194,15 +194,13 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>>       return domain;
>>   }
>>   -int pci_host_common_probe(struct dt_device_node *dev, const void *data)
>> +int pci_host_common_probe(struct dt_device_node *dev,
>> +                          const struct pci_ecam_ops *ops)
>>   {
>>       struct pci_host_bridge *bridge;
>>       struct pci_config_window *cfg;
>> -    struct pci_ecam_ops *ops;
>>       int err;
>
> ... in staging, the code has an two additional lines here:
>
>     if ( dt_device_for_passthrough(dev) )
>         return 0;
>
> Is this series relying on patch that are not yet upstreamed?
Yes, I mistakenly had a patch below that I didn't want to upstream with
this series, so this is why. Sorry about that.
Frankly, I didn't expect patches to be merged from this series now as
1) I expect v7
2) I thought we won't push until the release

That being said: do you mind if I put your Acked-by in this patch, so
it is ready for v7?
>
> Cheers,
>
Thank you,
Oleksandr
Julien Grall Nov. 22, 2021, 5:48 p.m. UTC | #4 
On 18/11/2021 07:34, Oleksandr Andrushchenko wrote:
> Hi, Julien!

Hi,

> On 17.11.21 23:45, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> On 05/11/2021 06:33, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> There is no reason to use void pointer while passing ECAM ops to the
>>> pci_host_common_probe function as it is anyway casted to struct pci_ecam_ops
>>> inside. For that reason remove the void pointer and pass struct pci_ecam_ops
>>> pointer as is.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> I was going to ack and push the patch. But then I couldn't apply the patch...
>>
>>>
>>> ---
>>> New in v4
>>> ---
>>>    xen/arch/arm/pci/ecam.c            | 4 ++--
>>>    xen/arch/arm/pci/pci-host-common.c | 6 ++----
>>>    xen/include/asm-arm/pci.h          | 5 +++--
>>>    3 files changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
>>> index 4f71b11c3057..6aeea12a68bf 100644
>>> --- a/xen/arch/arm/pci/ecam.c
>>> +++ b/xen/arch/arm/pci/ecam.c
>>> @@ -24,8 +24,8 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>>>                                   pci_sbdf_t sbdf, uint32_t where)
>>>    {
>>>        const struct pci_config_window *cfg = bridge->cfg;
>>> -    struct pci_ecam_ops *ops =
>>> -        container_of(bridge->ops, struct pci_ecam_ops, pci_ops);
>>> +    const struct pci_ecam_ops *ops =
>>> +        container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
>>>        unsigned int devfn_shift = ops->bus_shift - 8;
>>>        void __iomem *base;
>>>    diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>>> index 6af845ab9d6c..1aad664b213e 100644
>>> --- a/xen/arch/arm/pci/pci-host-common.c
>>> +++ b/xen/arch/arm/pci/pci-host-common.c
>>> @@ -194,15 +194,13 @@ static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>>>        return domain;
>>>    }
>>>    -int pci_host_common_probe(struct dt_device_node *dev, const void *data)
>>> +int pci_host_common_probe(struct dt_device_node *dev,
>>> +                          const struct pci_ecam_ops *ops)
>>>    {
>>>        struct pci_host_bridge *bridge;
>>>        struct pci_config_window *cfg;
>>> -    struct pci_ecam_ops *ops;
>>>        int err;
>>
>> ... in staging, the code has an two additional lines here:
>>
>>      if ( dt_device_for_passthrough(dev) )
>>          return 0;
>>
>> Is this series relying on patch that are not yet upstreamed?
> Yes, I mistakenly had a patch below that I didn't want to upstream with
> this series, so this is why. Sorry about that.
> Frankly, I didn't expect patches to be merged from this series now as
> 1) I expect v7

We tend to merge patches in a different order, if there are no 
dependencies and would make sense without the rest of the series. This 
help reducing the size of the series.

> 2) I thought we won't push until the release

For Arm, Stefano and I have been created for-next/XX.YY (for this 
release the branch is for-next/4.17) to queue patches until the tree is 
re-opened for several releases.

> 
> That being said: do you mind if I put your Acked-by in this patch, so
> it is ready for v7?

Sure. So long this is a simple rebase:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

[1] 
https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/for-next/4.17
diff mbox series

Patch

diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
index 4f71b11c3057..6aeea12a68bf 100644
--- a/xen/arch/arm/pci/ecam.c
+++ b/xen/arch/arm/pci/ecam.c
@@ -24,8 +24,8 @@  void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
                                pci_sbdf_t sbdf, uint32_t where)
 {
     const struct pci_config_window *cfg = bridge->cfg;
-    struct pci_ecam_ops *ops =
-        container_of(bridge->ops, struct pci_ecam_ops, pci_ops);
+    const struct pci_ecam_ops *ops =
+        container_of(bridge->ops, const struct pci_ecam_ops, pci_ops);
     unsigned int devfn_shift = ops->bus_shift - 8;
     void __iomem *base;
 
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 6af845ab9d6c..1aad664b213e 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -194,15 +194,13 @@  static int pci_bus_find_domain_nr(struct dt_device_node *dev)
     return domain;
 }
 
-int pci_host_common_probe(struct dt_device_node *dev, const void *data)
+int pci_host_common_probe(struct dt_device_node *dev,
+                          const struct pci_ecam_ops *ops)
 {
     struct pci_host_bridge *bridge;
     struct pci_config_window *cfg;
-    struct pci_ecam_ops *ops;
     int err;
 
-    ops = (struct pci_ecam_ops *) data;
-
     bridge = pci_alloc_host_bridge();
     if ( !bridge )
         return -ENOMEM;
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 3d706fdd1d88..4199e0267d24 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -65,7 +65,7 @@  struct pci_host_bridge {
     struct list_head node;           /* Node in list of host bridges */
     uint16_t segment;                /* Segment number */
     struct pci_config_window* cfg;   /* Pointer to the bridge config window */
-    struct pci_ops *ops;
+    const struct pci_ops *ops;
 };
 
 struct pci_ops {
@@ -94,7 +94,8 @@  struct pci_ecam_ops {
 /* Default ECAM ops */
 extern const struct pci_ecam_ops pci_generic_ecam_ops;
 
-int pci_host_common_probe(struct dt_device_node *dev, const void *data);
+int pci_host_common_probe(struct dt_device_node *dev,
+                          const struct pci_ecam_ops *ops);
 int pci_generic_config_read(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,
                             uint32_t reg, uint32_t len, uint32_t *value);
 int pci_generic_config_write(struct pci_host_bridge *bridge, pci_sbdf_t sbdf,