[PATCH 1/2] HID: intel-ish-hid: add printf attribute to print_log()

[PATCH 1/2] HID: intel-ish-hid: add printf attribute to print_log()

[Nicolas Iooss]

Structure ishtp_device contains a logging function, print_log(), which
formats some of its parameters using vsnprintf(). Add a __printf
attribute to this function field (and to ish_event_tracer()) in order to
detect at compile time issues related to the printf-like formatting.

While at it, make format parameter a const pointer as print_log() is not
supposed to modify it.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c     | 3 ++-
 drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index 20d647d2dd2c..34c95de6885e 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -47,7 +47,8 @@ MODULE_DEVICE_TABLE(pci, ish_pci_tbl);
  *
  * Callback to direct log messages to Linux trace buffers
  */
-static void ish_event_tracer(struct ishtp_device *dev, char *format, ...)
+static __printf(2, 3)
+void ish_event_tracer(struct ishtp_device *dev, const char *format, ...)
 {
 	if (trace_ishtp_dump_enabled()) {
 		va_list args;
diff --git a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
index a94f9a8a96a0..6a6d927b78b0 100644
--- a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
+++ b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
@@ -238,7 +238,8 @@ struct ishtp_device {
 	uint64_t ishtp_host_dma_rx_buf_phys;
 
 	/* Dump to trace buffers if enabled*/
-	void (*print_log)(struct ishtp_device *dev, char *format, ...);
+	__printf(2, 3) void (*print_log)(struct ishtp_device *dev,
+					 const char *format, ...);
 
 	/* Debug stats */
 	unsigned int	ipc_rx_cnt;
-- 
2.11.0

[PATCH 2/2] HID: intel-ish-hid: format 32-bit integers with %X

[Nicolas Iooss]

In ishtp_hid_probe(), use %04X instead of %04hX to format __u32 values,
in order to silent a format error reported by clang:

    drivers/hid/intel-ish-hid/ishtp-hid.c:212:3: error: format specifies
    type 'unsigned short' but the argument has type '__u32' (aka
    'unsigned int') [-Werror,-Wformat]
                    hid->vendor, hid->product);
                    ^~~~~~~~~~~
    drivers/hid/intel-ish-hid/ishtp-hid.c:212:16: error: format
    specifies type 'unsigned short' but the argument has type '__u32'
    (aka 'unsigned int') [-Werror,-Wformat]
                    hid->vendor, hid->product);
                                 ^~~~~~~~~~~~

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 drivers/hid/intel-ish-hid/ishtp-hid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.c b/drivers/hid/intel-ish-hid/ishtp-hid.c
index 277983aa1d90..cd23903ddcf1 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid.c
@@ -208,7 +208,7 @@ int ishtp_hid_probe(unsigned int cur_hid_dev,
 	hid->version = le16_to_cpu(ISH_HID_VERSION);
 	hid->vendor = le16_to_cpu(ISH_HID_VENDOR);
 	hid->product = le16_to_cpu(ISH_HID_PRODUCT);
-	snprintf(hid->name, sizeof(hid->name), "%s %04hX:%04hX", "hid-ishtp",
+	snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X", "hid-ishtp",
 		hid->vendor, hid->product);
 
 	rv = hid_add_device(hid);
-- 
2.11.0

Re: [PATCH 2/2] HID: intel-ish-hid: format 32-bit integers with %X

[Srinivas Pandruvada]

On Thu, 2016-12-22 at 11:09 +0100, Nicolas Iooss wrote:
> In ishtp_hid_probe(), use %04X instead of %04hX to format __u32
> values,
> in order to silent a format error reported by clang:
> 
>     drivers/hid/intel-ish-hid/ishtp-hid.c:212:3: error: format
> specifies
>     type 'unsigned short' but the argument has type '__u32' (aka
>     'unsigned int') [-Werror,-Wformat]
>                     hid->vendor, hid->product);
>                     ^~~~~~~~~~~
>     drivers/hid/intel-ish-hid/ishtp-hid.c:212:16: error: format
>     specifies type 'unsigned short' but the argument has type '__u32'
>     (aka 'unsigned int') [-Werror,-Wformat]
>                     hid->vendor, hid->product);
>                                  ^~~~~~~~~~~~
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/hid/intel-ish-hid/ishtp-hid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.c
> b/drivers/hid/intel-ish-hid/ishtp-hid.c
> index 277983aa1d90..cd23903ddcf1 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid.c
> @@ -208,7 +208,7 @@ int ishtp_hid_probe(unsigned int cur_hid_dev,
>  	hid->version = le16_to_cpu(ISH_HID_VERSION);
>  	hid->vendor = le16_to_cpu(ISH_HID_VENDOR);
>  	hid->product = le16_to_cpu(ISH_HID_PRODUCT);
> -	snprintf(hid->name, sizeof(hid->name), "%s %04hX:%04hX",
> "hid-ishtp",
> +	snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X", "hid-
> ishtp",
>  		hid->vendor, hid->product);
>  
>  	rv = hid_add_device(hid);

Re: [PATCH 1/2] HID: intel-ish-hid: add printf attribute to print_log()

[Srinivas Pandruvada]

On Thu, 2016-12-22 at 11:09 +0100, Nicolas Iooss wrote:
> Structure ishtp_device contains a logging function, print_log(),
> which
> formats some of its parameters using vsnprintf(). Add a __printf
> attribute to this function field (and to ish_event_tracer()) in order
> to
> detect at compile time issues related to the printf-like formatting.
> 
> While at it, make format parameter a const pointer as print_log() is
> not
> supposed to modify it.
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/hid/intel-ish-hid/ipc/pci-ish.c     | 3 ++-
>  drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index 20d647d2dd2c..34c95de6885e 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -47,7 +47,8 @@ MODULE_DEVICE_TABLE(pci, ish_pci_tbl);
>   *
>   * Callback to direct log messages to Linux trace buffers
>   */
> -static void ish_event_tracer(struct ishtp_device *dev, char *format,
> ...)
> +static __printf(2, 3)
> +void ish_event_tracer(struct ishtp_device *dev, const char *format,
> ...)
>  {
>  	if (trace_ishtp_dump_enabled()) {
>  		va_list args;
> diff --git a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> index a94f9a8a96a0..6a6d927b78b0 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> +++ b/drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h
> @@ -238,7 +238,8 @@ struct ishtp_device {
>  	uint64_t ishtp_host_dma_rx_buf_phys;
>  
>  	/* Dump to trace buffers if enabled*/
> -	void (*print_log)(struct ishtp_device *dev, char *format,
> ...);
> +	__printf(2, 3) void (*print_log)(struct ishtp_device *dev,
> +					 const char *format, ...);
>  
>  	/* Debug stats */
>  	unsigned int	ipc_rx_cnt;

Re: [PATCH 1/2] HID: intel-ish-hid: add printf attribute to print_log()

[Jiri Kosina]

On Thu, 22 Dec 2016, Nicolas Iooss wrote:

> Structure ishtp_device contains a logging function, print_log(), which
> formats some of its parameters using vsnprintf(). Add a __printf
> attribute to this function field (and to ish_event_tracer()) in order to
> detect at compile time issues related to the printf-like formatting.
> 
> While at it, make format parameter a const pointer as print_log() is not
> supposed to modify it.

Both patches applied to for-4.11/intel-ish. Thanks,

-- 
Jiri Kosina
SUSE Labs