| Message ID | 1383743117-4138-1-git-send-email-treding@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 06, 2013 at 01:05:17PM +0000, Thierry Reding wrote: > Commit 3d1975b57097 (arm,arm64: do not always merge bio_vec if we are > running on Xen) unconditionally added code using the bio_vec typedefs > which causes build errors on configurations where CONFIG_BLOCK is > disabled. I guess this commit is only in -next. Would it have the same hash when hitting mainline? Otherwise I'm fine with the patch, maybe Stephano can pick it together with the patch that breaks it.
On Wed, Nov 6, 2013 at 5:05 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > Commit 3d1975b57097 (arm,arm64: do not always merge bio_vec if we are > running on Xen) unconditionally added code using the bio_vec typedefs > which causes build errors on configurations where CONFIG_BLOCK is > disabled. > > Add #ifdef CONFIG_BLOCK protection to fix this. > > Signed-off-by: Thierry Reding <treding@nvidia.com> I commented on the offending patch (which is a good way to make people aware of it instead of posting standalone patches like you did), and Stefano posted patch that declares struct bio_vec; instead. Either way is ok with me, I'd generally prefer to avoid the ifdefs though. -Olof
On Wed, 6 Nov 2013, Thierry Reding wrote: > Commit 3d1975b57097 (arm,arm64: do not always merge bio_vec if we are > running on Xen) unconditionally added code using the bio_vec typedefs > which causes build errors on configurations where CONFIG_BLOCK is > disabled. > > Add #ifdef CONFIG_BLOCK protection to fix this. > > Signed-off-by: Thierry Reding <treding@nvidia.com> Thierry, thank you for the patch. However I was thinking of declaring struct bio_vec instead, see: http://marc.info/?l=linux-kernel&m=138374169030300&w=2 > arch/arm/include/asm/io.h | 2 ++ > arch/arm64/include/asm/io.h | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h > index c45effc..68ef696 100644 > --- a/arch/arm/include/asm/io.h > +++ b/arch/arm/include/asm/io.h > @@ -367,6 +367,7 @@ struct pci_dev; > > extern void pci_iounmap(struct pci_dev *dev, void __iomem *addr); > > +#ifdef CONFIG_BLOCK > /* > * can the hardware map this into one segment or not, given no other > * constraints. > @@ -379,6 +380,7 @@ extern bool xen_biovec_phys_mergeable(const struct bio_vec *vec1, > #define BIOVEC_PHYS_MERGEABLE(vec1, vec2) \ > (__BIOVEC_PHYS_MERGEABLE(vec1, vec2) && \ > (!xen_domain() || xen_biovec_phys_mergeable(vec1, vec2))) > +#endif > > #ifdef CONFIG_MMU > #define ARCH_HAS_VALID_PHYS_ADDR_RANGE > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > index 5a482fc..940dbe2 100644 > --- a/arch/arm64/include/asm/io.h > +++ b/arch/arm64/include/asm/io.h > @@ -266,11 +266,13 @@ extern int devmem_is_allowed(unsigned long pfn); > */ > #define xlate_dev_kmem_ptr(p) p > > +#ifdef CONFIG_BLOCK > extern bool xen_biovec_phys_mergeable(const struct bio_vec *vec1, > const struct bio_vec *vec2); > #define BIOVEC_PHYS_MERGEABLE(vec1, vec2) \ > (__BIOVEC_PHYS_MERGEABLE(vec1, vec2) && \ > (!xen_domain() || xen_biovec_phys_mergeable(vec1, vec2))) > +#endif > > #endif /* __KERNEL__ */ > #endif /* __ASM_IO_H */ > -- > 1.8.4 >
On Wed, 6 Nov 2013, Catalin Marinas wrote: > On Wed, Nov 06, 2013 at 01:05:17PM +0000, Thierry Reding wrote: > > Commit 3d1975b57097 (arm,arm64: do not always merge bio_vec if we are > > running on Xen) unconditionally added code using the bio_vec typedefs > > which causes build errors on configurations where CONFIG_BLOCK is > > disabled. > > I guess this commit is only in -next. Would it have the same hash when > hitting mainline? Otherwise I'm fine with the patch, maybe Stephano can > pick it together with the patch that breaks it. I sent this patch earlier today to fix the problem: http://marc.info/?l=linux-kernel&m=138374169030300&w=2 I prefer it to adding more ifdefs. I was going to add it to linux-next as soon as possible and then send it upstream together with the rest of the series.
On Wed, Nov 06, 2013 at 07:40:43AM -0800, Olof Johansson wrote: > On Wed, Nov 6, 2013 at 5:05 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > > Commit 3d1975b57097 (arm,arm64: do not always merge bio_vec if we are > > running on Xen) unconditionally added code using the bio_vec typedefs > > which causes build errors on configurations where CONFIG_BLOCK is > > disabled. > > > > Add #ifdef CONFIG_BLOCK protection to fix this. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > I commented on the offending patch (which is a good way to make people > aware of it instead of posting standalone patches like you did), The standalone patch got Stefano's attention, so I don't see why you think it was a bad idea. I don't expect such patches to always be applied as is. If they aren't proper fixes and someone comes up with a much better way (which they did in this case), then I'm just as happy. Also it actually takes about the same amount of time to write up a patch, compile-test it and send it out than it takes to complain about the breakage by mail. I also find it slightly more helpful than just telling somebody that their patch broke something. Perhaps this is a bad example because it's really a trivial issue, but if it were anything slightly more complex, then sending a patch for it may save some maintainer the additional work. I don't immediately see what's wrong with that. Perhaps you'd care to elaborate. > and Stefano posted patch that declares struct bio_vec; instead. > > Either way is ok with me, I'd generally prefer to avoid the ifdefs though. I agree, that's much nicer than the #ifdef. Thierry
On Wed, Nov 06, 2013 at 04:21:09PM +0000, Stefano Stabellini wrote: > On Wed, 6 Nov 2013, Catalin Marinas wrote: > > On Wed, Nov 06, 2013 at 01:05:17PM +0000, Thierry Reding wrote: > > > Commit 3d1975b57097 (arm,arm64: do not always merge bio_vec if we are > > > running on Xen) unconditionally added code using the bio_vec typedefs > > > which causes build errors on configurations where CONFIG_BLOCK is > > > disabled. > > > > I guess this commit is only in -next. Would it have the same hash when > > hitting mainline? Otherwise I'm fine with the patch, maybe Stephano can > > pick it together with the patch that breaks it. > > I sent this patch earlier today to fix the problem: > > http://marc.info/?l=linux-kernel&m=138374169030300&w=2 > > I prefer it to adding more ifdefs. I was going to add it to linux-next > as soon as possible and then send it upstream together with the rest of > the series. I generally prefer to avoid #ifdefs too. Although there are cases where an #ifdef can be an advantage as well. For instance if you call the xen_biovec_phys_mergeable() function and don't have CONFIG_BLOCK enabled the kernel will happily compile but fail to link. That might be somewhat more difficult to figure out than a compile error. Perhaps it won't. I have no objections to your proposed patch, and it fixes the build issues, so I'm good. Thierry
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h index c45effc..68ef696 100644 --- a/arch/arm/include/asm/io.h +++ b/arch/arm/include/asm/io.h @@ -367,6 +367,7 @@ struct pci_dev; extern void pci_iounmap(struct pci_dev *dev, void __iomem *addr); +#ifdef CONFIG_BLOCK /* * can the hardware map this into one segment or not, given no other * constraints. @@ -379,6 +380,7 @@ extern bool xen_biovec_phys_mergeable(const struct bio_vec *vec1, #define BIOVEC_PHYS_MERGEABLE(vec1, vec2) \ (__BIOVEC_PHYS_MERGEABLE(vec1, vec2) && \ (!xen_domain() || xen_biovec_phys_mergeable(vec1, vec2))) +#endif #ifdef CONFIG_MMU #define ARCH_HAS_VALID_PHYS_ADDR_RANGE diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index 5a482fc..940dbe2 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -266,11 +266,13 @@ extern int devmem_is_allowed(unsigned long pfn); */ #define xlate_dev_kmem_ptr(p) p +#ifdef CONFIG_BLOCK extern bool xen_biovec_phys_mergeable(const struct bio_vec *vec1, const struct bio_vec *vec2); #define BIOVEC_PHYS_MERGEABLE(vec1, vec2) \ (__BIOVEC_PHYS_MERGEABLE(vec1, vec2) && \ (!xen_domain() || xen_biovec_phys_mergeable(vec1, vec2))) +#endif #endif /* __KERNEL__ */ #endif /* __ASM_IO_H */
Commit 3d1975b57097 (arm,arm64: do not always merge bio_vec if we are running on Xen) unconditionally added code using the bio_vec typedefs which causes build errors on configurations where CONFIG_BLOCK is disabled. Add #ifdef CONFIG_BLOCK protection to fix this. Signed-off-by: Thierry Reding <treding@nvidia.com> --- arch/arm/include/asm/io.h | 2 ++ arch/arm64/include/asm/io.h | 2 ++ 2 files changed, 4 insertions(+)