Re: [PATCH] staging: iio: frequency: rename macros

Tudor Gheorghiu
2024-10-02T02:49:19Z

On Wed, Oct 02, 2024 at 12:54:26AM +0200, Nam Cao wrote:
>
> You probably want to elaborate what you mean by "their naming choice" (i.e.
> how does the naming choice causes this false warning?)
>
> I got curious and digged into checkpatch.pl. This script expects macros
> whose names match "IIO_DEV_ATTR_[A-Z_]+" to have the first integer argument
> to be octal. And this driver defines macros which "luckily" match that
> pattern.
>
> There is only IIO_DEV_ATTR_SAMP_FREQ which matches the pattern, and accepts
> umode_t as its first argument.
>
> Instead of changing code just to make checkpatch.pl happy, perhaps it's
> better to fix the checkpatch script? Maybe something like the untested
> patch below?
>
> Or since checkpatch is wrong, maybe just ignore it.
>
> Best regards,
> Nam
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4427572b2477..2fb4549fede2 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -817,7 +817,7 @@ our @mode_permission_funcs = (
> ["debugfs_create_(?:file|u8|u16|u32|u64|x8|x16|x32|x64|size_t|atomic_t|bool|blob|regset32|u32_array)", 2],
> ["proc_create(?:_data|)", 2],
> ["(?:CLASS|DEVICE|SENSOR|SENSOR_DEVICE|IIO_DEVICE)_ATTR", 2],
> - ["IIO_DEV_ATTR_[A-Z_]+", 1],
> + ["IIO_DEV_ATTR_SAMP_FREQ", 1],
> ["SENSOR_(?:DEVICE_|)ATTR_2", 2],
> ["SENSOR_TEMPLATE(?:_2|)", 3],
> ["__ATTR", 2],

Hi!

Yes, this is exactly what I discovered while inspecting checkpatch
myself, however it did not occur to me this could be a problem with
checkpatch. But looking deeper, it seems like it is:

IIO_DEV_ATTR_SAMP_FREQ is defined in include/linux/iio/sysfs.h, along
with other helper macros:

> /**
> * IIO_DEV_ATTR_SAMP_FREQ - sets any internal clock frequency
> * @_mode: sysfs file mode/permissions
> * @_show: output method for the attribute
> * @_store: input method for the attribute
> **/
> #define IIO_DEV_ATTR_SAMP_FREQ(_mode, _show, _store) \
> IIO_DEVICE_ATTR(sampling_frequency, _mode, _show, _store, 0)
>
> /**
> * IIO_DEV_ATTR_SAMP_FREQ_AVAIL - list available sampling frequencies
> * @_show: output method for the attribute
> *
> * May be mode dependent on some devices
> **/
> #define IIO_DEV_ATTR_SAMP_FREQ_AVAIL(_show) \
> IIO_DEVICE_ATTR(sampling_frequency_available, S_IRUGO, _show, NULL, 0)
> /**
> * IIO_DEV_ATTR_INT_TIME_AVAIL - list available integration times
> * @_show: output method for the attribute
> **/
> #define IIO_DEV_ATTR_INT_TIME_AVAIL(_show) \
> IIO_DEVICE_ATTR(integration_time_available, S_IRUGO, _show, NULL, 0)
>
> #define IIO_DEV_ATTR_TEMP_RAW(_show) \
> IIO_DEVICE_ATTR(in_temp_raw, S_IRUGO, _show, NULL, 0)

The checkpatch script will match all these macros, even if
IIO_DEV_ATTR_SAMP_FREQ is the only one where we need to check for octal
literal arguments. I grep'd through the entire sourcecode, and the only
false positives with literal decimal arguments which match "IIO_DEV_ATTR_[A-Z_]+"
are inside this driver.

I accidentally discovered this issue by running
checkpatch on the said driver files.

I will submit a patch to the checkpatch maintainers with this thread
linked, and if they agree this is a bug and accept the patch,
this driver patch will no longer be needed, since checkpatch will no longer flag
these macros as false positives.

Do I have your permission to add your name and email to Suggested-by?

Thanks!
Tudor