| 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: <51630B48.9050005@gmail.com>
Date: Mon, 08 Apr 2013 20:24:08 +0200
From: Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
CC: Grant Likely <grant.likely@...retlab.ca>,
Rob Herring <rob.herring@...xeda.com>,
Rob Landley <rob@...dley.net>,
Mike Turquette <mturquette@...aro.org>,
Stephen Warren <swarren@...dia.com>,
Thierry Reding <thierry.reding@...onic-design.de>,
Dom Cobley <popcornmix@...il.com>,
Linus Walleij <linus.walleij@...aro.org>,
Arnd Bergmann <arnd@...db.de>,
Andrew Morton <akpm@...ux-foundation.org>,
Pawel Moll <pawel.moll@....com>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
Russell King - ARM Linux <linux@....linux.org.uk>,
Rabeeh Khoury <rabeeh@...id-run.com>,
Daniel Mack <zonque@...il.com>,
Jean-Francois Moine <moinejf@...e.fr>,
Lars-Peter Clausen <lars@...afoo.de>,
devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v6] clk: add si5351 i2c common clock driver
On 04/08/2013 07:46 PM, Guenter Roeck wrote:
> On Mon, Apr 08, 2013 at 06:46:57PM +0200, Sebastian Hesselbarth wrote:
>> This patch adds a common clock driver for Silicon Labs Si5351a/b/c
>> i2c programmable clock generators. Currently, the driver does not
>> support VXCO feature of si5351b. Passing platform_data or DT bindings
>> selectively allow to overwrite stored Si5351 configuration which is
>> very helpful for clock generators with empty eeprom configuration.
>> Corresponding device tree binding documentation is also added.
>>
>> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@...il.com>
>
> [ ... ]
>
>> +
>> +/*
>> + * Si5351 i2c probe and DT
>> + */
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id si5351_dt_ids[] = {
>> + { .compatible = "silabs,si5351a", .data = (void *)SI5351_VARIANT_A, },
>> + { .compatible = "silabs,si5351a-msop",
>> + .data = (void *)SI5351_VARIANT_A3, },
>> + { .compatible = "silabs,si5351b", .data = (void *)SI5351_VARIANT_B, },
>> + { .compatible = "silabs,si5351c", .data = (void *)SI5351_VARIANT_C, },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, si5351_dt_ids);
>> +
>> +static int si5351_dt_parse(struct i2c_client *client)
>> +{
>> + struct device_node *child, *np = client->dev.of_node;
>> + struct si5351_platform_data *pdata;
>> + const struct of_device_id *match;
>> + struct property *prop;
>> + const __be32 *p;
>> + int num = 0;
>> + u32 val;
>> +
>> + if (np == NULL)
>> + return 0;
>> +
>> + match = of_match_node(si5351_dt_ids, np);
>> + if (match == NULL)
>> + return -EINVAL;
>> +
>> + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata)
>> + return -ENOMEM;
>> +
>> + pdata->variant = (enum si5351_variant)match->data;
>> + pdata->clk_xtal = of_clk_get(np, 0);
>> + if (!IS_ERR(pdata->clk_xtal))
>> + clk_put(pdata->clk_xtal);
>> + pdata->clk_clkin = of_clk_get(np, 1);
>> + if (!IS_ERR(pdata->clk_clkin))
>> + clk_put(pdata->clk_clkin);
>> +
>> + /*
>> + * property silabs,pll-source :<num src>, [<..>]
>> + * allow to selectively set pll source
>> + */
>> + of_property_for_each_u32(np, "silabs,pll-source", prop, p, num) {
>> + if (num>= 2) {
>> + dev_err(&client->dev,
>> + "invalid pll %d on pll-source prop\n", num);
>> + break;
>
> You report dev_err here, but you don't return an error to the caller.
> Is this on purpose ? If it is not a fatal error, maybe it should be dev_info ?
This shouldn't break but continue with one u32 skipped. Actually, it is
a warning because somebody passed an invalid value and should be
dev_warn(). But see my note below, I will make them all fatal.
>> + }
>> +
>> + p = of_prop_next_u32(prop, p,&val);
>> + if (!p)
>> + break;
>> +
>> + switch (val) {
>> + case 0:
>> + pdata->pll_src[num] = SI5351_PLL_SRC_XTAL;
>> + break;
>> + case 1:
>> + pdata->pll_src[num] = SI5351_PLL_SRC_CLKIN;
>> + break;
>> + default:
>> + dev_warn(&client->dev,
>> + "invalid parent %d for pll %d\n", val, num);
>> + continue;
>
> Same here, and a couple of times below. Given the context, I think it would
> be better if the error cases would return an error. After all, the condition
> suggests that the device tree data is wrong, meaning one has to assume it
> won't work anyway and should be fixed in the device tree and not be ignored
> by the driver.
I am skipping invalid DT data enties here, but I can make them all
fatal. I will also add more variant checks in the corresponding
_si5351_* functions.
>> + }
>> + }
>> +
>> + /* per clkout properties */
>> + num = of_get_child_count(np);
>> +
>> + if (num == 0)
>> + return 0;
>> +
>
> This doesn't set client->dev.platform_data yet returns no error, meaning the
> calling code will either use provided platform data or fail after all if it is
> NULL. That seems to be inconsistent, given that a dt entry was already detected.
> The user might end up wondering why the provided device tree data is not used,
> not realizing that it is incomplete.
>
> If children are not mandatory, you could just drop the code above and go through
> for_each_child_of_node() anyway; it won't do anything and set
> client->dev.platform_data at the end. If children are mandatory, it might make
> sense to return an eror here (if there is dt information, it should be complete
> and consistent).
Not having children is valid as you only provide them if you want to
overwrite the current config stored in the eeprom. But yes, skipping
for_each_child_of_node below is a left-over from an implementation where
I used dynamically allocated ->pll/->clkout.
>> + for_each_child_of_node(np, child) {
>> + if (of_property_read_u32(child, "reg",&num)) {
>> + dev_err(&client->dev, "missing reg property of %s\n",
>> + child->name);
>> + continue;
>> + }
>> +
> What if "num" returns 100 ? Or 1000 ?
Segmentation fault? ;) I will add a check for 0 <= num < 8.
Thanks for the review,
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists