[PATCH] arm,arm64: Conditionalize bio_vec usage

[PATCH] arm,arm64: Conditionalize bio_vec usage

[Thierry Reding]

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(+)

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

[PATCH] arm,arm64: Conditionalize bio_vec usage

[Catalin Marinas]

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.

-- 
Catalin

[PATCH] arm,arm64: Conditionalize bio_vec usage

[Olof Johansson]

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

[PATCH] arm,arm64: Conditionalize bio_vec usage

[Stefano Stabellini]

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
> 

[PATCH] arm,arm64: Conditionalize bio_vec usage

[Stefano Stabellini]

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.

[PATCH] arm,arm64: Conditionalize bio_vec usage

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131111/1700a2d2/attachment.sig>

[PATCH] arm,arm64: Conditionalize bio_vec usage

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131111/e29912cc/attachment.sig>