From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966313AbcAZP1Y (ORCPT ); Tue, 26 Jan 2016 10:27:24 -0500 Received: from mga09.intel.com ([134.134.136.24]:65525 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966058AbcAZP1V (ORCPT ); Tue, 26 Jan 2016 10:27:21 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,350,1449561600"; d="scan'208";a="641666244" From: Alexander Shishkin To: Mathieu Poirier , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Cc: linux-doc@vger.kernel.org, zhang.chunyan@linaro.org, mike.leach@arm.com, tor@ti.com, al.grant@arm.com, rabin@rab.in, Mathieu Poirier Subject: Re: [PATCH V8 18/23] coresight: etm-perf: new PMU driver for ETM tracers In-Reply-To: <1452807977-8069-19-git-send-email-mathieu.poirier@linaro.org> References: <1452807977-8069-1-git-send-email-mathieu.poirier@linaro.org> <1452807977-8069-19-git-send-email-mathieu.poirier@linaro.org> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Tue, 26 Jan 2016 17:27:16 +0200 Message-ID: <878u3c744r.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Mathieu Poirier writes: > +static int etm_event_init(struct perf_event *event) > +{ > + if (event->attr.type != etm_pmu.type) > + return -ENOENT; > + > + if (event->cpu >= nr_cpu_ids) > + return -EINVAL; perf_event_alloc() already does this. Except for this one doesn't cover the negative space. [snip] > +static void etm_free_aux(void *data) > +{ > + struct etm_event_data *event_data = data; > + > + pr_err("Queing work\n"); Probably not pr_err(). > + schedule_work(&event_data->work); > +} [snip] > +static void etm_event_start(struct perf_event *event, int flags) > +{ > + int cpu = smp_processor_id(); > + struct etm_event_data *event_data; > + struct perf_output_handle *handle = this_cpu_ptr(&ctx_handle); > + struct coresight_device *sink, *csdev = per_cpu(csdev_src, cpu); > + > + if (!csdev) > + goto fail; > + > + /* > + * Deal with the ring buffer API and get a handle on the > + * session's information. > + */ > + event_data = perf_aux_output_begin(handle, event); > + if (WARN_ON_ONCE(!event_data)) > + goto fail; There really shouldn't be a warning here. I understand that the 'no buffer' case is taped over by the !csdev check above, but there are other ligitimate reasons for perf_aux_output_begin() to return NULL, like no-space-left. > + > + /* We need a sink, no need to continue without one */ > + sink = coresight_get_sink(event_data->path[cpu]); > + if (!sink || !sink_ops(sink)->set_buffer) > + goto fail_end_stop; Is this possible after the coresight_build_path() things in setup_aux? Might be a better candidate for WARN_*ONCE(). > + > + /* Configure the sink */ > + if (sink_ops(sink)->set_buffer(sink, handle, > + event_data->snk_config)) > + goto fail_end_stop; > + > + /* Nothing will happen without a path */ > + if (coresight_enable_path(event_data->path[cpu], CS_MODE_PERF)) > + goto fail_end_stop; I'd like to understand all the potential failures here, because it's really a good idea to keep those to a minimum for the sake of consistency. That is, if the user succeeded in creating an event, about the only good reason for the event not starting is a filled up buffer. This is why it makes a lot of sense to keep all the coresight_build_path()/coresight_enable_path() to the .event_init() phase and let them fail early, if they should fail. Regards, -- Alex