From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752669AbdARSVT (ORCPT ); Wed, 18 Jan 2017 13:21:19 -0500 Received: from mail.skyhub.de ([78.46.96.112]:44685 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751316AbdARSVR (ORCPT ); Wed, 18 Jan 2017 13:21:17 -0500 Date: Wed, 18 Jan 2017 19:20:55 +0100 From: Borislav Petkov To: Suravee Suthikulpanit Cc: linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, joro@8bytes.org, peterz@infradead.org, mingo@redhat.com Subject: Re: [PATCH v8 2/9] perf/amd/iommu: Clean up perf_iommu_enable_event Message-ID: <20170118182055.wpgo5b5xe3m4nrql@pd.tnic> References: <1484551416-5440-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1484551416-5440-3-git-send-email-Suravee.Suthikulpanit@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1484551416-5440-3-git-send-email-Suravee.Suthikulpanit@amd.com> User-Agent: NeoMutt/20161014 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 16, 2017 at 01:23:29AM -0600, Suravee Suthikulpanit wrote: > From: Suravee Suthikulpanit > > * Clean up various bitwise operations in perf_iommu_enable_event () It is a function. > * Make use macros BIT(x) "Make use"? > This should not affect logic and functionality. > > Cc: Peter Zijlstra > Cc: Borislav Petkov > Signed-off-by: Suravee Suthikulpanit > --- > arch/x86/events/amd/iommu.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c > index 44638d0..1aa25d8 100644 > --- a/arch/x86/events/amd/iommu.c > +++ b/arch/x86/events/amd/iommu.c > @@ -164,11 +164,11 @@ static int get_next_avail_iommu_bnk_cntr(struct perf_amd_iommu *perf_iommu) > for (bank = 0, shift = 0; bank < max_banks; bank++) { > for (cntr = 0; cntr < max_cntrs; cntr++) { > shift = bank + (bank*3) + cntr; > - if (perf_iommu->cntr_assign_mask & (1ULL< + if (perf_iommu->cntr_assign_mask & BIT(shift)) { BIT_ULL() otherwise you're introducing a bug. > continue; > } else { > - perf_iommu->cntr_assign_mask |= (1ULL< - retval = ((u16)((u16)bank<<8) | (u8)(cntr)); > + perf_iommu->cntr_assign_mask |= BIT(shift); Ditto. > + retval = ((u16)((u16)bank << 8) | (u8)(cntr)); That's some ugly casting. Why? (u8)(cntr) is supposed to do what exactly? Cut off to 255 if max_cntrs grows bigger? Can that even happen? Same for bank? Then you're casting to u16 even though retval is int. Ah, because it can be negative too. How about this instead? retval = ((bank & 0xff) << 8) | (cntr & 0xff); This way it is really obvious what you're trying to do. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.