| 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
| ||
|
Date: Tue, 22 May 2018 11:33:43 -0700
From: rishabhb@...eaurora.org
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
linux-arm-msm@...r.kernel.org,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-arm@...ts.infradead.org, tsoni@...eaurora.org,
ckadabi@...eaurora.org, evgreen@...omium.org,
Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver
On 2018-05-18 14:01, Andy Shevchenko wrote:
> On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
> <rishabhb@...eaurora.org> wrote:
>> LLCC (Last Level Cache Controller) provides additional cache memory
>> in the system. LLCC is partitioned into multiple slices and each
>> slice gets its own priority, size, ID and other config parameters.
>> LLCC driver programs these parameters for each slice. Clients that
>> are assigned to use LLCC need to get information such size & ID of the
>> slice they get and activate or deactivate the slice as needed. LLCC
>> driver
>> provides API for the clients to perform these operations.
>
>> +static const struct of_device_id sdm845_qcom_llcc_of_match[] = {
>> + { .compatible = "qcom,sdm845-llcc", },
>
>> + { },
>
> Slightly better w/o comma
Changed
>
>> +};
>
>> +static struct platform_driver sdm845_qcom_llcc_driver = {
>> + .driver = {
>> + .name = "sdm845-llcc",
>
>> + .owner = THIS_MODULE,
>
> No need. See below.
>
>> + .of_match_table = sdm845_qcom_llcc_of_match,
>> + },
>> + .probe = sdm845_qcom_llcc_probe,
>> +};
>
>> +
>> +static int __init sdm845_init_qcom_llcc_init(void)
>> +{
>> + return platform_driver_register(&sdm845_qcom_llcc_driver);
>> +}
>> +module_init(sdm845_init_qcom_llcc_init);
>> +
>> +static void __exit sdm845_exit_qcom_llcc_exit(void)
>> +{
>> + platform_driver_unregister(&sdm845_qcom_llcc_driver);
>> +}
>> +module_exit(sdm845_exit_qcom_llcc_exit);
>
> Why not to use module_platform_driver() macro?
Done
>
>> +#define ACTIVATE 0x1
>> +#define DEACTIVATE 0x2
>> +#define ACT_CTRL_OPCODE_ACTIVATE 0x1
>> +#define ACT_CTRL_OPCODE_DEACTIVATE 0x2
>> +#define ACT_CTRL_ACT_TRIG 0x1
>
> Are these bits? Perhaps BIT() ?
>
isn't it just better to use fixed size as u suggest in the next comment?
>> +#define ACT_CTRL_OPCODE_SHIFT 0x1
>> +#define ATTR1_PROBE_TARGET_WAYS_SHIFT 0x2
>> +#define ATTR1_FIXED_SIZE_SHIFT 0x3
>> +#define ATTR1_PRIORITY_SHIFT 0x4
>> +#define ATTR1_MAX_CAP_SHIFT 0x10
>
> Better to use fixed size pattern, i.e. 0x01, 0x02, 0x03, 0x04, 0x10.
>
>> +#define ATTR0_RES_WAYS_MASK 0x00000fff
>> +#define ATTR0_BONUS_WAYS_MASK 0x0fff0000
>
> GENMASK()
Done
>
>> +#define LLCC_LB_CNT_MASK 0xf0000000
>
> Ditto.
>
>> +#define MAX_CAP_TO_BYTES(n) (n * 1024)
>
> (n * SZ_1K) ?
Done
>
>> +#define LLCC_TRP_ACT_CTRLn(n) (n * 0x1000)
>
> SZ_4K ?
>
>> +#define LLCC_TRP_STATUSn(n) (4 + n * 0x1000)
>
> Ditto.
>
>> +struct llcc_slice_desc *llcc_slice_getd(u32 uid)
>> +{
>> + const struct llcc_slice_config *cfg;
>> + struct llcc_slice_desc *desc;
>> + u32 sz, count = 0;
>> +
>> + cfg = drv_data->cfg;
>> + sz = drv_data->cfg_size;
>> +
>
>> + while (cfg && count < sz) {
>> + if (cfg->usecase_id == uid)
>> + break;
>> + cfg++;
>> + count++;
>> + }
>> + if (cfg == NULL || count == sz)
>> + return ERR_PTR(-ENODEV);
>
> if (!cfg)
> return ERR_PTR(-ENODEV);
>
> while (cfg->... != uid) {
> cfg++;
> count++;
> }
>
> if (count == sz)
> return ...
>
> Though I would rather put it to for () loop.
>
In each while loop iteration the cfg pointer needs to be checked for
NULL. What if the usecase id never matches the uid passed by client
and we keep iterating. At some point it will crash.
>> +static int llcc_update_act_ctrl(u32 sid,
>> + u32 act_ctrl_reg_val, u32 status)
>> +{
>> + u32 act_ctrl_reg;
>> + u32 status_reg;
>> + u32 slice_status;
>
>> + int ret = 0;
>
> Useless assignment. Check entire patch series for a such.
Checked and removed these.
>
>> + ret = regmap_read_poll_timeout(drv_data->regmap, status_reg,
>> + slice_status, !(slice_status & status), 0,
>> LLCC_STATUS_READ_DELAY);
>
> Wrong indentation.
Corrected
>
>> + return ret;
>> +}
>
>> + ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
>> + DEACTIVATE);
>
> Perhaps one line (~83 characters here is OK) ?
The checkpatch script complains about such lines.
>
>> + ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
>> + ACTIVATE);
>
> Ditto.
>
>> + attr1_cfg = bcast_off +
>> +
>> LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);
>> + attr0_cfg = bcast_off +
>> +
>> LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);
>
> Ditto.
>
>> + attr1_val |= llcc_table[i].probe_target_ways <<
>> + ATTR1_PROBE_TARGET_WAYS_SHIFT;
>> + attr1_val |= llcc_table[i].fixed_size <<
>> + ATTR1_FIXED_SIZE_SHIFT;
>> + attr1_val |= llcc_table[i].priority <<
>> ATTR1_PRIORITY_SHIFT;
>
> foo |=
> bar << SHIFT;
>
> would look slightly better.
>
>> +int qcom_llcc_probe(struct platform_device *pdev,
>> + const struct llcc_slice_config *llcc_cfg, u32
>> sz)
>> +{
>
>> + drv_data->offsets = devm_kzalloc(dev, num_banks * sizeof(u32),
>> + GFP_KERNEL);
>> + if (!drv_data->offsets)
>> + return -ENOMEM;
>
> devm_kcalloc() ?
changed
>> +
>> + for (i = 0; i < num_banks; i++)
>> + drv_data->offsets[i] = (i * BANK_OFFSET_STRIDE);
>
> Pointless parens.
Removed
>
>> + drv_data->bitmap = devm_kcalloc(dev,
>> + BITS_TO_LONGS(drv_data->max_slices), sizeof(unsigned long),
>> + GFP_KERNEL);
>> + if (!drv_data->bitmap)
>> + return -ENOMEM;
>
> Perhaps at some point someone will add
> bitmap_alloc()
> devm_bitmap_alloc()
>
>> + bitmap_zero(drv_data->bitmap, drv_data->max_slices);
>
> Pointless
Removed
Powered by blists - more mailing lists