| Message ID | 1464859685-18666-1-git-send-email-maxg@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> IB_DEVICE_CROSS_CHANNEL = (1 << 27), > IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), > IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), > - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), > + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), > IB_DEVICE_SG_GAPS_REG = (1ULL << 32), > IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), > IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34), So now we've got three different styles to define the constants. I'd really prefer to move all of them over to the 1ULL << offset style. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/2/2016 2:33 PM, Christoph Hellwig wrote: >> IB_DEVICE_CROSS_CHANNEL = (1 << 27), >> IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), >> IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), >> - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), >> + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), >> IB_DEVICE_SG_GAPS_REG = (1ULL << 32), >> IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), >> IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34), > > So now we've got three different styles to define the constants. My next patch will align casting of IB_DEVICE_VIRTUAL_FUNCTION and IB_DEVICE_RAW_SCATTER_FCS to ULL. > > I'd really prefer to move all of them over to the 1ULL << offset style. me too :) > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> IB_DEVICE_CROSS_CHANNEL = (1 << 27), >>> IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), >>> IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), >>> - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), >>> + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), >>> IB_DEVICE_SG_GAPS_REG = (1ULL << 32), >>> IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), >>> IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34), >> >> So now we've got three different styles to define the constants. > > My next patch will align casting of IB_DEVICE_VIRTUAL_FUNCTION and > IB_DEVICE_RAW_SCATTER_FCS to ULL. > >> >> I'd really prefer to move all of them over to the 1ULL << offset style. > > me too :) Sounds good to me too... Max, give it a day or two before resending (trying to save you the multi-respin overhead). Also, please next time when you submit a patch make sure to document the patch version and what was changed from the last version. Cheers, Sagi. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote: > ib_device_cap_flags 64-bit expansion caused caps overlapping > and made consumers read wrong device capabilities. For example > IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing > it to use a non-existing capability. This happened because signed > int becomes sign extended when converted it to u64. Fix this by > casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL. > > Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags') > Reported-by: Robert LeBlanc <robert@leblancnet.us> > Cc: Stable <stable@vger.kernel.org> #[v4.6+] > Acked-by: Sagi Grimberg <sagi@grimberg.me> > Signed-off-by: Max Gurtovoy <maxg@mellanox.com> > Signed-off-by: Matan Barak <matanb@mellanox.com> > --- > include/rdma/ib_verbs.h | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index 432bed5..c97357b 100644 > --- a/include/rdma/ib_verbs.h > +++ b/include/rdma/ib_verbs.h > @@ -217,7 +217,7 @@ enum ib_device_cap_flags { > IB_DEVICE_CROSS_CHANNEL = (1 << 27), > IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), > IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), > - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), > + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), > IB_DEVICE_SG_GAPS_REG = (1ULL << 32), > IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), > IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34), Why not just use the BIT() and BIT_ULL() macros instead of "open coding" these? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/02/2016 09:24 AM, Greg KH wrote: > On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote: >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h >> index 432bed5..c97357b 100644 >> --- a/include/rdma/ib_verbs.h >> +++ b/include/rdma/ib_verbs.h >> @@ -217,7 +217,7 @@ enum ib_device_cap_flags { >> IB_DEVICE_CROSS_CHANNEL = (1 << 27), >> IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), >> IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), >> - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), >> + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), >> IB_DEVICE_SG_GAPS_REG = (1ULL << 32), >> IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), >> IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34), > > Why not just use the BIT() and BIT_ULL() macros instead of "open coding" > these? Hi Greg, Are you sure that these macros are useful? My personal opinion is that these macros makes code harder to read instead of easier. I'd like to see these macros disappear. Thanks, Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/2/2016 7:24 PM, Greg KH wrote: > On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote: >> ib_device_cap_flags 64-bit expansion caused caps overlapping >> and made consumers read wrong device capabilities. For example >> IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing >> it to use a non-existing capability. This happened because signed >> int becomes sign extended when converted it to u64. Fix this by >> casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL. >> >> Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags') >> Reported-by: Robert LeBlanc <robert@leblancnet.us> >> Cc: Stable <stable@vger.kernel.org> #[v4.6+] >> Acked-by: Sagi Grimberg <sagi@grimberg.me> >> Signed-off-by: Max Gurtovoy <maxg@mellanox.com> >> Signed-off-by: Matan Barak <matanb@mellanox.com> >> --- >> include/rdma/ib_verbs.h | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h >> index 432bed5..c97357b 100644 >> --- a/include/rdma/ib_verbs.h >> +++ b/include/rdma/ib_verbs.h >> @@ -217,7 +217,7 @@ enum ib_device_cap_flags { >> IB_DEVICE_CROSS_CHANNEL = (1 << 27), >> IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), >> IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), >> - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), >> + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), >> IB_DEVICE_SG_GAPS_REG = (1ULL << 32), >> IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), >> IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34), > > Why not just use the BIT() and BIT_ULL() macros instead of "open coding" > these? Good idea. do you think it will be a good idea to set all the elements in this ib_device_cap_flags enum to BIT_ULL() in this patch ? to have 1 style instead of many ? > > thanks, > > greg k-h > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2 Jun 2016, Greg KH wrote: > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > index 432bed5..c97357b 100644 > > --- a/include/rdma/ib_verbs.h > > +++ b/include/rdma/ib_verbs.h > > @@ -217,7 +217,7 @@ enum ib_device_cap_flags { > > IB_DEVICE_CROSS_CHANNEL = (1 << 27), > > IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), > > IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), > > - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), > > + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), > > IB_DEVICE_SG_GAPS_REG = (1ULL << 32), > > IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), > > IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34), > > Why not just use the BIT() and BIT_ULL() macros instead of "open coding" > these? This whole enum approach for powers of two also looks not that good. Do an unsigned long long or u64 please? Use enum to define the individual bits and not for the bitmask? Could add _MASK for that if you really want it. unsigned long long ib_device_cap_flags; enum ib_dev_caps = { IB_DEVICE_CROSS_CHANNEL, IB_DEVICE_SG_GAPS_REG, .... NR_IB_DEV_CAPS }; Then use BIT_ULL(IB_DEVICE_CROSS_CHANNEL) in the code? Or just specify the bitmasks using hex constants? from sched.h: #define SD_LOAD_BALANCE 0x0001 /* Do load balancing on this domain. */ #define SD_BALANCE_NEWIDLE 0x0002 /* Balance when about to become idle */ #define SD_BALANCE_EXEC 0x0004 /* Balance on exec */ #define SD_BALANCE_FORK 0x0008 /* Balance on fork, clone */ #define SD_BALANCE_WAKE 0x0010 /* Balance on wakeup */ #define SD_WAKE_AFFINE 0x0020 /* Wake task to waking CPU */ #define SD_SHARE_CPUCAPACITY 0x0080 /* Domain members share cpu power */ #define SD_SHARE_POWERDOMAIN 0x0100 /* Domain members share power domain */ #define SD_SHARE_PKG_RESOURCES 0x0200 /* Domain members share cpu pkg resources */ #define SD_SERIALIZE 0x0400 /* Only a single load balancing instance */ #define SD_ASYM_PACKING 0x0800 /* Place busy groups earlier in the domain */ #define SD_PREFER_SIBLING 0x1000 /* Prefer to place tasks in a sibling domain */ #define SD_OVERLAP 0x2000 /* sched_domains of this level overlap */ #define SD_NUMA 0x4000 /* cross-node balancing */ -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 02, 2016 at 04:33:53AM -0700, Christoph Hellwig wrote: > > IB_DEVICE_CROSS_CHANNEL = (1 << 27), > > IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), > > IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), > > - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), > > + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), > > IB_DEVICE_SG_GAPS_REG = (1ULL << 32), > > IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), > > IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34), > > So now we've got three different styles to define the constants. > > I'd really prefer to move all of them over to the 1ULL << offset style. If people can agree on the style that would be a nice followup cleanup patch, but since this fixes a bug and needs a pass through stable shouldn't it stay succinct like this? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote: > ib_device_cap_flags 64-bit expansion caused caps overlapping > and made consumers read wrong device capabilities. For example > IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing > it to use a non-existing capability. This happened because signed > int becomes sign extended when converted it to u64. Fix this by > casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL. > > Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags') > Reported-by: Robert LeBlanc <robert@leblancnet.us> > Cc: Stable <stable@vger.kernel.org> #[v4.6+] > Acked-by: Sagi Grimberg <sagi@grimberg.me> > Signed-off-by: Max Gurtovoy <maxg@mellanox.com> > Signed-off-by: Matan Barak <matanb@mellanox.com> > include/rdma/ib_verbs.h | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 02, 2016 at 09:41:03AM -0700, Bart Van Assche wrote: > On 06/02/2016 09:24 AM, Greg KH wrote: > > On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote: > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > index 432bed5..c97357b 100644 > > > --- a/include/rdma/ib_verbs.h > > > +++ b/include/rdma/ib_verbs.h > > > @@ -217,7 +217,7 @@ enum ib_device_cap_flags { > > > IB_DEVICE_CROSS_CHANNEL = (1 << 27), > > > IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), > > > IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), > > > - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), > > > + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), > > > IB_DEVICE_SG_GAPS_REG = (1ULL << 32), > > > IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), > > > IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34), > > > > Why not just use the BIT() and BIT_ULL() macros instead of "open coding" > > these? > > Hi Greg, > > Are you sure that these macros are useful? Yes. > My personal opinion is that these macros makes code harder to read > instead of easier. I'd like to see these macros disappear. Sorry but we are converting the whole kernel to use them, I suggest you get used to them :) greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 02, 2016 at 07:41:10PM +0300, Max Gurtovoy wrote: > > > On 6/2/2016 7:24 PM, Greg KH wrote: > > On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote: > > > ib_device_cap_flags 64-bit expansion caused caps overlapping > > > and made consumers read wrong device capabilities. For example > > > IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing > > > it to use a non-existing capability. This happened because signed > > > int becomes sign extended when converted it to u64. Fix this by > > > casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL. > > > > > > Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags') > > > Reported-by: Robert LeBlanc <robert@leblancnet.us> > > > Cc: Stable <stable@vger.kernel.org> #[v4.6+] > > > Acked-by: Sagi Grimberg <sagi@grimberg.me> > > > Signed-off-by: Max Gurtovoy <maxg@mellanox.com> > > > Signed-off-by: Matan Barak <matanb@mellanox.com> > > > --- > > > include/rdma/ib_verbs.h | 2 +- > > > 1 files changed, 1 insertions(+), 1 deletions(-) > > > > > > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > > > index 432bed5..c97357b 100644 > > > --- a/include/rdma/ib_verbs.h > > > +++ b/include/rdma/ib_verbs.h > > > @@ -217,7 +217,7 @@ enum ib_device_cap_flags { > > > IB_DEVICE_CROSS_CHANNEL = (1 << 27), > > > IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), > > > IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), > > > - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), > > > + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), > > > IB_DEVICE_SG_GAPS_REG = (1ULL << 32), > > > IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), > > > IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34), > > > > Why not just use the BIT() and BIT_ULL() macros instead of "open coding" > > these? > > Good idea. do you think it will be a good idea to set all the elements in > this ib_device_cap_flags enum to BIT_ULL() in this patch ? to have 1 style > instead of many ? Yes. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 02, 2016 at 12:28:05PM +0300, Max Gurtovoy wrote: > ib_device_cap_flags 64-bit expansion caused caps overlapping > and made consumers read wrong device capabilities. For example > IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing > it to use a non-existing capability. This happened because signed > int becomes sign extended when converted it to u64. Fix this by > casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL. > > Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags') Everything fine, except this Fixes line, which is similar to the issue, but unrelated. The sign extension error was produced after adding bit(32) to the enum and the commit fb532d6a79b9 didn't do it and didn't mean to do it.
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 432bed5..c97357b 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -217,7 +217,7 @@ enum ib_device_cap_flags { IB_DEVICE_CROSS_CHANNEL = (1 << 27), IB_DEVICE_MANAGED_FLOW_STEERING = (1 << 29), IB_DEVICE_SIGNATURE_HANDOVER = (1 << 30), - IB_DEVICE_ON_DEMAND_PAGING = (1 << 31), + IB_DEVICE_ON_DEMAND_PAGING = (1ULL << 31), IB_DEVICE_SG_GAPS_REG = (1ULL << 32), IB_DEVICE_VIRTUAL_FUNCTION = ((u64)1 << 33), IB_DEVICE_RAW_SCATTER_FCS = ((u64)1 << 34),
ib_device_cap_flags 64-bit expansion caused caps overlapping and made consumers read wrong device capabilities. For example IB_DEVICE_SG_GAPS_REG was falsely read by the iser driver causing it to use a non-existing capability. This happened because signed int becomes sign extended when converted it to u64. Fix this by casting IB_DEVICE_ON_DEMAND_PAGING enumeration to ULL. Fixes: fb532d6a79b9 ('IB/{core, ulp} Support above 32 possible device capability flags') Reported-by: Robert LeBlanc <robert@leblancnet.us> Cc: Stable <stable@vger.kernel.org> #[v4.6+] Acked-by: Sagi Grimberg <sagi@grimberg.me> Signed-off-by: Max Gurtovoy <maxg@mellanox.com> Signed-off-by: Matan Barak <matanb@mellanox.com> --- include/rdma/ib_verbs.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)