| Message ID | 20241011115334.367736-1-0xff07@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] iio: Fix uninitialized variable | expand |
On 11/10/2024 13:52, Yo-Jung (Leo) Lin wrote: > clang found that the "offset" in bmp580_trigger_handler doesn't get > initialized before access. Add proper initialization to this variable. > > Signed-off-by: Yo-Jung (Leo) Lin <0xff07@gmail.com> > --- > Change in v2: > - Make value initialization immediate before its first use. > - Link to v1: https://lore.kernel.org/all/20241011093752.30685-1-0xff07@gmail.com/ > > --- > drivers/iio/pressure/bmp280-core.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > index f4df222ed0c3..682329f81886 100644 > --- a/drivers/iio/pressure/bmp280-core.c > +++ b/drivers/iio/pressure/bmp280-core.c > @@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p) > goto out; > } > > + offset = 0; > + > /* Pressure calculations */ > memcpy(&data->sensor_data[offset], &data->buf[3], 3); > That was a quick reply. I would recommend you to wait a little bit while the first version is under discussion. I still see the offset thing a bit weird. data->sensor_data uses an offset to avoid hard-coded numbers, but for data->buf we do exactly that, in the very same lines. Setting offset to 0 to access the first element i.e. no offset required, and then adding the actual offset sizeof(s32), which could even be a const if the first access was to sensor_data[0], looks to verbose. These things are of course not critical, and the proposed fix is definitely ok, but I am missing some consistency here.
Hi Javier, On Fri, Oct 11, 2024 at 8:31 PM Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > > On 11/10/2024 13:52, Yo-Jung (Leo) Lin wrote: > > clang found that the "offset" in bmp580_trigger_handler doesn't get > > initialized before access. Add proper initialization to this variable. > > > > Signed-off-by: Yo-Jung (Leo) Lin <0xff07@gmail.com> > > --- > > Change in v2: > > - Make value initialization immediate before its first use. > > - Link to v1: https://lore.kernel.org/all/20241011093752.30685-1-0xff07@gmail.com/ > > > > --- > > drivers/iio/pressure/bmp280-core.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > > index f4df222ed0c3..682329f81886 100644 > > --- a/drivers/iio/pressure/bmp280-core.c > > +++ b/drivers/iio/pressure/bmp280-core.c > > @@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p) > > goto out; > > } > > > > + offset = 0; > > + > > /* Pressure calculations */ > > memcpy(&data->sensor_data[offset], &data->buf[3], 3); > > > > That was a quick reply. I would recommend you to wait a little bit while > the first version is under discussion. It was my bad not waiting for enough feedback to finalize another patch. I’ll definitely do better next time. I feel that initializing it as sizeof(s32) in the beginning and not using it until the second memcpy() only widens the gap between value initialization and its first access, which doesn’t address Shevchenko’s concern. > > I still see the offset thing a bit weird. data->sensor_data uses an > offset to avoid hard-coded numbers, but for data->buf we do exactly > that, in the very same lines. > > Setting offset to 0 to access the first element i.e. no offset required, > and then adding the actual offset sizeof(s32), which could even be a > const if the first access was to sensor_data[0], looks to verbose. While that is also true, it’ll take a more general refactoring to make it driver-wise consistent. Maybe that refactoring should be on top of a fix, instead of being part of a fix. > > These things are of course not critical, and the proposed fix is > definitely ok, but I am missing some consistency here. Best, Leo
On Fri, Oct 11, 2024 at 02:31:00PM +0200, Javier Carrasco wrote: > On 11/10/2024 13:52, Yo-Jung (Leo) Lin wrote: > > clang found that the "offset" in bmp580_trigger_handler doesn't get > > initialized before access. Add proper initialization to this variable. > > > > Signed-off-by: Yo-Jung (Leo) Lin <0xff07@gmail.com> > > --- > > Change in v2: > > - Make value initialization immediate before its first use. > > - Link to v1: https://lore.kernel.org/all/20241011093752.30685-1-0xff07@gmail.com/ > > > > --- > > drivers/iio/pressure/bmp280-core.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c > > index f4df222ed0c3..682329f81886 100644 > > --- a/drivers/iio/pressure/bmp280-core.c > > +++ b/drivers/iio/pressure/bmp280-core.c > > @@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p) > > goto out; > > } > > > > + offset = 0; > > + > > /* Pressure calculations */ > > memcpy(&data->sensor_data[offset], &data->buf[3], 3); > > > > That was a quick reply. I would recommend you to wait a little bit while > the first version is under discussion. > > I still see the offset thing a bit weird. data->sensor_data uses an > offset to avoid hard-coded numbers, but for data->buf we do exactly > that, in the very same lines. > > Setting offset to 0 to access the first element i.e. no offset required, > and then adding the actual offset sizeof(s32), which could even be a > const if the first access was to sensor_data[0], looks to verbose. > > These things are of course not critical, and the proposed fix is > definitely ok, but I am missing some consistency here. Hi everyone! So if you check also the conversations that we had here [1] and in the previous versions, indeed the idea behind the offset is to use it as an self-explanatory index to a char buffer that holds in fact s32 variables. The data->buf here holds the values that have just been read from the sensor. If you check on the channel specification of this sensor, you will see ".realbits = 24" in both values that the sensor returns so hence the value 3. I am not sure if it makes sense to use a macro here for each one of the 3's that are going to be used only one time each and in order to be more "consistent". But I might have a wrong view on this one so feel free to correct me! For the initialization of the offset indeed, it was already mentioned here [2] this morning, but on a different patch!!! I couldn't get this error though with gcc... Cheers, Vasilis [1]: https://lore.kernel.org/all/20240930202353.38203-3-vassilisamir@gmail.com/ [2]: https://lore.kernel.org/linux-iio/202410111221.YIeXHxOv-lkp@intel.com/
... > > Hi everyone! > > So if you check also the conversations that we had here [1] and in the > previous versions, indeed the idea behind the offset is to use it as an > self-explanatory index to a char buffer that holds in fact s32 variables. > > The data->buf here holds the values that have just been read from the > sensor. If you check on the channel specification of this sensor, > you will see ".realbits = 24" in both values that the sensor returns so > hence the value 3. > So you are using 3 = 24 bits, but s32 not as 4 bytes? the whole thing would have turned into sensor_data[0], sensor_data[4], and no variables implied, correct? But I am discussing too much for something that in the end is more or less the same, I am fine with this proposal. > I am not sure if it makes sense to use a macro here for each one of the > 3's that are going to be used only one time each and in order to be more > "consistent". But I might have a wrong view on this one so feel free to > correct me! > > For the initialization of the offset indeed, it was already mentioned > here [2] this morning, but on a different patch!!! I couldn't get this > error though with gcc... > > Cheers, > Vasilis > At least for the things I do in the kernel, clang catches more issues than gcc. Sometimes even gcc will not complain, and clang will fail to compile (e.g. a goto before a cleanup attribute). And if you run smatch before sending the series, you might discover a couple of extra "surprises". Best regards, Javier Carrasco
On Fri, 11 Oct 2024 21:32:38 +0200 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > ... > > > > > Hi everyone! > > > > So if you check also the conversations that we had here [1] and in the > > previous versions, indeed the idea behind the offset is to use it as an > > self-explanatory index to a char buffer that holds in fact s32 variables. > > > > The data->buf here holds the values that have just been read from the > > sensor. If you check on the channel specification of this sensor, > > you will see ".realbits = 24" in both values that the sensor returns so > > hence the value 3. > > > > So you are using 3 = 24 bits, but s32 not as 4 bytes? the whole thing > would have turned into sensor_data[0], sensor_data[4], and no variables > implied, correct? But I am discussing too much for something that in the > end is more or less the same, I am fine with this proposal. Applied the fix. Thanks, Jonathan > > > I am not sure if it makes sense to use a macro here for each one of the > > 3's that are going to be used only one time each and in order to be more > > "consistent". But I might have a wrong view on this one so feel free to > > correct me! > > > > For the initialization of the offset indeed, it was already mentioned > > here [2] this morning, but on a different patch!!! I couldn't get this > > error though with gcc... > > > > Cheers, > > Vasilis > > > > At least for the things I do in the kernel, clang catches more issues > than gcc. Sometimes even gcc will not complain, and clang will fail to > compile (e.g. a goto before a cleanup attribute). > > And if you run smatch before sending the series, you might discover a > couple of extra "surprises". > > Best regards, > Javier Carrasco
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c index f4df222ed0c3..682329f81886 100644 --- a/drivers/iio/pressure/bmp280-core.c +++ b/drivers/iio/pressure/bmp280-core.c @@ -2222,6 +2222,8 @@ static irqreturn_t bmp580_trigger_handler(int irq, void *p) goto out; } + offset = 0; + /* Pressure calculations */ memcpy(&data->sensor_data[offset], &data->buf[3], 3);
clang found that the "offset" in bmp580_trigger_handler doesn't get initialized before access. Add proper initialization to this variable. Signed-off-by: Yo-Jung (Leo) Lin <0xff07@gmail.com> --- Change in v2: - Make value initialization immediate before its first use. - Link to v1: https://lore.kernel.org/all/20241011093752.30685-1-0xff07@gmail.com/ --- drivers/iio/pressure/bmp280-core.c | 2 ++ 1 file changed, 2 insertions(+)