linux-kernel - Re: [PATCH v6 5/6] platform/x86: intel_pmc_ipc: Fix iTCO

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 7 Apr 2017 00:37:54 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
Cc:     Andy Shevchenko <andy@...radead.org>,
        Zha Qipeng <qipeng.zha@...el.com>,
        "dvhart@...radead.org" <dvhart@...radead.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Wim Van Sebroeck <wim@...ana.be>,
        Sathyanarayanan Kuppuswamy Natarajan <sathyaosid@...il.com>,
        David Box <david.e.box@...ux.intel.com>,
        Rajneesh Bhardwaj <rajneesh.bhardwaj@...el.com>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-watchdog@...r.kernel.org
Subject: Re: [PATCH v6 5/6] platform/x86: intel_pmc_ipc: Fix iTCO_wdt GCS
 memory mapping failure

On Thu, Apr 6, 2017 at 1:54 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@...ux.intel.com> wrote:
> iTCO_wdt driver need access to PMC_CFG GCR register to modify the
> noreboot setting. Currently, this is done by passing PMC_CFG reg
> address as memory resource to watchdog driver and allowing it directly
> modify the PMC_CFG register. But currently PMC driver also has
> requirement to memory map the entire GCR register space in this driver.
> This causes mem request failure in watchdog driver. So this patch fixes
> this issue by adding API to update noreboot flag and passes them
> to watchdog driver via platform data.

> +/* PMC_CFG_REG bit masks */
> +#define PMC_CFG_NO_REBOOT_MASK         BIT(4)

> +#define PMC_CFG_NO_REBOOT_ENABLE       BIT(4)
> +#define PMC_CFG_NO_REBOOT_DISABLE      0

I would go for those like
(1 << 4)
and
(0 << 4)
which for my opinion is better to get a picture.

> +static int update_no_reboot_bit(void *priv, bool set)
> +{
> +       if (set)
> +               return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
> +                                           PMC_CFG_NO_REBOOT_MASK,
> +                                           PMC_CFG_NO_REBOOT_ENABLE);
> +
> +       return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
> +                                   PMC_CFG_NO_REBOOT_MASK,
> +                                   PMC_CFG_NO_REBOOT_DISABLE);

u32 value = set ? PMC_CFG_NO_REBOOT_ENABLE : PMC_CFG_NO_REBOOT_DISABLE;

(also you may consider to use just *_EN and *_DIS)

return intel_pmc_gcr_update(PMC_GCR_PMC_CFG_REG,
                                   PMC_CFG_NO_REBOOT_MASK,
                                   value);
> +}

-- 
With Best Regards,
Andy Shevchenko