| 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: Wed, 19 Jan 2022 07:46:52 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Andy Shevchenko <andy.shevchenko@...il.com>,
Terry Bowman <terry.bowman@....com>
Cc: linux-watchdog@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
linux-i2c <linux-i2c@...r.kernel.org>,
Wolfram Sang <wsa@...nel.org>,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Wim Van Sebroeck <wim@...ux-watchdog.org>,
Robert Richter <rrichter@....com>,
Tom Lendacky <thomas.lendacky@....com>,
"Shah, Nehal-bakulchandra" <Nehal-bakulchandra.Shah@....com>,
Basavaraj Natikar <Basavaraj.Natikar@....com>,
Shyam Sundar S K <Shyam-sundar.S-k@....com>,
Mario Limonciello <Mario.Limonciello@....com>
Subject: Re: [PATCH v3 2/4] Watchdog: sp5100_tco: Refactor MMIO base address
initialization
On 1/19/22 3:53 AM, Andy Shevchenko wrote:
> On Tue, Jan 18, 2022 at 10:23 PM Terry Bowman <terry.bowman@....com> wrote:
>>
>> Combine MMIO base address and alternate base address detection. Combine
>> based on layout type. This will simplify the function by eliminating
>> a switch case.
>>
>> Move existing request/release code into functions. This currently only
>> supports port I/O request/release. The move into a separate function
>> will make it ready for adding MMIO region support.
>
> ...
>
>> To: Guenter Roeck <linux@...ck-us.net>
>> To: linux-watchdog@...r.kernel.org
>> To: Jean Delvare <jdelvare@...e.com>
>> To: linux-i2c@...r.kernel.org
>> To: Wolfram Sang <wsa@...nel.org>
>> To: Andy Shevchenko <andy.shevchenko@...il.com>
>> To: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>> Cc: linux-kernel@...r.kernel.org
>> Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>
>> Cc: Robert Richter <rrichter@....com>
>> Cc: Thomas Lendacky <thomas.lendacky@....com>
>
> Same comment to all your patches.
>
> ...
>
>> +static int __sp5100_tco_prepare_base(struct sp5100_tco *tco,
>> + u32 mmio_addr,
>> + const char *dev_name)
>> +{
>> + struct device *dev = tco->wdd.parent;
>
>> + int ret = 0;
>
> Not really used variable.
>
>> + if (!mmio_addr)
>> + return -ENOMEM;
>> +
>> + if (!devm_request_mem_region(dev, mmio_addr,
>> + SP5100_WDT_MEM_MAP_SIZE,
>> + dev_name)) {
>> + dev_dbg(dev, "MMIO address 0x%08x already in use\n",
>> + mmio_addr);
>> + return -EBUSY;
>> + }
>> +
>> + tco->tcobase = devm_ioremap(dev, mmio_addr,
>> + SP5100_WDT_MEM_MAP_SIZE);
Talking about line splits, is this one necessary ?
>> + if (!tco->tcobase) {
>> + dev_dbg(dev, "MMIO address 0x%08x failed mapping.\n",
>> + mmio_addr);
>
>> + devm_release_mem_region(dev, mmio_addr,
>> + SP5100_WDT_MEM_MAP_SIZE);
>
> Why? If it's a short live mapping, do not use devm.
>
This is not short lived; it is needed by the driver. The release
is an artifact of calling this function twice and ignoring the error
from devm_ioremap() if the first call fails. devm_release_mem_region()
isn't strictly needed but that would result in keeping the memory
region reserved even though it isn't used by the driver.
There is a functional difference to the original code, though.
The failing devm_ioremap() causes the code to try the alternate
address. I am not sure if that really adds value; devm_ioremap()
fails because the system is out of virtual memory, and calling
it again on a different address doesn't seem to add much value.
I preferred the original code, which would only call devm_ioremap()
after successfully reserving a memory region.
>> + return -ENOMEM;
>> + }
>
>> + dev_info(dev, "Using 0x%08x for watchdog MMIO address\n",
>> + mmio_addr);
>
> Unneeded noise.
>
>> + return ret;
>
> On top of above it's a NIH devm_ioremap_resource().
>
Not sure what NIH means, but if you refer to the lack of
devm_release_mem_region(), again, it isn't short lived.
>> +}
>
>
> ...
>
>> + int ret = 0;
>
> Redundant assignment.
>
> ...
>
>> + /* Check MMIO address conflict */
>> + ret = __sp5100_tco_prepare_base(tco, mmio_addr, dev_name);
>
>> +
>> + /* Check alternate MMIO address conflict */
>
> Unify this with the previous comment.
>
Why ? It refers to the code below. If that is a single or two comments
is really just POV.
>> + if (ret)
>> + ret = __sp5100_tco_prepare_base(tco, alt_mmio_addr,
>> + dev_name);
>
> ...
>
>> + if (alt_mmio_addr & ((SB800_ACPI_MMIO_DECODE_EN |
>> + SB800_ACPI_MMIO_SEL) !=
>> + SB800_ACPI_MMIO_DECODE_EN)) {
>
> The split looks ugly. Consider to use temporary variables or somehow
> rearrange the condition that it doesn't break in the middle of the one
> logical token.
Split at the &, maybe.
>
>> + alt_mmio_addr &= ~0xFFF;
>
> Why capital letters?
>
>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
>> + }
>
> ...
>
>> + if (!(alt_mmio_addr & (((SB800_ACPI_MMIO_DECODE_EN |
>> + SB800_ACPI_MMIO_SEL)) !=
>> + SB800_ACPI_MMIO_DECODE_EN))) {
>
> Ditto.
>
>> + alt_mmio_addr &= ~0xFFF;
>
> Ditto.
>
>> + alt_mmio_addr += SB800_PM_WDT_MMIO_OFFSET;
>
> ...
>
> Okay, I see this is the original code like this... Perhaps it makes
> sense to reshuffle them (indentation-wise) at the same time and
> mention this in the changelog.
>
> ...
>
>> release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
>
> Is it still needed? I have no context to say if devm_iomap() and this
> are not colliding, please double check the correctness.
>
Not sure I understand. This is the release of the io region reserved with
request_muxed_region() at the beginning of this function. Why would it no
longer be necessary to release that region ?
Guenter
Powered by blists - more mailing lists