| 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
| ||
|
Message-ID: <26ef74e0-67ff-77c4-1682-8f8261b71463@gmail.com>
Date: Thu, 18 Mar 2021 19:18:58 +0300
From: Dmitry Osipenko <digetx@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>
Cc: linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v1] memory: tegra20: Add debug statistics
18.03.2021 18:23, Krzysztof Kozlowski пишет:
...
>> + mc->debugfs.root = debugfs_create_dir("mc", NULL);
>> + if (!mc->debugfs.root)
>> + dev_err(&pdev->dev, "failed to create debugfs directory\n");
>
> It's error pointer, not null, but anyway there is no need for handling
> debugfs error. See Greg KH's commits like "remove pointless check for
> debugfs_create_dir()".
Indeed!
>> +
>> + if (mc->soc->init) {
>> + err = mc->soc->init(mc);
>> + if (err < 0)
>> + dev_err(&pdev->dev,
>> + "failed to register initialize SoC driver: %d\n",
>
> "failed to initialize SoC driver:...."
Good catch!
>> + err);
>> + }
>> +
>> err = tegra_mc_reset_setup(mc);
>> if (err < 0)
>> dev_err(&pdev->dev, "failed to register reset controller: %d\n",
>> diff --git a/drivers/memory/tegra/tegra20.c b/drivers/memory/tegra/tegra20.c
>> index 29ecf02805a0..513c07104296 100644
>> --- a/drivers/memory/tegra/tegra20.c
>> +++ b/drivers/memory/tegra/tegra20.c
>> @@ -3,6 +3,8 @@
>> * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
>> */
>>
>> +#include <linux/bitfield.h>
>> +#include <linux/delay.h>
>> #include <linux/of_device.h>
>> #include <linux/slab.h>
>> #include <linux/string.h>
>> @@ -11,6 +13,77 @@
>>
>> #include "mc.h"
>>
>> +#define MC_STAT_CONTROL 0x90
>> +#define MC_STAT_EMC_CLOCK_LIMIT 0xa0
>> +#define MC_STAT_EMC_CLOCKS 0xa4
>> +#define MC_STAT_EMC_CONTROL_0 0xa8
>> +#define MC_STAT_EMC_CONTROL_1 0xac
>> +#define MC_STAT_EMC_COUNT_0 0xb8
>> +#define MC_STAT_EMC_COUNT_1 0xbc
>> +
>> +#define MC_STAT_CONTROL_CLIENT_ID GENMASK(13, 8)
>> +#define MC_STAT_CONTROL_EVENT GENMASK(23, 16)
>> +#define MC_STAT_CONTROL_PRI_EVENT GENMASK(25, 24)
>> +#define MC_STAT_CONTROL_FILTER_CLIENT_ENABLE GENMASK(26, 26)
>> +#define MC_STAT_CONTROL_FILTER_PRI GENMASK(29, 28)
>> +
>> +#define MC_STAT_CONTROL_PRI_EVENT_HP 0
>> +#define MC_STAT_CONTROL_PRI_EVENT_TM 1
>> +#define MC_STAT_CONTROL_PRI_EVENT_BW 2
>> +
>> +#define MC_STAT_CONTROL_FILTER_PRI_DISABLE 0
>> +#define MC_STAT_CONTROL_FILTER_PRI_NO 1
>> +#define MC_STAT_CONTROL_FILTER_PRI_YES 2
>> +
>> +#define MC_STAT_CONTROL_EVENT_QUALIFIED 0
>> +#define MC_STAT_CONTROL_EVENT_ANY_READ 1
>> +#define MC_STAT_CONTROL_EVENT_ANY_WRITE 2
>> +#define MC_STAT_CONTROL_EVENT_RD_WR_CHANGE 3
>> +#define MC_STAT_CONTROL_EVENT_SUCCESSIVE 4
>> +#define MC_STAT_CONTROL_EVENT_ARB_BANK_AA 5
>> +#define MC_STAT_CONTROL_EVENT_ARB_BANK_BB 6
>> +#define MC_STAT_CONTROL_EVENT_PAGE_MISS 7
>> +#define MC_STAT_CONTROL_EVENT_AUTO_PRECHARGE 8
>> +
>> +#define EMC_GATHER_RST (0 << 8)
>> +#define EMC_GATHER_CLEAR (1 << 8)
>> +#define EMC_GATHER_DISABLE (2 << 8)
>> +#define EMC_GATHER_ENABLE (3 << 8)
>> +
>> +#define MC_STAT_SAMPLE_TIME_USEC 16000
>> +
>> +/* we store collected statistics as a fixed point values */
>> +#define MC_FX_FRAC_SCALE 100
>> +
>> +struct tegra20_mc_stat_gather {
>> + unsigned int pri_filter;
>> + unsigned int pri_event;
>> + unsigned int result;
>> + unsigned int client;
>> + unsigned int event;
>> + bool client_enb;
>> +};
>> +
>> +struct tegra20_mc_stat {
>> + struct tegra20_mc_stat_gather gather0;
>> + struct tegra20_mc_stat_gather gather1;
>> + unsigned int sample_time_usec;
>> + struct tegra_mc *mc;
>> +};
>> +
>> +struct tegra20_mc_client_stat {
>> + unsigned int events;
>> + unsigned int arb_high_prio;
>> + unsigned int arb_timeout;
>> + unsigned int arb_bandwidth;
>> + unsigned int rd_wr_change;
>> + unsigned int successive;
>> + unsigned int page_miss;
>> + unsigned int auto_precharge;
>> + unsigned int arb_bank_aa;
>> + unsigned int arb_bank_bb;
>> +};
>> +
>> static const struct tegra_mc_client tegra20_mc_clients[] = {
>> {
>> .id = 0x00,
>> @@ -356,6 +429,274 @@ static const struct tegra_mc_icc_ops tegra20_mc_icc_ops = {
>> .set = tegra20_mc_icc_set,
>> };
>>
>> +static u32 tegra20_mc_stat_gather_control(struct tegra20_mc_stat_gather *g)
>
> "g" looks like pointer to const here
I'll add couple more consts to the code for consistency.
>> +{
>> + u32 control;
>> +
>> + control = FIELD_PREP(MC_STAT_CONTROL_EVENT, g->event);
>> + control |= FIELD_PREP(MC_STAT_CONTROL_CLIENT_ID, g->client);
>> + control |= FIELD_PREP(MC_STAT_CONTROL_PRI_EVENT, g->pri_event);
>> + control |= FIELD_PREP(MC_STAT_CONTROL_FILTER_PRI, g->pri_filter);
>> + control |= FIELD_PREP(MC_STAT_CONTROL_FILTER_CLIENT_ENABLE, g->client_enb);
>> +
>> + return control;
>> +}
...
>> +static void tegra20_mc_collect_stats(struct tegra_mc *mc,
>> + struct tegra20_mc_client_stat *stats)
>> +{
>> + const struct tegra_mc_client *client0, *client1;
>> + unsigned int i, clienta, clientb;
>
> Define the clienta and clientb inside the loop, to reduce the scope.
> Unless it is used between the loop iterations?
okay
>> +
>> + /* collect memory controller utilization percent for each client */
>> + for (i = 0; i < mc->soc->num_clients; i += 2) {
>> + client0 = &mc->soc->clients[i];
>> + client1 = &mc->soc->clients[i + 1];
>> +
>> + if (i + 1 == mc->soc->num_clients)
>> + client1 = NULL;
>> +
>> + tegra20_mc_stat_events(mc, client0, client1,
>> + MC_STAT_CONTROL_FILTER_PRI_DISABLE,
>> + MC_STAT_CONTROL_PRI_EVENT_HP,
>> + MC_STAT_CONTROL_EVENT_QUALIFIED,
>> + &stats[i + 0].events,
>> + &stats[i + 1].events);
>> + }
>> +
>> + /* collect more info from active clients */
>> + for (i = 0; i < mc->soc->num_clients; i++) {
>> + clientb = mc->soc->num_clients;
>> +
>> + for (client0 = NULL; i < mc->soc->num_clients; i++) {
>> + if (stats[i].events) {
>> + client0 = &mc->soc->clients[i];
>> + clienta = i++;
>> + break;
>> + }
>> + }
>
> Could all clients have 0 events ending with "clienta" being undefined?
> "client0" would be non-NULL because of loop before.
As per 6.8.5.3 of C99 standard, the declaration of a for-loop "is
reached in the order of execution before the first evaluation of the
controlling expression".
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
>> +
>> + for (client1 = NULL; i < mc->soc->num_clients; i++) {
>> + if (stats[i].events) {
>> + client1 = &mc->soc->clients[i];
>> + clientb = i;
>> + break;
>> + }
>> + }
>> +
>> + if (!client0 && !client1)
>> + break;
this means that both client0 and client1 are set t0 NULL here if all
clients have 0 events.
Powered by blists - more mailing lists