linux-kernel - Re: [PATCH v2 2/2] watchdog: Add new arm_smc

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 for Android: free password hash cracker in your pocket

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

Message-ID: <e382a9fe-9909-5d07-4b71-efe80cb35e4c@roeck-us.net>
Date:   Tue, 21 Apr 2020 20:23:22 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Julius Werner <jwerner@...omium.org>,
        Evan Benn <evanbenn@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Xingyu Chen <xingyu.chen@...ogic.com>,
        Anson Huang <Anson.Huang@....com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Catalin Marinas <catalin.marinas@....com>,
        "David S. Miller" <davem@...emloft.net>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Leonard Crestez <leonard.crestez@....com>,
        Li Yang <leoyang.li@....com>,
        Marcin Juszkiewicz <marcin.juszkiewicz@...aro.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Olof Johansson <olof@...om.net>, Rob Herring <robh@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Valentin Schneider <valentin.schneider@....com>,
        Will Deacon <will@...nel.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        LINUX-WATCHDOG <linux-watchdog@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] watchdog: Add new arm_smc_wdt watchdog driver

On 4/21/20 1:31 PM, Julius Werner wrote:
>> +static int smcwd_call(unsigned long smc_func_id, enum smcwd_call call,
>> +                     unsigned long arg, struct arm_smccc_res *res)
> 
> I think you should just take a struct watchdog_device* here and do the
> drvdata unpacking inside the function.
> 
>> +static int smcwd_probe(struct platform_device *pdev)
>> +{
>> +       struct watchdog_device *wdd;
>> +       int err;
>> +       struct arm_smccc_res res;
>> +       u32 *smc_func_id;
>> +
>> +       smc_func_id =
>> +               devm_kzalloc(&pdev->dev, sizeof(*smc_func_id), GFP_KERNEL);
>> +       if (!smc_func_id)
>> +               return -ENOMEM;
> 
> nit: Could save the allocation by just casting the value itself to a
> pointer? Or is that considered too hacky?
> 

Actually, the current code is what is hacky. I'd either do
what you suggest, or allocate a structure such as

struct local_data {
	u32 smc_func_id;
	struct watchdog_device wdd;
};

and use it accordingly.

Guenter

>> +static const struct of_device_id smcwd_dt_ids[] = {
>> +       { .compatible = "mediatek,mt8173-smc-wdt" },
>> +       {}
>> +};
>> +MODULE_DEVICE_TABLE(of, smcwd_dt_ids);
> 
> So I'm a bit confused about this... I thought the plan was to either
> use arm,smc-id and then there'll be no reason to put platform-specific
> quirks into the driver, so we can just use a generic "arm,smc-wdt"
> compatible string on all platforms; or we put individual compatible
> strings for each platform and use them to hardcode platform-specific
> differences (like the SMC ID) in the driver. But now you're kinda
> doing both by making the driver code platform-independent but still
> using a platform-specific compatible string, that doesn't seem to fit
> together. (If the driver can be platform independent, I think it's
> nicer to have a generic compatible string so that future platforms
> which support the same interface don't have to land code changes in
> order to just use the driver.)
>