| 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: <925d86d9-4a7f-53e3-b62f-dc77904fa285@roeck-us.net>
Date: Tue, 16 Jan 2018 14:37:41 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Lyude Paul <lyude@...hat.com>
Cc: Wim Van Sebroeck <wim@...ana.be>, linux-watchdog@...r.kernel.org,
linux-kernel@...r.kernel.org,
Zoltán Böszörményi <zboszor@...hu>
Subject: Re: [01/12] watchdog: sp5100_tco: Always use SP5100_IO_PM_{INDEX_REG,
DATA_REG}
On 01/16/2018 12:17 PM, Lyude Paul wrote:
> Oh has this patchset already been reviwed and pushed upstream? I checked
> patchwork beforehand and it looked like it was still pending
>
Depends on Wim what he wants to do with it. We'll see when the commit window
opens.
Guenter
> On Tue, 2018-01-16 at 12:11 -0800, Guenter Roeck wrote:
>> On Tue, Jan 16, 2018 at 02:34:19PM -0500, Lyude Paul wrote:
>>> Sorry this review took so long! Just sent off a big patchset myself to
>>> nouveau's mailing list.
>>>
>>> Anyway, unfortunately it should be noted that as I've learned from this
>>> thread
>>> that I have no machines that i know of with an actual sp5100_tco. Might
>>> double
>>> check though to see if I'm wrong, but for now this is all solely review.
>>>
>>> Overall, nice patch! I'm glad to see ugly corners of the kernel being
>>> cleaned
>>> up like this :). Some comments below
>>>
>>> On Sun, 2017-12-24 at 13:03 -0800, Guenter Roeck wrote:
>>>> SP5100_IO_PM_INDEX_REG and SB800_IO_PM_INDEX_REG are used inconsistently
>>>> and define the same value. Just use SP5100_IO_PM_INDEX_REG throughout.
>>>> Do the same for SP5100_IO_PM_DATA_REG and SB800_IO_PM_DATA_REG.
>>>> Use helper functions to access the indexed registers.
>>>>
>>>> Cc: Zoltán Böszörményi <zboszor@...hu>
>>>> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
>>>> ---
>>>> drivers/watchdog/sp5100_tco.c | 94 ++++++++++++++++++++++------------
>>>> ----
>>>> -----
>>>> drivers/watchdog/sp5100_tco.h | 7 +---
>>>> 2 files changed, 51 insertions(+), 50 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/sp5100_tco.c
>>>> b/drivers/watchdog/sp5100_tco.c
>>>> index 028618c5eeba..05f9d27a306a 100644
>>>> --- a/drivers/watchdog/sp5100_tco.c
>>>> +++ b/drivers/watchdog/sp5100_tco.c
>>>> @@ -48,7 +48,6 @@
>>>> static u32 tcobase_phys;
>>>> static u32 tco_wdt_fired;
>>>> static void __iomem *tcobase;
>>>> -static unsigned int pm_iobase;
>>>> static DEFINE_SPINLOCK(tco_lock); /* Guards the hardware */
>>>> static unsigned long timer_alive;
>>>> static char tco_expect_close;
>>>> @@ -132,25 +131,38 @@ static int tco_timer_set_heartbeat(int t)
>>>> return 0;
>>>> }
>>>>
>>>> -static void tco_timer_enable(void)
>>>> +static u8 sp5100_tco_read_pm_reg8(u8 index)
>>>> +{
>>>> + outb(index, SP5100_IO_PM_INDEX_REG);
>>>> + return inb(SP5100_IO_PM_DATA_REG);
>>>> +}
>>>> +
>>>> +static void sp5100_tco_update_pm_reg8(u8 index, u8 reset, u8 set)
>>>> {
>>>> - int val;
>>>> + u8 val;
>>>>
>>>> + outb(index, SP5100_IO_PM_INDEX_REG);
>>>> + val = inb(SP5100_IO_PM_DATA_REG);
>>>> + val &= reset;
>>>> + val |= set;
>>>> + outb(val, SP5100_IO_PM_DATA_REG);
>>>> +}
>>>> +
>>>> +static void tco_timer_enable(void)
>>>> +{
>>>> if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
>>>> /* For SB800 or later */
>>>> /* Set the Watchdog timer resolution to 1 sec */
>>>> - outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
>>>> - val = inb(SB800_IO_PM_DATA_REG);
>>>> - val |= SB800_PM_WATCHDOG_SECOND_RES;
>>>> - outb(val, SB800_IO_PM_DATA_REG);
>>>> + sp5100_tco_update_pm_reg8(SB800_PM_WATCHDOG_CONFIG,
>>>> + 0xff,
>>>> SB800_PM_WATCHDOG_SECOND_RES);
>>>>
>>>> /* Enable watchdog decode bit and watchdog timer */
>>>> - outb(SB800_PM_WATCHDOG_CONTROL, SB800_IO_PM_INDEX_REG);
>>>> - val = inb(SB800_IO_PM_DATA_REG);
>>>> - val |= SB800_PCI_WATCHDOG_DECODE_EN;
>>>> - val &= ~SB800_PM_WATCHDOG_DISABLE;
>>>> - outb(val, SB800_IO_PM_DATA_REG);
>>>> + sp5100_tco_update_pm_reg8(SB800_PM_WATCHDOG_CONTROL,
>>>> + ~SB800_PM_WATCHDOG_DISABLE,
>>>> + SB800_PCI_WATCHDOG_DECODE_EN)
>>>> ;
>>>> } else {
>>>> + u32 val;
>>>> +
>>>> /* For SP5100 or SB7x0 */
>>>> /* Enable watchdog decode bit */
>>>> pci_read_config_dword(sp5100_tco_pci,
>>>> @@ -164,11 +176,9 @@ static void tco_timer_enable(void)
>>>> val);
>>>>
>>>> /* Enable Watchdog timer and set the resolution to 1
>>>> sec */
>>>> - outb(SP5100_PM_WATCHDOG_CONTROL,
>>>> SP5100_IO_PM_INDEX_REG);
>>>> - val = inb(SP5100_IO_PM_DATA_REG);
>>>> - val |= SP5100_PM_WATCHDOG_SECOND_RES;
>>>> - val &= ~SP5100_PM_WATCHDOG_DISABLE;
>>>> - outb(val, SP5100_IO_PM_DATA_REG);
>>>> + sp5100_tco_update_pm_reg8(SP5100_PM_WATCHDOG_CONTROL,
>>>> + ~SP5100_PM_WATCHDOG_DISABLE,
>>>> + SP5100_PM_WATCHDOG_SECOND_RES
>>>> );
>>>> }
>>>> }
>>>>
>>>> @@ -321,6 +331,17 @@ static const struct pci_device_id
>>>> sp5100_tco_pci_tbl[]
>>>> = {
>>>> };
>>>> MODULE_DEVICE_TABLE(pci, sp5100_tco_pci_tbl);
>>>>
>>>> +static u8 sp5100_tco_read_pm_reg32(u8 index)
>>>> +{
>>>> + u32 val = 0;
>>>> + int i;
>>>> +
>>>> + for (i = 3; i >= 0; i--)
>>>> + val = (val << 8) + sp5100_tco_read_pm_reg8(index + i);
>>>> +
>>>> + return val;
>>>> +}
>>>> +
>>>> /*
>>>> * Init & exit routines
>>>> */
>>>> @@ -329,7 +350,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>>>> struct pci_dev *dev = NULL;
>>>> const char *dev_name = NULL;
>>>> u32 val;
>>>> - u32 index_reg, data_reg, base_addr;
>>>> + u8 base_addr;
>>>>
>>>> /* Match the PCI device */
>>>> for_each_pci_dev(dev) {
>>>> @@ -351,35 +372,25 @@ static unsigned char sp5100_tco_setupdevice(void)
>>>> */
>>>> if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
>>>> dev_name = SP5100_DEVNAME;
>>>> - index_reg = SP5100_IO_PM_INDEX_REG;
>>>> - data_reg = SP5100_IO_PM_DATA_REG;
>>>> base_addr = SP5100_PM_WATCHDOG_BASE;
>>>> } else {
>>>> dev_name = SB800_DEVNAME;
>>>> - index_reg = SB800_IO_PM_INDEX_REG;
>>>> - data_reg = SB800_IO_PM_DATA_REG;
>>>> base_addr = SB800_PM_WATCHDOG_BASE;
>>>> }
>>>>
>>>> /* Request the IO ports used by this driver */
>>>> - pm_iobase = SP5100_IO_PM_INDEX_REG;
>>>> - if (!request_region(pm_iobase, SP5100_PM_IOPORTS_SIZE,
>>>> dev_name)) {
>>>> - pr_err("I/O address 0x%04x already in use\n",
>>>> pm_iobase);
>>>> + if (!request_region(SP5100_IO_PM_INDEX_REG,
>>>> SP5100_PM_IOPORTS_SIZE,
>>>> + dev_name)) {
>>>> + pr_err("I/O address 0x%04x already in use\n",
>>>> + SP5100_IO_PM_INDEX_REG);
>>>
>>> It's good we're printing an error here, but I also don't think (since this
>>> has
>>> happened a couple times already even just on this thread) we should simply
>>> leave the user with the impression that the driver actually failed if the
>>> real
>>> situation is that the user is supposed to use w83627hf_wdt or some other
>>> watchdog driver. Maybe leave a suggestion in this message to the user to
>>> try
>>> another driver, or see if we could improve this module's ability to
>>> realize
>>> that it's not actually supported on the system it's being loaded on?
>>
>> Keep in mind that this is not the only driver in the system which doesn't
>> apply for a given hardware. We would end up with an endless amount of
>> messages if we were to do that.
>>
>>>> goto exit;
>>>> }
>>>>
>>>> /*
>>>> * First, Find the watchdog timer MMIO address from indirect
>>>> I/O.
>>>> + * Low three bits of BASE are reserved.
>>>> */
>>>> - outb(base_addr+3, index_reg);
>>>> - val = inb(data_reg);
>>>> - outb(base_addr+2, index_reg);
>>>> - val = val << 8 | inb(data_reg);
>>>> - outb(base_addr+1, index_reg);
>>>> - val = val << 8 | inb(data_reg);
>>>> - outb(base_addr+0, index_reg);
>>>> - /* Low three bits of BASE are reserved */
>>>> - val = val << 8 | (inb(data_reg) & 0xf8);
>>>> + val = sp5100_tco_read_pm_reg32(base_addr) & 0xfffffff8;
>>>>
>>>> pr_debug("Got 0x%04x from indirect I/O\n", val);
>>>>
>>>> @@ -400,14 +411,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>>>> SP5100_SB_RESOURCE_MMIO_BASE,
>>>> &val);
>>>> } else {
>>>> /* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
>>>> - outb(SB800_PM_ACPI_MMIO_EN+3, SB800_IO_PM_INDEX_REG);
>>>> - val = inb(SB800_IO_PM_DATA_REG);
>>>> - outb(SB800_PM_ACPI_MMIO_EN+2, SB800_IO_PM_INDEX_REG);
>>>> - val = val << 8 | inb(SB800_IO_PM_DATA_REG);
>>>> - outb(SB800_PM_ACPI_MMIO_EN+1, SB800_IO_PM_INDEX_REG);
>>>> - val = val << 8 | inb(SB800_IO_PM_DATA_REG);
>>>> - outb(SB800_PM_ACPI_MMIO_EN+0, SB800_IO_PM_INDEX_REG);
>>>> - val = val << 8 | inb(SB800_IO_PM_DATA_REG);
>>>> + val = sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN);
>>>> }
>>>>
>>>> /* The SBResource_MMIO is enabled and mapped memory space? */
>>>> @@ -470,7 +474,7 @@ static unsigned char sp5100_tco_setupdevice(void)
>>>> unreg_mem_region:
>>>> release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
>>>> unreg_region:
>>>> - release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>>>> + release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>>>> exit:
>>>> return 0;
>>>> }
>>>> @@ -517,7 +521,7 @@ static int sp5100_tco_init(struct platform_device
>>>> *dev)
>>>> exit:
>>>> iounmap(tcobase);
>>>> release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
>>>> - release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>>>> + release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>>>> return ret;
>>>> }
>>>>
>>>> @@ -531,7 +535,7 @@ static void sp5100_tco_cleanup(void)
>>>> misc_deregister(&sp5100_tco_miscdev);
>>>> iounmap(tcobase);
>>>> release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
>>>> - release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>>>> + release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>>>> }
>>>>
>>>> static int sp5100_tco_remove(struct platform_device *dev)
>>>> diff --git a/drivers/watchdog/sp5100_tco.h
>>>> b/drivers/watchdog/sp5100_tco.h
>>>> index 1af4dee71337..f495fe03887a 100644
>>>> --- a/drivers/watchdog/sp5100_tco.h
>>>> +++ b/drivers/watchdog/sp5100_tco.h
>>>> @@ -24,10 +24,11 @@
>>>> * read them from a register.
>>>> */
>>>>
>>>> -/* For SP5100/SB7x0 chipset */
>>>> +/* For SP5100/SB7x0/SB8x0 chipset */
>>>> #define SP5100_IO_PM_INDEX_REG 0xCD6
>>>> #define SP5100_IO_PM_DATA_REG 0xCD7
>>>>
>>>> +/* For SP5100/SB7x0 chipset */
>>>> #define SP5100_SB_RESOURCE_MMIO_BASE 0x9C
>>>>
>>>> #define SP5100_PM_WATCHDOG_CONTROL 0x69
>>>> @@ -44,11 +45,7 @@
>>>>
>>>> #define SP5100_DEVNAME "SP5100 TCO"
>>>>
>>>> -
>>>> /* For SB8x0(or later) chipset */
>>>> -#define SB800_IO_PM_INDEX_REG 0xCD6
>>>> -#define SB800_IO_PM_DATA_REG 0xCD7
>>>> -
>>>
>>> IMO I would leave these macro definitions as SB800. For DRM drivers at
>>> least,
>>> we usually take the route of naming commonly used registers/methods after
>>> the
>>> first generation they appeared in, not the last.
>>>
>>
>> That may apply if there is only one set of defines. We have two
>> sets here. Agreed, I could have removed SP5100 instead.
>> As it is, I'd rather have the series applied to v4.16 instead
>> of making cosmetic changes. I'll consider your suggestion if the
>> series does not make it into 4.16.
>>
>> Thanks,
>> Guenter
>>
>>>> #define SB800_PM_ACPI_MMIO_EN 0x24
>>>> #define SB800_PM_WATCHDOG_CONTROL 0x48
>>>> #define SB800_PM_WATCHDOG_BASE 0x48
>>>
>>> --
>>> Cheers,
>>> Lyude Paul
Powered by blists - more mailing lists