| Message ID | 1335808019-24502-1-git-send-email-blogic@openwrt.org |
|---|---|
| State | Not Applicable |
| Headers | show |
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; > } -- 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
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 -- 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
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> -- 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
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> > -- 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
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 -- 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
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 -- 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
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 -- 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
> 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 -- 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
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 -- 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
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; }
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(-)