[PATCH] OF: PCI: const usage needed by MIPS

[PATCH] OF: PCI: const usage needed by MIPS

[John Crispin]

On MIPS we want to call of_irq_map_pci from inside

arch/mips/include/asm/pci.h:extern int pcibios_map_irq(
				const struct pci_dev *dev, u8 slot, u8 pin);

For this to work we need to change several functions to const usage.

Signed-off-by: John Crispin <blogic@openwrt.org>
Cc: linux-pci@vger.kernel.org
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-mips@linux-mips.org
---
I am not sure which tree this should go via. Grant, can you take it ?

 drivers/of/of_pci_irq.c |    2 +-
 drivers/pci/pci.c       |    2 +-
 include/linux/of_pci.h  |    2 +-
 include/linux/pci.h     |    5 +++--
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
index 9312516..6770538 100644
--- a/drivers/of/of_pci_irq.c
+++ b/drivers/of/of_pci_irq.c
@@ -15,7 +15,7 @@
  * PCI tree until an device-node is found, at which point it will finish
  * resolving using the OF tree walking.
  */
-int of_irq_map_pci(struct pci_dev *pdev, struct of_irq *out_irq)
+int of_irq_map_pci(const struct pci_dev *pdev, struct of_irq *out_irq)
 {
 	struct device_node *dn, *ppnode;
 	struct pci_dev *ppdev;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d20f133..4c79586 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2363,7 +2363,7 @@ void pci_enable_acs(struct pci_dev *dev)
  * number is always 0 (see the Implementation Note in section 2.2.8.1 of
  * the PCI Express Base Specification, Revision 2.1)
  */
-u8 pci_swizzle_interrupt_pin(struct pci_dev *dev, u8 pin)
+u8 pci_swizzle_interrupt_pin(const struct pci_dev *dev, u8 pin)
 {
 	int slot;
 
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index f93e217..bb115de 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -5,7 +5,7 @@
 
 struct pci_dev;
 struct of_irq;
-int of_irq_map_pci(struct pci_dev *pdev, struct of_irq *out_irq);
+int of_irq_map_pci(const struct pci_dev *pdev, struct of_irq *out_irq);
 
 struct device_node;
 struct device_node *of_pci_find_child_device(struct device_node *parent,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e444f5b..3bbc77e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -680,7 +680,7 @@ int __must_check pci_bus_add_device(struct pci_dev *dev);
 void pci_read_bridge_bases(struct pci_bus *child);
 struct resource *pci_find_parent_resource(const struct pci_dev *dev,
 					  struct resource *res);
-u8 pci_swizzle_interrupt_pin(struct pci_dev *dev, u8 pin);
+u8 pci_swizzle_interrupt_pin(const struct pci_dev *dev, u8 pin);
 int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
 u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
 extern struct pci_dev *pci_dev_get(struct pci_dev *dev);
@@ -1685,7 +1685,8 @@ extern void pci_release_bus_of_node(struct pci_bus *bus);
 /* Arch may override this (weak) */
 extern struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus);
 
-static inline struct device_node *pci_device_to_OF_node(struct pci_dev *pdev)
+static inline struct device_node *
+pci_device_to_OF_node(const struct pci_dev *pdev)
 {
 	return pdev ? pdev->dev.of_node : NULL;
 }
-- 
1.7.9.1

Re: [PATCH] OF: PCI: const usage needed by MIPS

[David Daney]

On 04/30/2012 10:46 AM, John Crispin wrote:
> On MIPS we want to call of_irq_map_pci from inside
>
> arch/mips/include/asm/pci.h:extern int pcibios_map_irq(
> 				const struct pci_dev *dev, u8 slot, u8 pin);
>
> For this to work we need to change several functions to const usage.

I think there is a mismatch on this throughout the kernel.

Properly fixing it requires touching many more places than these. 
Although I haven't tried it, I wouldn't be surprised if doing this 
caused warnings to appear in non-MIPS code.

Ralf had a patch at one point that tried to make this consistent 
tree-wide, but it is not yet applied.

David Daney

>
> Signed-off-by: John Crispin<blogic@openwrt.org>
> Cc: linux-pci@vger.kernel.org
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: linux-mips@linux-mips.org
> ---
> I am not sure which tree this should go via. Grant, can you take it ?
>
>   drivers/of/of_pci_irq.c |    2 +-
>   drivers/pci/pci.c       |    2 +-
>   include/linux/of_pci.h  |    2 +-
>   include/linux/pci.h     |    5 +++--
>   4 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/of/of_pci_irq.c b/drivers/of/of_pci_irq.c
> index 9312516..6770538 100644
> --- a/drivers/of/of_pci_irq.c
> +++ b/drivers/of/of_pci_irq.c
> @@ -15,7 +15,7 @@
>    * PCI tree until an device-node is found, at which point it will finish
>    * resolving using the OF tree walking.
>    */
> -int of_irq_map_pci(struct pci_dev *pdev, struct of_irq *out_irq)
> +int of_irq_map_pci(const struct pci_dev *pdev, struct of_irq *out_irq)
>   {
>   	struct device_node *dn, *ppnode;
>   	struct pci_dev *ppdev;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d20f133..4c79586 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2363,7 +2363,7 @@ void pci_enable_acs(struct pci_dev *dev)
>    * number is always 0 (see the Implementation Note in section 2.2.8.1 of
>    * the PCI Express Base Specification, Revision 2.1)
>    */
> -u8 pci_swizzle_interrupt_pin(struct pci_dev *dev, u8 pin)
> +u8 pci_swizzle_interrupt_pin(const struct pci_dev *dev, u8 pin)
>   {
>   	int slot;
>
> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> index f93e217..bb115de 100644
> --- a/include/linux/of_pci.h
> +++ b/include/linux/of_pci.h
> @@ -5,7 +5,7 @@
>
>   struct pci_dev;
>   struct of_irq;
> -int of_irq_map_pci(struct pci_dev *pdev, struct of_irq *out_irq);
> +int of_irq_map_pci(const struct pci_dev *pdev, struct of_irq *out_irq);
>
>   struct device_node;
>   struct device_node *of_pci_find_child_device(struct device_node *parent,
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e444f5b..3bbc77e 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -680,7 +680,7 @@ int __must_check pci_bus_add_device(struct pci_dev *dev);
>   void pci_read_bridge_bases(struct pci_bus *child);
>   struct resource *pci_find_parent_resource(const struct pci_dev *dev,
>   					  struct resource *res);
> -u8 pci_swizzle_interrupt_pin(struct pci_dev *dev, u8 pin);
> +u8 pci_swizzle_interrupt_pin(const struct pci_dev *dev, u8 pin);
>   int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
>   u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
>   extern struct pci_dev *pci_dev_get(struct pci_dev *dev);
> @@ -1685,7 +1685,8 @@ extern void pci_release_bus_of_node(struct pci_bus *bus);
>   /* Arch may override this (weak) */
>   extern struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus);
>
> -static inline struct device_node *pci_device_to_OF_node(struct pci_dev *pdev)
> +static inline struct device_node *
> +pci_device_to_OF_node(const struct pci_dev *pdev)
>   {
>   	return pdev ? pdev->dev.of_node : NULL;
>   }

Re: [PATCH] OF: PCI: const usage needed by MIPS

[John Crispin]

On 30/04/12 19:54, David Daney wrote:
> On 04/30/2012 10:46 AM, John Crispin wrote:
>> On MIPS we want to call of_irq_map_pci from inside
>>
>> arch/mips/include/asm/pci.h:extern int pcibios_map_irq(
>>                 const struct pci_dev *dev, u8 slot, u8 pin);
>>
>> For this to work we need to change several functions to const usage.
>
> I think there is a mismatch on this throughout the kernel.
>
> Properly fixing it requires touching many more places than these.
> Although I haven't tried it, I wouldn't be surprised if doing this
> caused warnings to appear in non-MIPS code.
>
> Ralf had a patch at one point that tried to make this consistent
> tree-wide, but it is not yet applied.
>
> David Daney

Hi,

Ok, lets see what Ralf has to say.

I just tested the patch on x86 with OF enabled and drivers turned on
that use the API. I did not see any errors appear.

Thanks,
John

Re: [PATCH] OF: PCI: const usage needed by MIPS

[Bjorn Helgaas]

On Tue, May 1, 2012 at 7:28 AM, John Crispin <blogic@openwrt.org> wrote:
> On 30/04/12 19:54, David Daney wrote:
>> On 04/30/2012 10:46 AM, John Crispin wrote:
>>> On MIPS we want to call of_irq_map_pci from inside
>>>
>>> arch/mips/include/asm/pci.h:extern int pcibios_map_irq(
>>>                 const struct pci_dev *dev, u8 slot, u8 pin);
>>>
>>> For this to work we need to change several functions to const usage.
>>
>> I think there is a mismatch on this throughout the kernel.
>>
>> Properly fixing it requires touching many more places than these.
>> Although I haven't tried it, I wouldn't be surprised if doing this
>> caused warnings to appear in non-MIPS code.
>>
>> Ralf had a patch at one point that tried to make this consistent
>> tree-wide, but it is not yet applied.
>>
>> David Daney
>
> Hi,
>
> Ok, lets see what Ralf has to say.
>
> I just tested the patch on x86 with OF enabled and drivers turned on
> that use the API. I did not see any errors appear.

I'm far from a const expert, but I think this should be safe.  Here's
my reasoning:

We're changing pci_swizzle_interrupt_pin() to take a pointer to a
constant struct pci_dev.  pci_swizzle_interrupt_pin() only reads the
struct pci_dev; it doesn't modify it.  It is legal to pass either
"struct pci_dev *" or "const struct pci_dev *" to a function expecting
"const struct pci_dev *"; the callee just won't be able to modify the
struct, even if the caller can.

Similar reasoning applies to of_irq_map_pci().

So I'm fine with this.  You sent it to Grant, so I'll assume he'll
merge it unless I hear otherwise.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Re: [PATCH] OF: PCI: const usage needed by MIPS

[David Daney]

On 05/03/2012 05:30 PM, Bjorn Helgaas wrote:
> On Tue, May 1, 2012 at 7:28 AM, John Crispin<blogic@openwrt.org>  wrote:
>> On 30/04/12 19:54, David Daney wrote:
>>> On 04/30/2012 10:46 AM, John Crispin wrote:
>>>> On MIPS we want to call of_irq_map_pci from inside
>>>>
>>>> arch/mips/include/asm/pci.h:extern int pcibios_map_irq(
>>>>                  const struct pci_dev *dev, u8 slot, u8 pin);
>>>>
>>>> For this to work we need to change several functions to const usage.
>>>
>>> I think there is a mismatch on this throughout the kernel.
>>>
>>> Properly fixing it requires touching many more places than these.
>>> Although I haven't tried it, I wouldn't be surprised if doing this
>>> caused warnings to appear in non-MIPS code.
>>>
>>> Ralf had a patch at one point that tried to make this consistent
>>> tree-wide, but it is not yet applied.
>>>
>>> David Daney
>>
>> Hi,
>>
>> Ok, lets see what Ralf has to say.
>>
>> I just tested the patch on x86 with OF enabled and drivers turned on
>> that use the API. I did not see any errors appear.
>
> I'm far from a const expert, but I think this should be safe.

> Here's my reasoning:
>
> We're changing pci_swizzle_interrupt_pin() to take a pointer to a
> constant struct pci_dev.  pci_swizzle_interrupt_pin() only reads the
> struct pci_dev; it doesn't modify it.  It is legal to pass either
> "struct pci_dev *" or "const struct pci_dev *" to a function expecting
> "const struct pci_dev *"; the callee just won't be able to modify the
> struct, even if the caller can.
>

The problem is when you start declaring function pointers in various ops 
vectors.

Consider:

void (*foo)(const struct pci_dev *)
void (*bar)(struct pci_dev *)

foo and bar are not type compatible, and you will get compiler warnings 
if you use one where the other is expected.

So the question is:  Are we ever going to the address of any of the 
functions that are being modified?  If so, we have created a problem.

> Similar reasoning applies to of_irq_map_pci().
>
> So I'm fine with this.  You sent it to Grant, so I'll assume he'll
> merge it unless I hear otherwise.
>
> Acked-by: Bjorn Helgaas<bhelgaas@google.com>
>

Re: [PATCH] OF: PCI: const usage needed by MIPS

[John Crispin]

Hi David,

> The problem is when you start declaring function pointers in various
> ops vectors.
>
> Consider:
>
> void (*foo)(const struct pci_dev *)
> void (*bar)(struct pci_dev *)
>
> foo and bar are not type compatible, and you will get compiler
> warnings if you use one where the other is expected.
>
> So the question is:  Are we ever going to the address of any of the
> functions that are being modified?  If so, we have created a problem.
>



i could not find any place in the code where this happens, which does
not mean that there are none.


>> Similar reasoning applies to of_irq_map_pci().
>>
>> So I'm fine with this.  You sent it to Grant, so I'll assume he'll
>> merge it unless I hear otherwise.
>>
>> Acked-by: Bjorn Helgaas<bhelgaas@google.com>
>>
>

Thanks for the Ack, i hope this patch gets accepted as is. I am simply
missing the overview of the pci subsystem to evaluate if this can cause
regressions.


John

Re: [PATCH] OF: PCI: const usage needed by MIPS

[Bjorn Helgaas]

On Fri, May 4, 2012 at 3:55 AM, John Crispin <blogic@openwrt.org> wrote:
> Hi David,
>
>> The problem is when you start declaring function pointers in various
>> ops vectors.
>>
>> Consider:
>>
>> void (*foo)(const struct pci_dev *)
>> void (*bar)(struct pci_dev *)
>>
>> foo and bar are not type compatible, and you will get compiler
>> warnings if you use one where the other is expected.

Oh, right.  I vaguely remember tripping over this a few years ago when
I refactored pci_swizzle_interrupt_pin().  Thanks for enhancing my
simple understanding.

>> So the question is:  Are we ever going to the address of any of the
>> functions that are being modified?  If so, we have created a problem.
>
> i could not find any place in the code where this happens, which does
> not mean that there are none.

I compiled alpha, ia64, mips, parisc, powerpc, sh, sparc, and x86 and
didn't see any issues related to this patch.  There might still be
something,  but I'm willing to help work through them or revert this
if it turns out to be a problem.  I'm still assuming that Grant will
handle this.

Bjorn

Re: [PATCH] OF: PCI: const usage needed by MIPS

[John Crispin]

Hi Bjorn,
> I compiled alpha, ia64, mips, parisc, powerpc, sh, sparc, and x86 and
> didn't see any issues related to this patch.  There might still be
> something,  but I'm willing to help work through them or revert this
> if it turns out to be a problem.  I'm still assuming that Grant will
> handle this.
>
> Bjorn

Thanks for the help,
John

Re: [PATCH] OF: PCI: const usage needed by MIPS

[John Crispin]

> I compiled alpha, ia64, mips, parisc, powerpc, sh, sparc, and x86 and
> didn't see any issues related to this patch.  There might still be
> something,  but I'm willing to help work through them or revert this
> if it turns out to be a problem.  I'm still assuming that Grant will
> handle this.
>
> Bjorn
Hi Grant,

Is this patch ok with you ? If so would you mind if Ralf takes this via
his tree ? (this would avoid merge order problems)

Thanks,
John

Re: [PATCH] OF: PCI: const usage needed by MIPS

[Rob Herring]

On 05/12/2012 12:55 AM, John Crispin wrote:
> 
>> I compiled alpha, ia64, mips, parisc, powerpc, sh, sparc, and x86 and
>> didn't see any issues related to this patch.  There might still be
>> something,  but I'm willing to help work through them or revert this
>> if it turns out to be a problem.  I'm still assuming that Grant will
>> handle this.
>>
>> Bjorn
> Hi Grant,
> 
> Is this patch ok with you ? If so would you mind if Ralf takes this via
> his tree ? (this would avoid merge order problems)
> 

This looks fine to me and merging thru Ralf's tree is fine.

Acked-by: Rob Herring <rob.herring@calxeda.com>

Rob

> Thanks,
> John
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss