linux-kernel - Re: [PATCH v2 1/2] nvme: add tracepoint for nvme_setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 Jan 2018 17:49:54 +0100
From:   Christoph Hellwig <hch@....de>
To:     Johannes Thumshirn <jthumshirn@...e.de>
Cc:     Christoph Hellwig <hch@....de>, Sagi Grimberg <sagi@...mberg.me>,
        Keith Busch <keith.busch@...el.com>,
        Linux Kernel Mailinglist <linux-kernel@...r.kernel.org>,
        Hannes Reinecke <hare@...e.de>,
        Linux NVMe Mailinglist <linux-nvme@...ts.infradead.org>
Subject: Re: [PATCH v2 1/2] nvme: add tracepoint for nvme_setup_cmd

> +	trace_seq_printf(p, "slba=%llu, length=%u, control=0x%x, dsmgmt=%u, reftag=%u",

Overly long line.

> +			 (unsigned long long)le64_to_cpu(rw.slba),

u64 now always is an unsigned long long, no need for the cast.

> +			 le16_to_cpu(rw.length), le16_to_cpu(rw.control),
> +			 le32_to_cpu(rw.dsmgmt), le32_to_cpu(rw.reftag));
> +	trace_seq_putc(p, 0);

This look weird, is there a a good reason for putting a 0 character?

Also I'd be tempted to add a new trace.c file for these and make them
dependent on whatever kernel option enables the tracing.

> --- /dev/null
> +++ b/drivers/nvme/host/trace.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Please add a real copyright header including the author.

const char *nvme_trace_parse_cmd(struct trace_seq *p, struct nvme_command cmnd);

> +#define __parse_nvme_cmd(cmd) nvme_trace_parse_cmd(p, cmd)

This looks weird.

> +TRACE_EVENT(nvme_setup_cmd,
> +
> +	    TP_PROTO(struct nvme_command *cmd),
> +
> +	    TP_ARGS(cmd),
> +

Please remove these empty lines.

> +	    TP_STRUCT__entry(
> +		    __field( __u8,                opcode   )
> +		    __field( __u8,                flags    )
> +		    __field( __u16,               cid      )
> +		    __field( __le32,              nsid     )
> +		    __field( __le64,              metadata )

> +		    __field_struct( struct nvme_command, cmnd )
And the additional whitespaces before and after the braces

> +	    TP_fast_assign(
> +		    __entry->opcode	= cmd->common.opcode;
> +		    __entry->flags	= cmd->common.flags;
> +		    __entry->cid	= cmd->common.command_id;
> +		    __entry->nsid	= cmd->common.nsid;
> +		    __entry->metadata	= cmd->common.metadata;
> +		    memcpy(&__entry->cmnd, cmd, sizeof(__entry->cmnd));

Copying the whole SQE in tracing seems rather excessive.

> +	    TP_printk("nsid=%u, command_id=%u, flags=0x%x, metadata=0x%llx, cmd=(%s %s)",

Too long line.

> +		      le32_to_cpu(__entry->nsid), __entry->cid, __entry->flags,
> +		      (unsigned long long) le64_to_cpu(__entry->metadata),

no need for the cast.

> +#undef TRACE_INCLUDE_PATH
> +#define TRACE_INCLUDE_PATH ../../drivers/nvme/host

This seems wrong.  Instead you should make sure we include the srcdir
in the makefile, take a look at fs/xfs/Makefile for example.