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

[Tudor Gheorghiu]

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