| Message ID | 20240614094338.467052-1-poros@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net,v2] ice: use proper macro for testing bit | expand |
On Fri, Jun 14, 2024 at 11:43:38AM +0200, Petr Oros wrote: > Do not use _test_bit() macro for testing bit. The proper macro for this > is one without underline. Hi Petr, it might be nice to include a brief explanation as to why test_bit() is correct. > > Fixes: 4da71a77fc3b ("ice: read internal temperature sensor") > Signed-off-by: Petr Oros <poros@redhat.com> > Acked-by: Ivan Vecera <ivecera@redhat.com> ...
From: Simon Horman <horms@kernel.org> Date: Sat, 15 Jun 2024 16:16:41 +0100 > On Fri, Jun 14, 2024 at 11:43:38AM +0200, Petr Oros wrote: >> Do not use _test_bit() macro for testing bit. The proper macro for this >> is one without underline. > > Hi Petr, > > it might be nice to include a brief explanation as to > why test_bit() is correct. Let me explain this as the author of all those bitops wrappers :D Petr is free to include either this or his own brief into v2. _test_bit() is what test_bit() was prior to my const-optimization. It directly calls arch_test_bit(), i.e. the arch-specific implementation (or the generic one). It's strictly _internal_ and shouldn't be used anywhere outside the actual test_bit() macro. test_bit() is a wrapper which checks whether the bitmap and the bit number are compile-time constants and if so, it calls the optimized function which evaluates this call to a compile-time constant as well. If either of them is not a compile-time constant, it just calls _test_bit(). test_bit() is the actual function to use anywhere in the kernel. IOW, calling _test_bit() avoids potential compile-time optimizations. From what I see in the code, &sensors is not a compile-time constant, thus most probably there are no object code changes before and after the patch. But anyway, we shouldn't call internal wrappers instead of the actual API, so this fix is correct. > >> >> Fixes: 4da71a77fc3b ("ice: read internal temperature sensor") >> Signed-off-by: Petr Oros <poros@redhat.com> >> Acked-by: Ivan Vecera <ivecera@redhat.com> To be added to v2: Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com> Thanks, Olek
On Mon, Jun 17, 2024 at 02:58:59PM +0200, Alexander Lobakin wrote: > From: Simon Horman <horms@kernel.org> > Date: Sat, 15 Jun 2024 16:16:41 +0100 > > > On Fri, Jun 14, 2024 at 11:43:38AM +0200, Petr Oros wrote: > >> Do not use _test_bit() macro for testing bit. The proper macro for this > >> is one without underline. > > > > Hi Petr, > > > > it might be nice to include a brief explanation as to > > why test_bit() is correct. > > Let me explain this as the author of all those bitops wrappers :D > Petr is free to include either this or his own brief into v2. > > _test_bit() is what test_bit() was prior to my const-optimization. It > directly calls arch_test_bit(), i.e. the arch-specific implementation > (or the generic one). It's strictly _internal_ and shouldn't be used > anywhere outside the actual test_bit() macro. > > test_bit() is a wrapper which checks whether the bitmap and the bit > number are compile-time constants and if so, it calls the optimized > function which evaluates this call to a compile-time constant as well. > If either of them is not a compile-time constant, it just calls _test_bit(). > test_bit() is the actual function to use anywhere in the kernel. > > IOW, calling _test_bit() avoids potential compile-time optimizations. > > >From what I see in the code, &sensors is not a compile-time constant, > thus most probably there are no object code changes before and after > the patch. But anyway, we shouldn't call internal wrappers instead of > the actual API, so this fix is correct. Thanks for this very comprehensive description, now I know :) > >> Fixes: 4da71a77fc3b ("ice: read internal temperature sensor") > >> Signed-off-by: Petr Oros <poros@redhat.com> > >> Acked-by: Ivan Vecera <ivecera@redhat.com> > > To be added to v2: > > Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com> > > Thanks, > Olek >
diff --git a/drivers/net/ethernet/intel/ice/ice_hwmon.c b/drivers/net/ethernet/intel/ice/ice_hwmon.c index e4c2c1bff6c086..b7aa6812510a4f 100644 --- a/drivers/net/ethernet/intel/ice/ice_hwmon.c +++ b/drivers/net/ethernet/intel/ice/ice_hwmon.c @@ -96,7 +96,7 @@ static bool ice_is_internal_reading_supported(struct ice_pf *pf) unsigned long sensors = pf->hw.dev_caps.supported_sensors; - return _test_bit(ICE_SENSOR_SUPPORT_E810_INT_TEMP_BIT, &sensors); + return test_bit(ICE_SENSOR_SUPPORT_E810_INT_TEMP_BIT, &sensors); }; void ice_hwmon_init(struct ice_pf *pf)
Do not use _test_bit() macro for testing bit. The proper macro for this is one without underline. Fixes: 4da71a77fc3b ("ice: read internal temperature sensor") Signed-off-by: Petr Oros <poros@redhat.com> Acked-by: Ivan Vecera <ivecera@redhat.com> --- Changes for v2: - added target tree --- drivers/net/ethernet/intel/ice/ice_hwmon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)