Re: [PATCH] vfio/cdx: Add parentheses between bitwise AND expression and logical NOT

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 3 Oct 2023 08:27:39 -0700
Nathan Chancellor <nathan@kernel.org> wrote:

> On Tue, Oct 03, 2023 at 09:40:02AM +0200, Philippe Mathieu-Daudé wrote:
> > Hi Nathan,
> > 
> > On 2/10/23 19:53, Nathan Chancellor wrote:  
> > > When building with clang, there is a warning (or error with
> > > CONFIG_WERROR=y) due to a bitwise AND and logical NOT in
> > > vfio_cdx_bm_ctrl():
> > > 
> > >    drivers/vfio/cdx/main.c:77:6: error: logical not is only applied to the left hand side of this bitwise operator [-Werror,-Wlogical-not-parentheses]
> > >       77 |         if (!vdev->flags & BME_SUPPORT)
> > >          |             ^            ~
> > >    drivers/vfio/cdx/main.c:77:6: note: add parentheses after the '!' to evaluate the bitwise operator first
> > >       77 |         if (!vdev->flags & BME_SUPPORT)
> > >          |             ^
> > >          |              (                        )
> > >    drivers/vfio/cdx/main.c:77:6: note: add parentheses around left hand side expression to silence this warning
> > >       77 |         if (!vdev->flags & BME_SUPPORT)
> > >          |             ^
> > >          |             (           )
> > >    1 error generated.
> > > 
> > > Add the parentheses as suggested in the first note, which is clearly
> > > what was intended here.
> > > 
> > > Closes: https://github.com/ClangBuiltLinux/linux/issues/1939
> > > Fixes: 8a97ab9b8b31 ("vfio-cdx: add bus mastering device feature support")  
> > 
> > My current /master points to commit ce36c8b14987 which doesn't include
> > 8a97ab9b8b31, so maybe this can be squashed / reordered in the VFIO tree
> > (where I assume this commit is). That said, the fix is correct, so:  
> 
> Yes, this is a -next only issue at the moment and I don't mind this
> change being squashed into the original if Alex rebases his tree (some
> maintainers don't).

Right, where practical we try not to change commit hashes once
something has been included into linux-next, preferring to layer fixes
or even reverts, but occasionally something will come up where it makes
sense to rebase.  This is not such a case :)  Thanks,

Alex

> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>  
> 
> Thanks a lot for taking a look!
> 
> Cheers,
> Nathan
> 
> > > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > > ---
> > >   drivers/vfio/cdx/main.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c
> > > index a437630be354..a63744302b5e 100644
> > > --- a/drivers/vfio/cdx/main.c
> > > +++ b/drivers/vfio/cdx/main.c
> > > @@ -74,7 +74,7 @@ static int vfio_cdx_bm_ctrl(struct vfio_device *core_vdev, u32 flags,
> > >   	struct vfio_device_feature_bus_master ops;
> > >   	int ret;
> > > -	if (!vdev->flags & BME_SUPPORT)
> > > +	if (!(vdev->flags & BME_SUPPORT))
> > >   		return -ENOTTY;
> > >   	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
> > > 
> > > ---
> > > base-commit: fcb2f2ed4a80cfe383d87da75caba958516507e9
> > > change-id: 20231002-vfio-cdx-logical-not-parentheses-aca8fbd6b278
> > > 
> > > Best regards,  
> >   
>