[PATCH 1/1] Drivers: hv: vmbus: Rework use of DMA_BIT_MASK(64)

[PATCH 1/1] Drivers: hv: vmbus: Rework use of DMA_BIT_MASK(64)

[Michael Kelley]

Using DMA_BIT_MASK(64) as an initializer for a global variable
causes problems with Clang 12.0.1. The compiler doesn't understand
that value 64 is excluded from the shift at compile time, resulting
in a build error.

While this is a compiler problem, avoid the issue by setting up
the dma_mask memory as part of struct hv_device, and initialize
it using dma_set_mask().

Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Vitaly Chikunov <vt@altlinux.org>
Reported-by: Jakub Kicinski <kuba@kernel.org>
Fixes: 743b237c3a7b ("scsi: storvsc: Add Isolation VM support for storvsc driver")
Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/hv/vmbus_drv.c | 4 ++--
 include/linux/hyperv.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 17bf55f..0d96634 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2079,7 +2079,6 @@ struct hv_device *vmbus_device_create(const guid_t *type,
 	return child_device_obj;
 }
 
-static u64 vmbus_dma_mask = DMA_BIT_MASK(64);
 /*
  * vmbus_device_register - Register the child device
  */
@@ -2120,8 +2119,9 @@ int vmbus_device_register(struct hv_device *child_device_obj)
 	}
 	hv_debug_add_dev_dir(child_device_obj);
 
-	child_device_obj->device.dma_mask = &vmbus_dma_mask;
 	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
+	child_device_obj->device.dma_mask = &child_device_obj->dma_mask;
+	dma_set_mask(&child_device_obj->device, DMA_BIT_MASK(64));
 	return 0;
 
 err_kset_unregister:
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index f565a89..fe2e017 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1262,6 +1262,7 @@ struct hv_device {
 	struct vmbus_channel *channel;
 	struct kset	     *channels_kset;
 	struct device_dma_parameters dma_parms;
+	u64 dma_mask;
 
 	/* place holder to keep track of the dir for hv device in debugfs */
 	struct dentry *debug_dir;
-- 
1.8.3.1

Re: [PATCH 1/1] Drivers: hv: vmbus: Rework use of DMA_BIT_MASK(64)

[Nathan Chancellor]

Hi Michael,

On Sun, Feb 06, 2022 at 11:36:56AM -0800, Michael Kelley wrote:
> Using DMA_BIT_MASK(64) as an initializer for a global variable
> causes problems with Clang 12.0.1. The compiler doesn't understand
> that value 64 is excluded from the shift at compile time, resulting
> in a build error.
> 
> While this is a compiler problem, avoid the issue by setting up
> the dma_mask memory as part of struct hv_device, and initialize
> it using dma_set_mask().
> 
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Reported-by: Vitaly Chikunov <vt@altlinux.org>
> Reported-by: Jakub Kicinski <kuba@kernel.org>
> Fixes: 743b237c3a7b ("scsi: storvsc: Add Isolation VM support for storvsc driver")
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>

Thanks a lot for working around this. I am hoping that this will be
fixed in clang soon, as it is high priority on our list of issues to
fix. Once it has been fixed, we should be able to undo this workaround
in one way or another.

I can confirm the warning is resolved, which will allow us to build
ARCH=arm64 and ARCH=x86_64 allmodconfig with -Werror on mainline with
clang once another fix [1] is merged.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>

[1]: https://git.kernel.org/gregkh/usb/c/b470947c3672f7eb7c4c271d510383d896831cc2

> ---
>  drivers/hv/vmbus_drv.c | 4 ++--
>  include/linux/hyperv.h | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 17bf55f..0d96634 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -2079,7 +2079,6 @@ struct hv_device *vmbus_device_create(const guid_t *type,
>  	return child_device_obj;
>  }
>  
> -static u64 vmbus_dma_mask = DMA_BIT_MASK(64);
>  /*
>   * vmbus_device_register - Register the child device
>   */
> @@ -2120,8 +2119,9 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  	}
>  	hv_debug_add_dev_dir(child_device_obj);
>  
> -	child_device_obj->device.dma_mask = &vmbus_dma_mask;
>  	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
> +	child_device_obj->device.dma_mask = &child_device_obj->dma_mask;
> +	dma_set_mask(&child_device_obj->device, DMA_BIT_MASK(64));
>  	return 0;
>  
>  err_kset_unregister:
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index f565a89..fe2e017 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1262,6 +1262,7 @@ struct hv_device {
>  	struct vmbus_channel *channel;
>  	struct kset	     *channels_kset;
>  	struct device_dma_parameters dma_parms;
> +	u64 dma_mask;
>  
>  	/* place holder to keep track of the dir for hv device in debugfs */
>  	struct dentry *debug_dir;
> -- 
> 1.8.3.1
> 

RE: [PATCH 1/1] Drivers: hv: vmbus: Rework use of DMA_BIT_MASK(64)

[Michael Kelley (LINUX)]

From: Nathan Chancellor <nathan@kernel.org> Sent: Sunday, February 6, 2022 5:38 PM
> 
> Hi Michael,
> 
> On Sun, Feb 06, 2022 at 11:36:56AM -0800, Michael Kelley wrote:
> > Using DMA_BIT_MASK(64) as an initializer for a global variable
> > causes problems with Clang 12.0.1. The compiler doesn't understand
> > that value 64 is excluded from the shift at compile time, resulting
> > in a build error.
> >
> > While this is a compiler problem, avoid the issue by setting up
> > the dma_mask memory as part of struct hv_device, and initialize
> > it using dma_set_mask().
> >
> > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > Reported-by: Vitaly Chikunov <vt@altlinux.org>
> > Reported-by: Jakub Kicinski <kuba@kernel.org>
> > Fixes: 743b237c3a7b ("scsi: storvsc: Add Isolation VM support for storvsc driver")
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> 
> Thanks a lot for working around this. I am hoping that this will be
> fixed in clang soon, as it is high priority on our list of issues to
> fix. Once it has been fixed, we should be able to undo this workaround
> in one way or another.

FWIW, the new code is as good a solution as the old code.  The new code
also follows some existing patterns, such as with struct platform_device.
As such, I don't think of this as a workaround that needs to be undone
in the future.

Michael

> 
> I can confirm the warning is resolved, which will allow us to build
> ARCH=arm64 and ARCH=x86_64 allmodconfig with -Werror on mainline with
> clang once another fix [1] is merged.
> 
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> Tested-by: Nathan Chancellor <nathan@kernel.org>
> 

Re: [PATCH 1/1] Drivers: hv: vmbus: Rework use of DMA_BIT_MASK(64)

[Wei Liu]

On Mon, Feb 07, 2022 at 02:42:31AM +0000, Michael Kelley (LINUX) wrote:
> From: Nathan Chancellor <nathan@kernel.org> Sent: Sunday, February 6, 2022 5:38 PM
> > 
> > Hi Michael,
> > 
> > On Sun, Feb 06, 2022 at 11:36:56AM -0800, Michael Kelley wrote:
> > > Using DMA_BIT_MASK(64) as an initializer for a global variable
> > > causes problems with Clang 12.0.1. The compiler doesn't understand
> > > that value 64 is excluded from the shift at compile time, resulting
> > > in a build error.
> > >
> > > While this is a compiler problem, avoid the issue by setting up
> > > the dma_mask memory as part of struct hv_device, and initialize
> > > it using dma_set_mask().
> > >
> > > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > > Reported-by: Vitaly Chikunov <vt@altlinux.org>
> > > Reported-by: Jakub Kicinski <kuba@kernel.org>
> > > Fixes: 743b237c3a7b ("scsi: storvsc: Add Isolation VM support for storvsc driver")
> > > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > 
> > Thanks a lot for working around this. I am hoping that this will be
> > fixed in clang soon, as it is high priority on our list of issues to
> > fix. Once it has been fixed, we should be able to undo this workaround
> > in one way or another.
> 
> FWIW, the new code is as good a solution as the old code.  The new code
> also follows some existing patterns, such as with struct platform_device.
> As such, I don't think of this as a workaround that needs to be undone
> in the future.
> 

Yes indeed.

Patch applied to hyperv-fixes. Thanks.